On Tue, Apr 26, 2022 at 7:39 AM Adrian Moreno <[email protected]> wrote: > > Hi Numan, > > Apart from a couple of small coments below, I'm wondering if this is a good > time > to surface this to ovs-*ctl commands. What do you think?
Hi Adrian, Thanks for the review. I thought about ovs-*ctl commands. I guess it would be too much to support this in all the add commands (like add-br, add-port, and ovn commands - ovn-nbctl ls-add, lsp-add etc). Instead, would it be fine to add the support in the generic "create" command ? Right now there is an option "--id=@name" supported in the create command. I think we can support something like - ovs-vsctl --id=<UUID> create ... What do you think ? I've incorporated it in v2. Please check it out. > > On 3/8/22 01:56, [email protected] wrote: > > From: Numan Siddique <[email protected]> > > > > ovsdb-server already supports specifying the uuid in the insert > > transaction by the client. But the C IDL client library was > > 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. > > > > Signed-off-by: Numan Siddique <[email protected]> > > --- > > lib/ovsdb-idl-provider.h | 1 + > > lib/ovsdb-idl.c | 87 ++++++++++++++++++++++++++++++---------- > > lib/ovsdb-idl.h | 3 ++ > > ovsdb/ovsdb-idlc.in | 15 +++++++ > > tests/ovsdb-idl.at | 36 +++++++++++++++++ > > tests/test-ovsdb.c | 59 +++++++++++++++++++++++++++ > > 6 files changed, 179 insertions(+), 22 deletions(-) > > > > diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h > > index 8797686f9..332c4075b 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 c19128d55..5434f9443 100644 > > --- a/lib/ovsdb-idl.c > > +++ b/lib/ovsdb-idl.c > > @@ -2800,10 +2800,16 @@ 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_array_create_2( > > + json_string_create("uuid"), > > + json_string_create_nocopy( > > + xasprintf(UUID_FMT, UUID_ARGS(&uuid)))); > > Is the creation of another json object really needed in this case? Isn't this > object the same as the provided one? Great catch. I missed it totally. Addressed in v2. > > > > + } else { > > + return json_array_create_2( > > + json_string_create("named-uuid"), > > + > > json_string_create_nocopy(ovsdb_data_row_name(&uuid))); > > + } > > } > > } > > > > @@ -3228,9 +3234,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; > > @@ -3706,6 +3722,32 @@ 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); > > + > > + if (uuid) { > > + ovs_assert(!ovsdb_idl_txn_get_row(txn, uuid)); > > + row->uuid = *uuid; > > + row->persist_uuid = persist_uuid; > > + } else { > > + uuid_generate(&row->uuid); > > + row->persist_uuid = false; > > + } > > + > > + 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'. > > * > > @@ -3723,22 +3765,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 d00599616..d36446dbb 100644 > > --- a/lib/ovsdb-idl.h > > +++ b/lib/ovsdb-idl.h > > @@ -363,6 +363,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 10a70ae26..09627fede 100755 > > --- a/ovsdb/ovsdb-idlc.in > > +++ b/ovsdb/ovsdb-idlc.in > > @@ -347,6 +347,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 > > @@ -794,6 +796,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/tests/ovsdb-idl.at b/tests/ovsdb-idl.at > > index 62e2b6383..3a9819bc2 100644 > > --- a/tests/ovsdb-idl.at > > +++ b/tests/ovsdb-idl.at > > @@ -2437,3 +2437,39 @@ 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]) > > + > > +# idltest2.ovsschema is the same as idltest.ovsschema, except that > > +# few tables and columns are missing. This test checks that idl doesn't > > +# include the missing tables and columns in the transactions. > > +# idl-missing-table-column-txn inserts > > +# - a row for table - 'simple' > > +# - a row for table - 'simple5' which is missing. This should not be > > +# included in the transaction. > > +# - a row for table - 'simple7 with the missing column 'id'. > > + > > +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, simple5 and simple7 > > +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 ca4e87b81..4c11da2e2 100644 > > --- a/tests/test-ovsdb.c > > +++ b/tests/test-ovsdb.c > > @@ -3381,6 +3381,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, simple5 and simple7\n", step++); > > Should they be simple3 and simple4? Yes. It's a copy/paste error from my side. Addressed in v2. > > > + 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 }, > > @@ -3421,6 +3478,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 }, > > }; > Please check out v2 -> https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/ Thanks. > -- > Adrián Moreno > > _______________________________________________ > 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
