On Tue, Jul 19, 2022 at 3:39 AM <[email protected]> wrote:
>
> From: Numan Siddique <[email protected]>
>
> ovsdb-server allows the OVSDB clients to specify the uuid for
> the row inserts [1]. The C IDL client library is missing this
> feature. This patch adds this support.
>
> For each schema table, a new function is generated -
> <schema_table>insert_persistent_uuid(txn, uuid) and the users
> of IDL client library can make use of this function.
>
> ovs-vsctl and other derivatives of ctl now supports the same
> in the generic 'create' command with the option "--id=<UUID>".
>
> [1] - a529e3cd1f("ovsdb-server: Allow OVSDB clients to specify the UUID for
> inserted rows.:)
>
> Signed-off-by: Numan Siddique <[email protected]>
> Acked-by: Adrian Moreno <[email protected]>
> Acked-by: Han Zhou <[email protected]>
Gentle ping.
Thanks
Numan
> ---
>
> v4 -> v5
> ---
> * Addressed review comments from Ilya.
> - Added NEWS item entry.
>
> v3 -> v4
> ---
> * Added an entry in python/TODO.rst.
>
> v2 -> v3
> ----
> * Addressed review comments from Han
> - Added test case for --id ctl option
>
> v1 -> v2
> -----
> * Addressed review comments from Adrian Moreno
> * Added the support in generic 'create' command to specify the uuid in
> --id option.
>
> NEWS | 3 +-
> lib/db-ctl-base.c | 38 ++++++++++++------
> lib/db-ctl-base.man | 5 ++-
> lib/db-ctl-base.xml | 6 ++-
> lib/ovsdb-idl-provider.h | 1 +
> lib/ovsdb-idl.c | 85 +++++++++++++++++++++++++++++-----------
> lib/ovsdb-idl.h | 3 ++
> ovsdb/ovsdb-idlc.in | 15 +++++++
> python/TODO.rst | 2 +
> tests/ovs-vsctl.at | 25 ++++++++++++
> tests/ovsdb-idl.at | 27 +++++++++++++
> tests/test-ovsdb.c | 59 ++++++++++++++++++++++++++++
> 12 files changed, 231 insertions(+), 38 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 024fa4428..41316da14 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -1,6 +1,7 @@
> Post-v3.0.0
> --------------------
> -
> + - OVSDB-IDL:
> + * Add the support to specify the uuid for row insert.
>
> v3.0.0 - xx xxx xxxx
> --------------------
> diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
> index bc85e9921..856832a04 100644
> --- a/lib/db-ctl-base.c
> +++ b/lib/db-ctl-base.c
> @@ -1731,29 +1731,43 @@ cmd_create(struct ctl_context *ctx)
> const struct ovsdb_idl_table_class *table;
> const struct ovsdb_idl_row *row;
> const struct uuid *uuid = NULL;
> + bool persist_uuid = false;
> + struct uuid uuid_;
> int i;
>
> ctx->error = get_table(table_name, &table);
> if (ctx->error) {
> return;
> }
> +
> if (id) {
> - struct ovsdb_symbol *symbol = NULL;
> + if (uuid_from_string(&uuid_, id)) {
> + uuid = &uuid_;
> + persist_uuid = true;
> + } else {
> + struct ovsdb_symbol *symbol = NULL;
>
> - ctx->error = create_symbol(ctx->symtab, id, &symbol, NULL);
> - if (ctx->error) {
> - return;
> - }
> - if (table->is_root) {
> - /* This table is in the root set, meaning that rows created in it
> - * won't disappear even if they are unreferenced, so disable
> - * warnings about that by pretending that there is a reference.
> */
> - symbol->strong_ref = true;
> + ctx->error = create_symbol(ctx->symtab, id, &symbol, NULL);
> + if (ctx->error) {
> + return;
> + }
> + if (table->is_root) {
> + /* This table is in the root set, meaning that rows created
> in
> + * it won't disappear even if they are unreferenced, so
> disable
> + * warnings about that by pretending that there is a
> + * reference. */
> + symbol->strong_ref = true;
> + }
> + uuid = &symbol->uuid;
> }
> - uuid = &symbol->uuid;
> }
>
> - row = ovsdb_idl_txn_insert(ctx->txn, table, uuid);
> + if (persist_uuid) {
> + row = ovsdb_idl_txn_insert_persist_uuid(ctx->txn, table, uuid);
> + } else {
> + row = ovsdb_idl_txn_insert(ctx->txn, table, uuid);
> + }
> +
> for (i = 2; i < ctx->argc; i++) {
> ctx->error = set_column(table, row, ctx->argv[i], ctx->symtab);
> if (ctx->error) {
> diff --git a/lib/db-ctl-base.man b/lib/db-ctl-base.man
> index a529d8b4d..c8111c9ef 100644
> --- a/lib/db-ctl-base.man
> +++ b/lib/db-ctl-base.man
> @@ -203,7 +203,7 @@ Without \fB\-\-if-exists\fR, it is an error if
> \fIrecord\fR does not
> exist. With \fB\-\-if-exists\fR, this command does nothing if
> \fIrecord\fR does not exist.
> .
> -.IP "[\fB\-\-id=@\fIname\fR] \fBcreate\fR \fItable
> column\fR[\fB:\fIkey\fR]\fB=\fIvalue\fR..."
> +.IP "[\fB\-\-id=(@\fIname\fR | \fIuuid\fR] \fBcreate\fR \fItable
> column\fR[\fB:\fIkey\fR]\fB=\fIvalue\fR..."
> Creates a new record in \fItable\fR and sets the initial values of
> each \fIcolumn\fR. Columns not explicitly set will receive their
> default values. Outputs the UUID of the new row.
> @@ -212,6 +212,9 @@ If \fB@\fIname\fR is specified, then the UUID for the new
> row may be
> referred to by that name elsewhere in the same \fB\*(PN\fR
> invocation in contexts where a UUID is expected. Such references may
> precede or follow the \fBcreate\fR command.
> +.IP
> +If a valid \fIuuid\fR is specified, then it is used as the UUID
> +of the new row.
> .
> .RS
> .IP "Caution (ovs-vsctl as example)"
> diff --git a/lib/db-ctl-base.xml b/lib/db-ctl-base.xml
> index f6efe98ea..27c999fe7 100644
> --- a/lib/db-ctl-base.xml
> +++ b/lib/db-ctl-base.xml
> @@ -310,7 +310,7 @@
> </p>
> </dd>
>
> - <dt>[<code>--id=@</code><var>name</var>] <code>create</code> <var>table
> column</var>[<code>:</code><var>key</var>]<code>=</code><var>value</var>...</dt>
> + <dt>[<code>--id=(@</code><var>name</var>|<var>uuid</var>)]
> <code>create</code> <var>table
> column</var>[<code>:</code><var>key</var>]<code>=</code><var>value</var>...</dt>
> <dd>
> <p>
> Creates a new record in <var>table</var> and sets the initial values
> of
> @@ -323,6 +323,10 @@
> invocation in contexts where a UUID is expected. Such references may
> precede or follow the <code>create</code> command.
> </p>
> + <p>
> + If a valid <var>uuid</var> is specified, then it is used as the
> + UUID of the new row.
> + </p>
> <dl>
> <dt>Caution (ovs-vsctl as example)</dt>
> <dd>
> diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h
> index 8797686f9..8d2b7d6b9 100644
> --- a/lib/ovsdb-idl-provider.h
> +++ b/lib/ovsdb-idl-provider.h
> @@ -74,6 +74,7 @@ struct ovsdb_idl_row {
> struct ovs_list dst_arcs; /* Backward arcs (ovsdb_idl_arc.dst_node). */
> struct ovsdb_idl_table *table; /* Containing table. */
> struct ovsdb_datum *old_datum; /* Committed data (null if orphaned). */
> + bool persist_uuid; /* Persist 'uuid' during insert txn if set.
> */
> bool parsed; /* Whether the row is parsed. */
> struct ovs_list reparse_node; /* Rows that needs to be re-parsed due to
> * insertion of a referenced row. */
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index 576979f9e..9a10d91e4 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -2851,11 +2851,14 @@ substitute_uuids(struct json *json, const struct
> ovsdb_idl_txn *txn)
>
> row = ovsdb_idl_txn_get_row(txn, &uuid);
> if (row && !row->old_datum && row->new_datum) {
> - json_destroy(json);
> -
> - return json_array_create_2(
> - json_string_create("named-uuid"),
> - json_string_create_nocopy(ovsdb_data_row_name(&uuid)));
> + if (row->persist_uuid) {
> + return json;
> + } else {
> + json_destroy(json);
> + return json_array_create_2(
> + json_string_create("named-uuid"),
> +
> json_string_create_nocopy(ovsdb_data_row_name(&uuid)));
> + }
> }
> }
>
> @@ -3280,9 +3283,19 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn)
>
> any_updates = true;
>
> - json_object_put(op, "uuid-name",
> - json_string_create_nocopy(
> - ovsdb_data_row_name(&row->uuid)));
> + char *uuid_json;
> + struct json *value;
> + if (row->persist_uuid) {
> + uuid_json = "uuid";
> + value = json_string_create_nocopy(
> + xasprintf(UUID_FMT, UUID_ARGS(&row->uuid)));
> + } else {
> + uuid_json = "uuid-name";
> + value = json_string_create_nocopy(
> + ovsdb_data_row_name(&row->uuid));
> + }
> +
> + json_object_put(op, uuid_json, value);
>
> insert = xmalloc(sizeof *insert);
> insert->dummy = row->uuid;
> @@ -3766,6 +3779,31 @@ ovsdb_idl_txn_delete(const struct ovsdb_idl_row *row_)
> row->new_datum = NULL;
> }
>
> +static const struct ovsdb_idl_row *
> +ovsdb_idl_txn_insert__(struct ovsdb_idl_txn *txn,
> + const struct ovsdb_idl_table_class *class,
> + const struct uuid *uuid,
> + bool persist_uuid)
> +{
> + struct ovsdb_idl_row *row = ovsdb_idl_row_create__(class);
> +
> + ovs_assert(uuid || !persist_uuid);
> + if (uuid) {
> + ovs_assert(!ovsdb_idl_txn_get_row(txn, uuid));
> + row->uuid = *uuid;
> + } else {
> + uuid_generate(&row->uuid);
> + }
> + row->persist_uuid = persist_uuid;
> + row->table = ovsdb_idl_table_from_class(txn->idl, class);
> + row->new_datum = xmalloc(class->n_columns * sizeof *row->new_datum);
> + hmap_insert(&row->table->rows, &row->hmap_node, uuid_hash(&row->uuid));
> + hmap_insert(&txn->txn_rows, &row->txn_node, uuid_hash(&row->uuid));
> + ovsdb_idl_add_to_indexes(row);
> +
> + return row;
> +}
> +
> /* Inserts and returns a new row in the table with the specified 'class' in
> the
> * database with open transaction 'txn'.
> *
> @@ -3783,22 +3821,23 @@ ovsdb_idl_txn_insert(struct ovsdb_idl_txn *txn,
> const struct ovsdb_idl_table_class *class,
> const struct uuid *uuid)
> {
> - struct ovsdb_idl_row *row = ovsdb_idl_row_create__(class);
> -
> - if (uuid) {
> - ovs_assert(!ovsdb_idl_txn_get_row(txn, uuid));
> - row->uuid = *uuid;
> - } else {
> - uuid_generate(&row->uuid);
> - }
> -
> - row->table = ovsdb_idl_table_from_class(txn->idl, class);
> - row->new_datum = xmalloc(class->n_columns * sizeof *row->new_datum);
> - hmap_insert(&row->table->rows, &row->hmap_node, uuid_hash(&row->uuid));
> - hmap_insert(&txn->txn_rows, &row->txn_node, uuid_hash(&row->uuid));
> - ovsdb_idl_add_to_indexes(row);
> + return ovsdb_idl_txn_insert__(txn, class, uuid, false);
> +}
>
> - return row;
> +/* Inserts and returns a new row in the table with the specified 'class' in
> the
> + * database with open transaction 'txn'.
> + *
> + * The new row is assigned the specified UUID (which cannot be null).
> + *
> + * Usually this function is used indirectly through one of the
> + * "insert_persist_uuid" functions generated by ovsdb-idlc. */
> +const struct ovsdb_idl_row *
> +ovsdb_idl_txn_insert_persist_uuid(struct ovsdb_idl_txn *txn,
> + const struct ovsdb_idl_table_class *class,
> + const struct uuid *uuid)
> +{
> + ovs_assert(uuid);
> + return ovsdb_idl_txn_insert__(txn, class, uuid, true);
> }
>
> static void
> diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h
> index fbd9f671a..9a3e19f20 100644
> --- a/lib/ovsdb-idl.h
> +++ b/lib/ovsdb-idl.h
> @@ -375,6 +375,9 @@ void ovsdb_idl_txn_delete(const struct ovsdb_idl_row *);
> const struct ovsdb_idl_row *ovsdb_idl_txn_insert(
> struct ovsdb_idl_txn *, const struct ovsdb_idl_table_class *,
> const struct uuid *);
> +const struct ovsdb_idl_row *ovsdb_idl_txn_insert_persist_uuid(
> + struct ovsdb_idl_txn *txn, const struct ovsdb_idl_table_class *class,
> + const struct uuid *uuid);
>
> struct ovsdb_idl *ovsdb_idl_txn_get_idl (struct ovsdb_idl_txn *);
> void ovsdb_idl_get_initial_snapshot(struct ovsdb_idl *);
> diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
> index 5a97a8ea3..9a54f06a1 100755
> --- a/ovsdb/ovsdb-idlc.in
> +++ b/ovsdb/ovsdb-idlc.in
> @@ -362,6 +362,8 @@ struct %(s)s *%(s)s_cursor_data(struct ovsdb_idl_cursor
> *);
> void %(s)s_init(struct %(s)s *);
> void %(s)s_delete(const struct %(s)s *);
> struct %(s)s *%(s)s_insert(struct ovsdb_idl_txn *);
> +struct %(s)s *%(s)s_insert_persist_uuid(
> + struct ovsdb_idl_txn *txn, const struct uuid *uuid);
>
> /* Returns true if the tracked column referenced by 'enum %(s)s_column_id' of
> * the row referenced by 'struct %(s)s *' was updated since the last change
> @@ -809,6 +811,19 @@ struct %(s)s *
> return %(s)s_cast(ovsdb_idl_txn_insert(txn, &%(p)stable_%(tl)s, NULL));
> }
>
> +/* Inserts and returns a new row in the table "%(t)s" in the database
> + * with open transaction 'txn'.
> + *
> + * The new row is assigned the UUID specified in the 'uuid' parameter
> + * (which cannot be null). ovsdb-server will try to assign the same
> + * UUID when 'txn' is committed. */
> +struct %(s)s *
> +%(s)s_insert_persist_uuid(struct ovsdb_idl_txn *txn, const struct uuid *uuid)
> +{
> + return %(s)s_cast(ovsdb_idl_txn_insert_persist_uuid(
> + txn, &%(p)stable_%(tl)s, uuid));
> +}
> +
> bool
> %(s)s_is_updated(const struct %(s)s *row, enum %(s)s_column_id column)
> {
> diff --git a/python/TODO.rst b/python/TODO.rst
> index 3a53489f1..a0b3e807a 100644
> --- a/python/TODO.rst
> +++ b/python/TODO.rst
> @@ -32,3 +32,5 @@ Python Bindings To-do List
>
> * Support write-only-changed monitor mode (equivalent of
> OVSDB_IDL_WRITE_CHANGED_ONLY).
> +
> + * Support accepting the uuid for row inserts.
> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> index d6cd2c084..abf4fb9cf 100644
> --- a/tests/ovs-vsctl.at
> +++ b/tests/ovs-vsctl.at
> @@ -1710,3 +1710,28 @@ ingress_policing_kpkts_rate: 100
> ])
> OVS_VSCTL_CLEANUP
> AT_CLEANUP
> +
> +AT_SETUP([ovs-vsctl create bridge with uuid])
> +AT_KEYWORDS([create bridge with uuid])
> +OVS_VSCTL_SETUP
> +
> +AT_CHECK([ovs-vsctl --no-wait --id=c5cc12f8-eaa1-43a7-8a73-bccd18df1111
> create bridge \
> +name=tst0 -- add open . bridges c5cc12f8-eaa1-43a7-8a73-bccd18df1111],
> [0],[dnl
> +c5cc12f8-eaa1-43a7-8a73-bccd18df1111
> +])
> +
> +AT_CHECK([ovs-vsctl --no-wait --id=c5cc12f8-eaa1-43a7-8a73-bccd18df1111
> create bridge \
> +name=tst1 -- add open . bridges c5cc12f8-eaa1-43a7-8a73-bccd18df1111], [1],
> [ignore], [ignore])
> +
> +AT_CHECK([ovs-vsctl --no-wait --bare --columns _uuid,name list bridge], [0],
> [dnl
> +c5cc12f8-eaa1-43a7-8a73-bccd18df1111
> +tst0
> +])
> +
> +ovs-vsctl --no-wait --id=@a create bridge \
> +name=tst1 -- add open . bridges @a
> +
> +AT_CHECK([ovs-vsctl --no-wait --bare --columns _uuid,name list bridge tst1],
> [0], [ignore])
> +
> +OVS_VSCTL_CLEANUP
> +AT_CLEANUP
> diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
> index d1cfa59c5..28c032105 100644
> --- a/tests/ovsdb-idl.at
> +++ b/tests/ovsdb-idl.at
> @@ -2466,3 +2466,30 @@ unix:socket2 remote has col id in table simple7
>
> OVSDB_SERVER_SHUTDOWN
> AT_CLEANUP
> +
> +AT_SETUP([idl creating rows with persistent uuid - C])
> +AT_KEYWORDS([ovsdb client idl txn])
> +
> +AT_CHECK([ovsdb_start_idltest "" "$abs_srcdir/idltest.ovsschema"])
> +AT_CHECK([test-ovsdb '-vPATTERN:console:test-ovsdb|%c|%m' -vjsonrpc -t10
> idl-txn-persistent-uuid unix:socket],
> + [0], [stdout], [stderr])
> +AT_CHECK([sort stdout], [0],
> + [[000: After inserting simple, simple3 and simple4
> +001: table simple3: name=simple3 uset=[]
> uref=[c5cc12f8-eaa1-43a7-8a73-bccd18df1111]
> uuid=c5cc12f8-eaa1-43a7-8a73-bccd18df2222
> +001: table simple4: name=simple4 uuid=c5cc12f8-eaa1-43a7-8a73-bccd18df1111
> +001: table simple: i=0 r=0 b=false s=simple
> u=00000000-0000-0000-0000-000000000000 ia=[] ra=[] ba=[] sa=[] ua=[]
> uuid=c5cc12f8-eaa1-43a7-8a73-bccd18df3333
> +002: After inserting simple with same uuid
> +003: table simple3: name=simple3 uset=[]
> uref=[c5cc12f8-eaa1-43a7-8a73-bccd18df1111]
> uuid=c5cc12f8-eaa1-43a7-8a73-bccd18df2222
> +003: table simple4: name=simple4 uuid=c5cc12f8-eaa1-43a7-8a73-bccd18df1111
> +003: table simple: i=0 r=0 b=false s=simple
> u=00000000-0000-0000-0000-000000000000 ia=[] ra=[] ba=[] sa=[] ua=[]
> uuid=c5cc12f8-eaa1-43a7-8a73-bccd18df3333
> +004: End test
> +]])
> +
> +AT_CHECK([grep ovsdb_idl stderr | sort], [0], [dnl
> +test-ovsdb|ovsdb_idl|transaction error: {"details":"This UUID would dnl
> +duplicate a UUID already present within the table or deleted within dnl
> +the same transaction.","error":"duplicate
> uuid","syntax":"\"c5cc12f8-eaa1-43a7-8a73-bccd18df3333\""}
> +])
> +
> +OVSDB_SERVER_SHUTDOWN
> +AT_CLEANUP
> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
> index 343833151..fe2760c41 100644
> --- a/tests/test-ovsdb.c
> +++ b/tests/test-ovsdb.c
> @@ -3390,6 +3390,63 @@ do_idl_table_column_check(struct ovs_cmdl_context *ctx)
> ovsdb_idl_destroy(idl);
> }
>
> +static void
> +do_idl_txn_persistent_uuid(struct ovs_cmdl_context *ctx)
> +{
> + struct ovsdb_idl *idl;
> + struct ovsdb_idl_txn *myTxn;
> + int step = 0;
> +
> + idl = ovsdb_idl_create(ctx->argv[1], &idltest_idl_class, true, true);
> + ovsdb_idl_get_initial_snapshot(idl);
> + ovsdb_idl_run(idl);
> +
> + myTxn = ovsdb_idl_txn_create(idl);
> +
> + struct uuid uuid1;
> + uuid_from_string(&uuid1, "c5cc12f8-eaa1-43a7-8a73-bccd18df1111");
> +
> + struct uuid uuid2;
> + uuid_from_string(&uuid2, "c5cc12f8-eaa1-43a7-8a73-bccd18df2222");
> +
> + struct idltest_simple4 *simple4_row =
> + idltest_simple4_insert_persist_uuid(myTxn, &uuid1);
> + idltest_simple4_set_name(simple4_row, "simple4");
> +
> + struct idltest_simple3 *simple3_row =
> + idltest_simple3_insert_persist_uuid(myTxn, &uuid2);
> + idltest_simple3_set_name(simple3_row, "simple3");
> + idltest_simple3_set_uref(simple3_row, &simple4_row, 1);
> +
> + struct uuid uuid3;
> + uuid_from_string(&uuid3, "c5cc12f8-eaa1-43a7-8a73-bccd18df3333");
> +
> + struct idltest_simple *simple_row =
> + idltest_simple_insert_persist_uuid(myTxn, &uuid3);
> + idltest_simple_set_s(simple_row, "simple");
> +
> + ovsdb_idl_txn_commit_block(myTxn);
> + ovsdb_idl_txn_destroy(myTxn);
> + ovsdb_idl_get_initial_snapshot(idl);
> + printf("%03d: After inserting simple, simple3 and simple4\n", step++);
> + print_idl(idl, step++, false);
> +
> + /* Create another txn, insert the row in simple table with the existing
> + * uuid. */
> + myTxn = ovsdb_idl_txn_create(idl);
> + simple_row =
> + idltest_simple_insert_persist_uuid(myTxn, &uuid3);
> + idltest_simple_set_s(simple_row, "simple_foo");
> + ovsdb_idl_txn_commit_block(myTxn);
> + ovsdb_idl_txn_destroy(myTxn);
> + ovsdb_idl_get_initial_snapshot(idl);
> + printf("%03d: After inserting simple with same uuid\n", step++);
> + print_idl(idl, step++, false);
> +
> + ovsdb_idl_destroy(idl);
> + printf("%03d: End test\n", step++);
> +}
> +
> static struct ovs_cmdl_command all_commands[] = {
> { "log-io", NULL, 2, INT_MAX, do_log_io, OVS_RO },
> { "default-atoms", NULL, 0, 0, do_default_atoms, OVS_RO },
> @@ -3430,6 +3487,8 @@ static struct ovs_cmdl_command all_commands[] = {
> do_idl_partial_update_set_column, OVS_RO },
> { "idl-table-column-check", NULL, 2, 2,
> do_idl_table_column_check, OVS_RO },
> + { "idl-txn-persistent-uuid", NULL, 1, INT_MAX,
> + do_idl_txn_persistent_uuid, OVS_RO },
> { "help", NULL, 0, INT_MAX, do_help, OVS_RO },
> { NULL, NULL, 0, 0, NULL, OVS_RO },
> };
> --
> 2.36.1
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
