| Message ID | 20220308005648.2178146-1-numans@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] ovsdb idl: Add the support to specify the uuid for row insert. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/intel-ovs-compilation | success | test: success |
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
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? On 3/8/22 01:56, numans@ovn.org wrote: > From: Numan Siddique <numans@ovn.org> > > 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 <numans@ovn.org> > --- > 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? > + } 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? > + 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 }, > };
On Tue, Apr 26, 2022 at 7:39 AM Adrian Moreno <amorenoz@redhat.com> 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, numans@ovn.org wrote: > > From: Numan Siddique <numans@ovn.org> > > > > 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 <numans@ovn.org> > > --- > > 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/20220504165147.1796350-1-numans@ovn.org/ Thanks. > -- > Adrián Moreno > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 5/4/22 18:53, Numan Siddique wrote: > On Tue, Apr 26, 2022 at 7:39 AM Adrian Moreno <amorenoz@redhat.com> 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 ... > Sounds reasonable. Thanks! > What do you think ? I've incorporated it in v2. Please check it out. > > >> >> On 3/8/22 01:56, numans@ovn.org wrote: >>> From: Numan Siddique <numans@ovn.org> >>> >>> 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 <numans@ovn.org> >>> --- >>> 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/20220504165147.1796350-1-numans@ovn.org/ > Thanks. > >> -- >> Adrián Moreno >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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)))); + } 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++); + 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 }, };