On 5/4/22 18:53, Numan Siddique wrote:
On Tue, Apr 26, 2022 at 7:39 AM Adrian Moreno <amore...@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, num...@ovn.org wrote:
From: Numan Siddique <num...@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 <num...@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-num...@ovn.org/
Thanks.

--
Adrián Moreno

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


--
Adrián Moreno

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to