diff mbox series

[ovs-dev,v3,2/2] ovsdb-idl: Exclude missing tables and columns in the transaction.

Message ID 20210823225915.3903510-1-numans@ovn.org
State Changes Requested
Headers show
Series ovsdb-idl: Address missing table and column | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Numan Siddique Aug. 23, 2021, 10:59 p.m. UTC
From: Numan Siddique <nusiddiq@redhat.com>

In the cases where the C idl client is compiled with a newer schema
and the ovsdb-server is running with older schema, the IDL clients
can included tables or columns in the transaction which are missing
in the server schema.  ovsdb-server will reject the transaction,
but the IDL client keeps on trying the transaction resulting
in a continuous loop.  This patch fixes this issue.

For the missing tables, the function ovsdb_idl_txn_insert() will now
return NULL.  The client should check for NULL before accessing the
row object.  For the missing columns, the columns are excluded from
the transaction object.

Relevant test cases are added.

Related issue: https://bugzilla.redhat.com/show_bug.cgi?id=1992705
Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
---
 lib/ovsdb-idl.c          | 15 +++++++++++-
 tests/idltest.ovsschema  |  9 +++++++
 tests/idltest2.ovsschema |  7 ++++++
 tests/ovsdb-idl.at       | 26 ++++++++++++++++++++
 tests/test-ovsdb.c       | 53 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 109 insertions(+), 1 deletion(-)

Comments

Ilya Maximets Aug. 24, 2021, 11:31 a.m. UTC | #1
On 8/24/21 12:59 AM, numans@ovn.org wrote:
> From: Numan Siddique <nusiddiq@redhat.com>
> 
> In the cases where the C idl client is compiled with a newer schema
> and the ovsdb-server is running with older schema, the IDL clients
> can included tables or columns in the transaction which are missing
> in the server schema.  ovsdb-server will reject the transaction,
> but the IDL client keeps on trying the transaction resulting
> in a continuous loop.  This patch fixes this issue.
> 
> For the missing tables, the function ovsdb_idl_txn_insert() will now
> return NULL.  The client should check for NULL before accessing the
> row object.  For the missing columns, the columns are excluded from
> the transaction object.
This doesn't look safe, because none of existing clients are checking
the return value of IDL wrapper functions and are using it right away.

Can we just fail the transaction before sending it to the server?
Clients are required to check the transaction status.  Clients will
need to check for existence of the certain columns before adding
them to transactions though with the API from patch #1.

Best regards, Ilya Maximets.
Numan Siddique Aug. 24, 2021, 2:48 p.m. UTC | #2
On Tue, Aug 24, 2021 at 7:31 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> On 8/24/21 12:59 AM, numans@ovn.org wrote:
> > From: Numan Siddique <nusiddiq@redhat.com>
> >
> > In the cases where the C idl client is compiled with a newer schema
> > and the ovsdb-server is running with older schema, the IDL clients
> > can included tables or columns in the transaction which are missing
> > in the server schema.  ovsdb-server will reject the transaction,
> > but the IDL client keeps on trying the transaction resulting
> > in a continuous loop.  This patch fixes this issue.
> >
> > For the missing tables, the function ovsdb_idl_txn_insert() will now
> > return NULL.  The client should check for NULL before accessing the
> > row object.  For the missing columns, the columns are excluded from
> > the transaction object.
> This doesn't look safe, because none of existing clients are checking
> the return value of IDL wrapper functions and are using it right away.
>

Thanks for the review.  I had this concern too.

> Can we just fail the transaction before sending it to the server?
> Clients are required to check the transaction status.  Clients will
> need to check for existence of the certain columns before adding
> them to transactions though with the API from patch #1.

Either we can fail the transaction at the beginning or just not exclude
that table/column when sending the transaction.   Can you please
Let me know what would you prefer ?  I'll

I'd prefer the latter because if the clients can check the existence of
table/columns, then probably they won't even be included in the
transaction in the first place.   Also by checking the status, can the clients
know which table/column caused the failure ?   So probably it seems
better to me to just ignore such tables/columns from the transaction
just like how monitor request ignores such tables/columns.

Is it possible to consider the patch #1 (if it looks fine) please as
we need it urgently
to unblock an upgrade issue we have.  With that, ovn-controller can check
for the existence of a table and exclude it from the transaction.

I'll work on patch #2 meanwhile.

Thanks
Numan



>
> Best regards, Ilya Maximets.
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index b2dfff46c..8e4ce43b7 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -3382,6 +3382,12 @@  ovsdb_idl_txn_write__(const struct ovsdb_idl_row *row_,
     }
 
     class = row->table->class_;
+
+    if (!ovsdb_idl_has_column_in_table(row->table->idl, class->name,
+                                       column->name)) {
+        goto discard_datum;
+    }
+
     column_idx = column - class->columns;
     write_only = row->table->modes[column_idx] == OVSDB_IDL_MONITOR;
 
@@ -3599,12 +3605,19 @@  ovsdb_idl_txn_delete(const struct ovsdb_idl_row *row_)
  * ovsdb-server.
  *
  * Usually this function is used indirectly through one of the "insert"
- * functions generated by ovsdb-idlc. */
+ * functions generated by ovsdb-idlc.
+ *
+ * Returns NULL if the table is not part of the idl schema.
+ * */
 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)
 {
+    if (!ovsdb_idl_has_table(txn->idl, class->name)) {
+        return NULL;
+    }
+
     struct ovsdb_idl_row *row = ovsdb_idl_row_create__(class);
 
     if (uuid) {
diff --git a/tests/idltest.ovsschema b/tests/idltest.ovsschema
index 3ddb612b0..65eadb961 100644
--- a/tests/idltest.ovsschema
+++ b/tests/idltest.ovsschema
@@ -210,6 +210,15 @@ 
       },
       "isRoot": true
     },
+    "simple7" : {
+      "columns" : {
+        "name" : {
+          "type": "string"
+        },
+        "id": {"type": "string"}
+      },
+      "isRoot" : true
+    },
     "singleton" : {
       "columns" : {
         "name" : {
diff --git a/tests/idltest2.ovsschema b/tests/idltest2.ovsschema
index 210e4c389..a8199e56c 100644
--- a/tests/idltest2.ovsschema
+++ b/tests/idltest2.ovsschema
@@ -139,6 +139,13 @@ 
           "type": "string"
         }
       }
+    },
+    "simple7" : {
+      "columns" : {
+        "name" : {
+          "type": "string"
+        }
+      }
     }
   }
 }
diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
index b11fabe70..b3cf31af0 100644
--- a/tests/ovsdb-idl.at
+++ b/tests/ovsdb-idl.at
@@ -999,6 +999,7 @@  test-ovsdb|ovsdb_idl|idltest database lacks simple5 table (database needs upgrad
 test-ovsdb|ovsdb_idl|idltest database lacks simple6 table (database needs upgrade?)
 test-ovsdb|ovsdb_idl|idltest database lacks singleton table (database needs upgrade?)
 test-ovsdb|ovsdb_idl|link1 table in idltest database lacks l2 column (database needs upgrade?)
+test-ovsdb|ovsdb_idl|simple7 table in idltest database lacks id column (database needs upgrade?)
 ])
 
 # Check that ovsdb-idl sent on "monitor" request and that it didn't
@@ -2410,3 +2411,28 @@  column l2 in table link1 is present
 
 OVSDB_SERVER_SHUTDOWN
 AT_CLEANUP
+
+AT_SETUP([idl transaction handling of missing tables and columns - 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/idltest2.ovsschema"])
+AT_CHECK([test-ovsdb '-vPATTERN:console:test-ovsdb|%c|%m' -vjsonrpc -t10 idl-missing-table-column-txn unix:socket],
+         [0], [stdout], [stderr])
+AT_CHECK([sort stdout | uuidfilt], [0],
+    [[000: After inserting simple, simple5 and simple7
+001: table simple7: name=simple7 id= uuid=<0>
+001: table simple: i=0 r=0 b=false s=simple u=<1> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<2>
+002: End test
+]])
+
+OVSDB_SERVER_SHUTDOWN
+AT_CLEANUP
diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
index 462d2e57e..9353a5a43 100644
--- a/tests/test-ovsdb.c
+++ b/tests/test-ovsdb.c
@@ -2140,6 +2140,17 @@  print_idl_row_simple6(const struct idltest_simple6 *s6, int step)
     print_idl_row_updated_simple6(s6, step);
 }
 
+static void
+print_idl_row_simple7(const struct idltest_simple7 *s7, int step)
+{
+    struct ds msg = DS_EMPTY_INITIALIZER;
+    ds_put_format(&msg, "name=%s id=%s", s7->name, s7->id);
+    char *row_msg = format_idl_row(&s7->header_, step, ds_cstr(&msg));
+    print_and_log("%s", row_msg);
+    ds_destroy(&msg);
+    free(row_msg);
+}
+
 static void
 print_idl_row_singleton(const struct idltest_singleton *sng, int step)
 {
@@ -2164,6 +2175,7 @@  print_idl(struct ovsdb_idl *idl, int step)
     const struct idltest_link1 *l1;
     const struct idltest_link2 *l2;
     const struct idltest_singleton *sng;
+    const struct idltest_simple7 *s7;
     int n = 0;
 
     IDLTEST_SIMPLE_FOR_EACH (s, idl) {
@@ -2194,6 +2206,10 @@  print_idl(struct ovsdb_idl *idl, int step)
         print_idl_row_singleton(sng, step);
         n++;
     }
+    IDLTEST_SIMPLE7_FOR_EACH (s7, idl) {
+        print_idl_row_simple7(s7, step);
+        n++;
+    }
     if (!n) {
         print_and_log("%03d: empty", step);
     }
@@ -3339,6 +3355,41 @@  do_idl_table_column_check(struct ovs_cmdl_context *ctx)
     ovsdb_idl_destroy(idl);
 }
 
+static void
+do_idl_missing_table_column_txn(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);
+
+    /* Insert a row in simple2. */
+    myTxn = ovsdb_idl_txn_create(idl);
+    struct idltest_simple *simple_row = idltest_simple_insert(myTxn);
+    idltest_simple_set_s(simple_row, "simple");
+
+    /* Insert a row in simple5. simple5 table doesn't exist. */
+    struct idltest_simple5 *simple5_row = idltest_simple5_insert(myTxn);
+    ovs_assert(!simple5_row);
+
+    struct idltest_simple7 *simple7_row = idltest_simple7_insert(myTxn);
+    idltest_simple7_set_name(simple7_row, "simple7");
+    idltest_simple7_set_id(simple7_row, "simple7_id");
+
+    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++);
+    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 },
@@ -3379,6 +3430,8 @@  static struct ovs_cmdl_command all_commands[] = {
         do_idl_partial_update_set_column, OVS_RO },
     { "idl-table-column-check", NULL, 1, INT_MAX,
         do_idl_table_column_check, OVS_RO },
+    { "idl-missing-table-column-txn", NULL, 1, INT_MAX,
+        do_idl_missing_table_column_txn, OVS_RO },
     { "help", NULL, 0, INT_MAX, do_help, OVS_RO },
     { NULL, NULL, 0, 0, NULL, OVS_RO },
 };