Message ID | 20210825031318.3163412-1-numans@ovn.org |
---|---|
State | Superseded |
Headers | show |
Series | ovsdb-idl: Address missing table and column issues. | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 8/25/21 5:13 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 by excluding > them for the jsonrpc transaction message. > > This patch chose to exclude the missing tables/columns from the > transaction instead of failing the transaction (TXN_ERROR) for > the following reasons: > > 1. IDL clients will not come to know the exact reason for the > transaction failure. > > 2. IDL client may not know all the transaction objects added in > its loop before calling ovsdb_idl_loop_commit_and_wait(). > > 3. If the client has to figure out the reason by checking the > presence of tables/columns using the APIs added in the > previous commit, it can very well exclude such tables/columns > from the transaction. > > Relevant test cases are added to cover this case. > > Related issue: https://bugzilla.redhat.com/show_bug.cgi?id=1992705 > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > --- > lib/ovsdb-idl.c | 16 ++++++++++++ > tests/idltest.ovsschema | 9 +++++++ > tests/idltest2.ovsschema | 7 ++++++ > tests/ovsdb-idl.at | 37 ++++++++++++++++++++++++++++ > tests/test-ovsdb.c | 53 ++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 122 insertions(+) > > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > index b2dfff46c..404cfc75a 100644 > --- a/lib/ovsdb-idl.c > +++ b/lib/ovsdb-idl.c > @@ -3084,6 +3084,16 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn) > HMAP_FOR_EACH (row, txn_node, &txn->txn_rows) { > const struct ovsdb_idl_table_class *class = row->table->class_; > > + if (!row->table->in_server_schema) { > + /* The table is not present in the server schema. Do not > + * include it in the transaction. */ > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > + VLOG_WARN_RL(&rl, "%s database lacks %s table, excluding from " > + "the txn.", row->table->idl->class_->database, > + row->table->class_->name); > + continue; This check is only for data, but we should, probably, exclude this table from prerequisits too. > + } > + > if (!row->new_datum) { > if (class->is_root) { > struct json *op = json_object_create(); > @@ -3382,6 +3392,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)) { Should we warn a user about missing columns? And again, do we need to add a check to the ovsdb_idl_txn_verify() ? Would be great to have a _verify() function in the test case too. > + goto discard_datum; > + } > + > column_idx = column - class->columns; > write_only = row->table->modes[column_idx] == OVSDB_IDL_MONITOR; > > 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..26212f265 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,39 @@ 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 > +]]) > + > +AT_CHECK([grep ovsdb_idl stderr | sort], [0], [dnl > +test-ovsdb|ovsdb_idl|idltest database lacks indexed table (database needs upgrade?) > +test-ovsdb|ovsdb_idl|idltest database lacks link2 table (database needs upgrade?) > +test-ovsdb|ovsdb_idl|idltest database lacks simple5 table (database needs upgrade?) > +test-ovsdb|ovsdb_idl|idltest database lacks simple5 table, excluding from the txn. > +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?) > +]) > + > +OVSDB_SERVER_SHUTDOWN > +AT_CLEANUP > diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c > index 462d2e57e..7f4dd32c5 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); > + idltest_simple5_set_name(simple5_row, "simple"); > + > + 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 }, > }; >
On Thu, Aug 26, 2021 at 6:23 AM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 8/25/21 5:13 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 by excluding > > them for the jsonrpc transaction message. > > > > This patch chose to exclude the missing tables/columns from the > > transaction instead of failing the transaction (TXN_ERROR) for > > the following reasons: > > > > 1. IDL clients will not come to know the exact reason for the > > transaction failure. > > > > 2. IDL client may not know all the transaction objects added in > > its loop before calling ovsdb_idl_loop_commit_and_wait(). > > > > 3. If the client has to figure out the reason by checking the > > presence of tables/columns using the APIs added in the > > previous commit, it can very well exclude such tables/columns > > from the transaction. > > > > Relevant test cases are added to cover this case. > > > > Related issue: https://bugzilla.redhat.com/show_bug.cgi?id=1992705 > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > > --- > > lib/ovsdb-idl.c | 16 ++++++++++++ > > tests/idltest.ovsschema | 9 +++++++ > > tests/idltest2.ovsschema | 7 ++++++ > > tests/ovsdb-idl.at | 37 ++++++++++++++++++++++++++++ > > tests/test-ovsdb.c | 53 ++++++++++++++++++++++++++++++++++++++++ > > 5 files changed, 122 insertions(+) > > > > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > > index b2dfff46c..404cfc75a 100644 > > --- a/lib/ovsdb-idl.c > > +++ b/lib/ovsdb-idl.c > > @@ -3084,6 +3084,16 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn) > > HMAP_FOR_EACH (row, txn_node, &txn->txn_rows) { > > const struct ovsdb_idl_table_class *class = row->table->class_; > > > > + if (!row->table->in_server_schema) { > > + /* The table is not present in the server schema. Do not > > + * include it in the transaction. */ > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > > + VLOG_WARN_RL(&rl, "%s database lacks %s table, excluding from " > > + "the txn.", row->table->idl->class_->database, > > + row->table->class_->name); > > + continue; > > This check is only for data, but we should, probably, exclude this > table from prerequisits too. In my opinion it will be complicated and we might miss out on conditions like this if we add the inserted row to the txn->txn_rows if the row is for the missing table. So I thought of creating a new hmap in the txn struct called 'excluded_rows'. Such rows will be added in this hmap and will be cleaned up when the txn object is destroyed. This is what is in my mind - https://github.com/numansiddique/ovs/commit/3ac1db8eb4f14f9648ab19e9a57c3948364de613 I need to spend some more time on this to address this issue completely. So I've dropped patch 2 from v6 for now. I'll submit this patch once I test thoroughly. Hope this is fine and patch 1 can be considered if it is good in a shape (to unblock OVN :)). If you've any comments on the above commit link I shared please do share. Thanks for the reviews Numan > > > + } > > + > > if (!row->new_datum) { > > if (class->is_root) { > > struct json *op = json_object_create(); > > @@ -3382,6 +3392,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)) { > > Should we warn a user about missing columns? And again, do we need > to add a check to the ovsdb_idl_txn_verify() ? > > Would be great to have a _verify() function in the test case too. > > > + goto discard_datum; > > + } > > + > > column_idx = column - class->columns; > > write_only = row->table->modes[column_idx] == OVSDB_IDL_MONITOR; > > > > 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..26212f265 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,39 @@ 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 > > +]]) > > + > > +AT_CHECK([grep ovsdb_idl stderr | sort], [0], [dnl > > +test-ovsdb|ovsdb_idl|idltest database lacks indexed table (database needs upgrade?) > > +test-ovsdb|ovsdb_idl|idltest database lacks link2 table (database needs upgrade?) > > +test-ovsdb|ovsdb_idl|idltest database lacks simple5 table (database needs upgrade?) > > +test-ovsdb|ovsdb_idl|idltest database lacks simple5 table, excluding from the txn. > > +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?) > > +]) > > + > > +OVSDB_SERVER_SHUTDOWN > > +AT_CLEANUP > > diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c > > index 462d2e57e..7f4dd32c5 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); > > + idltest_simple5_set_name(simple5_row, "simple"); > > + > > + 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 }, > > }; > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index b2dfff46c..404cfc75a 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -3084,6 +3084,16 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn) HMAP_FOR_EACH (row, txn_node, &txn->txn_rows) { const struct ovsdb_idl_table_class *class = row->table->class_; + if (!row->table->in_server_schema) { + /* The table is not present in the server schema. Do not + * include it in the transaction. */ + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); + VLOG_WARN_RL(&rl, "%s database lacks %s table, excluding from " + "the txn.", row->table->idl->class_->database, + row->table->class_->name); + continue; + } + if (!row->new_datum) { if (class->is_root) { struct json *op = json_object_create(); @@ -3382,6 +3392,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; 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..26212f265 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,39 @@ 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 +]]) + +AT_CHECK([grep ovsdb_idl stderr | sort], [0], [dnl +test-ovsdb|ovsdb_idl|idltest database lacks indexed table (database needs upgrade?) +test-ovsdb|ovsdb_idl|idltest database lacks link2 table (database needs upgrade?) +test-ovsdb|ovsdb_idl|idltest database lacks simple5 table (database needs upgrade?) +test-ovsdb|ovsdb_idl|idltest database lacks simple5 table, excluding from the txn. +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?) +]) + +OVSDB_SERVER_SHUTDOWN +AT_CLEANUP diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c index 462d2e57e..7f4dd32c5 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); + idltest_simple5_set_name(simple5_row, "simple"); + + 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 }, };