On Tue, Oct 25, 2022 at 5:17 PM Ilya Maximets <[email protected]> wrote:
>
> On 9/26/22 19:52, [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]>
> > ---
> > v6 -> v7
> > ---
> > * Rebased to resolve conflicts.
> >
> > v5 -> v6
> > ---
> > * Rebased to resolve conflicts.
> >
> > 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 | 2 +
> > 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(+), 37 deletions(-)
> >
> > 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.
>
Thanks for the review. Sorry for the late reply.
> How hard it is to actually add the support?
> it would be great to not increase the difference between
> implementations. Sometimes we do not add features to
> python idl, but that mostly happens when they depend on
> some other missing features. UUID persistense seems to
> not have any extra dependencies.
>
It turned out to be straightforward. Addressed in v8.
> > 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 965b0f913..3edb2bc78 100644
> > --- a/tests/test-ovsdb.c
> > +++ b/tests/test-ovsdb.c
> > @@ -3451,6 +3451,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++);
> > +}
>
> This test case is a bit worrying, because whenever we'll have
> a python implementation, the test case will have to be
> duplicated or fully re-written. Can we add a new command
> to the 'transact' test instead, e.g. 'insert-with-uuid' and
> write a generic test case for it?
Addressed this in v8.
It was not possible to add a new comment in the transact tests (I
suppose you mean tests in ovsdb-transaction.at)
as the tests there don't create any IDL objects.
But added subcommit - "insert_with_uuid" in do_idl set command.
Please take a look at v8 -
https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/
@Terry Wilson Can you please take a look at v8 of this patch as it
also modifies the python idl.
I want to make sure this change doesn't break ovsdbapp.
Thanks
Numan
>
> > +
> > 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 },
> > @@ -3491,6 +3548,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 },
> > };
>
> _______________________________________________
> 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
