| Message ID | 20210825031257.3163318-1-numans@ovn.org |
|---|---|
| State | Changes Requested |
| 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:12 AM, numans@ovn.org wrote: > From: Numan Siddique <nusiddiq@redhat.com> > > This patch adds 2 new APIs in the ovsdb-idl client library > - ovsdb_idl_has_table() and ovsdb_idl_has_column_in_table() to > query if a table and a column is present in the IDL or not. This > is required for scenarios where the server schema is old and > missing a table or column and the client (built with a new schema > version) does a transaction with the missing table or column. This > results in a continuous loop of transaction failures. > > OVN would require the API - ovsdb_idl_has_table() to address this issue > when an old ovsdb-server is used (OVS 2.11) which has the 'datapath' > table missing. A recent commit in OVN creates a 'datapath' row > in the local ovsdb-server. ovsdb_idl_has_column_in_table() would be > useful to have. > > Related issue: https://bugzilla.redhat.com/show_bug.cgi?id=1992705 > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > --- > lib/ovsdb-idl-provider.h | 4 +++ > lib/ovsdb-idl.c | 36 ++++++++++++++++++++ > lib/ovsdb-idl.h | 3 ++ > tests/ovsdb-idl.at | 38 +++++++++++++++++++++ > tests/test-ovsdb.c | 73 ++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 154 insertions(+) > > diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h > index 0f38f9b34..0f122e23c 100644 > --- a/lib/ovsdb-idl-provider.h > +++ b/lib/ovsdb-idl-provider.h > @@ -24,6 +24,7 @@ > #include "ovsdb-set-op.h" > #include "ovsdb-types.h" > #include "openvswitch/shash.h" > +#include "sset.h" > #include "uuid.h" > > #ifdef __cplusplus > @@ -117,9 +118,12 @@ struct ovsdb_idl_table { > bool need_table; /* Monitor table even if no columns are selected > * for replication. */ > struct shash columns; /* Contains "const struct ovsdb_idl_column *"s. */ > + struct sset schema_columns; /* Column names from schema. */ Why did you decide to go with sset instead of 'in_server_schema' boolean for the struct ovsdb_idl_column? > struct hmap rows; /* Contains "struct ovsdb_idl_row"s. */ > struct ovsdb_idl *idl; /* Containing IDL instance. */ > unsigned int change_seqno[OVSDB_IDL_CHANGE_MAX]; > + bool in_server_schema; /* Indicates if this table is in the server schema > + * or not. */ > struct ovs_list indexes; /* Contains "struct ovsdb_idl_index"s */ > struct ovs_list track_list; /* Tracked rows (ovsdb_idl_row.track_node). */ > }; > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > index 2198c69c6..b2dfff46c 100644 > --- a/lib/ovsdb-idl.c > +++ b/lib/ovsdb-idl.c > @@ -287,6 +287,8 @@ ovsdb_idl_create_unconnected(const struct ovsdb_idl_class *class, > = table->change_seqno[OVSDB_IDL_CHANGE_MODIFY] > = table->change_seqno[OVSDB_IDL_CHANGE_DELETE] = 0; > table->idl = idl; > + table->in_server_schema = true; /* Assume it's in server schema. */ Hmm. Why is that? We do not make assumptions about columns, we should, probably, not do that for tables too. What do you think? > + sset_init(&table->schema_columns); > } > > return idl; > @@ -337,6 +339,7 @@ ovsdb_idl_destroy(struct ovsdb_idl *idl) > struct ovsdb_idl_table *table = &idl->tables[i]; > ovsdb_idl_destroy_indexes(table); > shash_destroy(&table->columns); > + sset_destroy(&table->schema_columns); > hmap_destroy(&table->rows); > free(table->modes); > } > @@ -718,6 +721,7 @@ ovsdb_idl_compose_monitor_request(const struct json *schema_json, void *idl_) > > struct json *columns > = table->need_table ? json_array_create_empty() : NULL; > + sset_clear(&table->schema_columns); > for (size_t j = 0; j < tc->n_columns; j++) { > const struct ovsdb_idl_column *column = &tc->columns[j]; > bool idl_has_column = (table_schema && > @@ -741,6 +745,7 @@ ovsdb_idl_compose_monitor_request(const struct json *schema_json, void *idl_) > } > json_array_add(columns, json_string_create(column->name)); > } > + sset_add(&table->schema_columns, column->name); > } > > if (columns) { > @@ -749,7 +754,12 @@ ovsdb_idl_compose_monitor_request(const struct json *schema_json, void *idl_) > "(database needs upgrade?)", > idl->class_->database, table->class_->name); > json_destroy(columns); > + /* Set 'table->in_server_schema' to false so that this can be > + * excluded from transactions. */ This comment seems redundant, since the functionality is in a different patch. > + table->in_server_schema = false; > continue; > + } else if (schema && table_schema) { > + table->in_server_schema = true; > } > > monitor_request = json_object_create(); > @@ -4256,3 +4266,29 @@ ovsdb_idl_loop_commit_and_wait(struct ovsdb_idl_loop *loop) > > return retval; > } > + > +static struct ovsdb_idl_table* > +ovsdb_idl_get_table(struct ovsdb_idl *idl, const char *table_name) > +{ > + struct ovsdb_idl_table *table = shash_find_data(&idl->table_by_name, > + table_name); > + return table && table->in_server_schema ? table : NULL; It's better to parenthesize the condition, the priority of operations is not obvious. > +} > + > +bool > +ovsdb_idl_has_table(struct ovsdb_idl *idl, const char *table_name) > +{ > + return ovsdb_idl_get_table(idl, table_name) ? true: false; Missing space before the ':'. > +} > + > +bool > +ovsdb_idl_has_column_in_table(struct ovsdb_idl *idl, const char *table_name, > + const char *column_name) > +{ > + struct ovsdb_idl_table *table = ovsdb_idl_get_table(idl, table_name); Empty line here would be nice to have. > + if (table && sset_find(&table->schema_columns, column_name)) { > + return true; > + } > + > + return false; > +} > diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h > index d93483245..48425b39a 100644 > --- a/lib/ovsdb-idl.h > +++ b/lib/ovsdb-idl.h > @@ -474,6 +474,9 @@ void ovsdb_idl_cursor_next_eq(struct ovsdb_idl_cursor *); > > struct ovsdb_idl_row *ovsdb_idl_cursor_data(struct ovsdb_idl_cursor *); > > +bool ovsdb_idl_has_table(struct ovsdb_idl *, const char *table_name); > +bool ovsdb_idl_has_column_in_table(struct ovsdb_idl *, const char *table_name, > + const char *column_name); This is a section of a header related to indexes and iteration. These functions should be defined somewhere close to ovsdb_idl_check_consistency() or ovsdb_idl_get_class(). > #ifdef __cplusplus > } > #endif > diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at > index 1386f1377..b11fabe70 100644 > --- a/tests/ovsdb-idl.at > +++ b/tests/ovsdb-idl.at > @@ -2372,3 +2372,41 @@ OVSDB_CHECK_CLUSTER_IDL([simple idl, initially empty, force reconnect], > [], > [], > reconnect.*waiting .* seconds before reconnect) > + > +AT_SETUP([idl table and column presence check]) > +AT_KEYWORDS([ovsdb server idl table column check]) > +AT_CHECK([ovsdb_start_idltest "" "$abs_srcdir/idltest2.ovsschema"]) > + > +ovsdb-tool create db2 $abs_srcdir/idltest.ovsschema > +ovsdb-server -vconsole:warn --log-file=ovsdb-server2.log --detach --no-chdir --pidfile=ovsdb-server2.pid --remote=punix:socket2 db2 Above two commands should be executed with AT_CHECK. It also would be nice to wrap the long line with 'dnl' inside AT_CHECK. > +on_exit 'kill `cat ovsdb-server2.pid`' > + > +# In this test, test-ovsdb first connects to the server with schema > +# idltest2.ovsschema and outputs the presence of tables and columns. > +# And then it connectes to the server with the schema idltest.ovsschema > +# and does the same. > +AT_CHECK([test-ovsdb -vconsole:off -t10 idl-table-column-check unix:socket unix:socket2 \ It's better to use 'dnl' for breaking lines, so it can be properly printed in the testsuite log. > +simple link1 link2 simple5 foo simple5:irefmap simple5:foo link1:l2], > +[0], [dnl > +table simple is present > +table link1 is present > +table link2 is not present > +table simple5 is not present > +table foo is not present > +table simple5 is not present > +table simple5 is not present > +column l2 in table link1 is not present > +--- remote 1 done --- > +table simple is present > +table link1 is present > +table link2 is present > +table simple5 is present > +table foo is not present > +column irefmap in table simple5 is present > +column foo in table simple5 is not present > +column l2 in table link1 is present > +--- remote 2 done --- > +]) > + > +OVSDB_SERVER_SHUTDOWN > +AT_CLEANUP > diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c > index daa55dab7..462d2e57e 100644 > --- a/tests/test-ovsdb.c > +++ b/tests/test-ovsdb.c > @@ -3268,6 +3268,77 @@ do_idl_compound_index(struct ovs_cmdl_context *ctx) > printf("%03d: done\n", step); > } > > +static void > +do_idl_table_column_check(struct ovs_cmdl_context *ctx) > +{ > + struct jsonrpc *rpc; > + struct ovsdb_idl *idl; > + unsigned int seqno = 0; > + int error; > + int i; > + > + if (ctx->argc < 3) { > + exit(1); > + } > + > + idl = ovsdb_idl_create(ctx->argv[1], &idltest_idl_class, true, true); > + ovsdb_idl_set_leader_only(idl, false); > + struct stream *stream; > + > + error = stream_open_block(jsonrpc_stream_open(ctx->argv[1], &stream, > + DSCP_DEFAULT), -1, &stream); > + if (error) { > + ovs_fatal(error, "failed to connect to \"%s\"", ctx->argv[1]); > + } > + rpc = jsonrpc_open(stream); > + > + for (int r = 1; r <= 2; r++) { > + ovsdb_idl_set_remote(idl, ctx->argv[r], true); > + ovsdb_idl_force_reconnect(idl); > + > + /* Wait for update. */ > + for (;;) { > + ovsdb_idl_run(idl); > + ovsdb_idl_check_consistency(idl); > + if (ovsdb_idl_get_seqno(idl) != seqno) { > + break; > + } > + jsonrpc_run(rpc); > + > + ovsdb_idl_wait(idl); > + jsonrpc_wait(rpc); > + poll_block(); > + } > + > + seqno = ovsdb_idl_get_seqno(idl); > + > + for (i = 3; i < ctx->argc; i++) { > + char *save_ptr2 = NULL; > + char *arg = xstrdup(ctx->argv[i]); > + char *table_name = strtok_r(arg, ":", &save_ptr2); > + char *column_name = strtok_r(NULL, ":", &save_ptr2); > + > + bool table_present = ovsdb_idl_has_table(idl, table_name); > + if (!table_present || !column_name) { > + printf("table %s %s present\n", table_name, > + table_present ? "is" : "is not"); Please shif this line to be on the same level with the text inside the '()' above. > + } > + if (table_present && column_name) { > + printf("column %s in table %s %s present\n", column_name, > + table_name, > + ovsdb_idl_has_column_in_table(idl, table_name, > + column_name) ? > + "is" : "is not"); Maybe: bool in = ovsdb_idl_has_column_in_table(idl, table_name, column_name); printf("column %s in table %s %s present\n", column_name, table_name, in ? "is" : "is not"); ? > + } > + free(arg); > + } > + printf("--- remote %d done ---\n", r); > + } > + > + jsonrpc_close(rpc); > + ovsdb_idl_destroy(idl); > +} > + > 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 }, > @@ -3306,6 +3377,8 @@ static struct ovs_cmdl_command all_commands[] = { > do_idl_partial_update_map_column, OVS_RO }, > { "idl-partial-update-set-column", NULL, 1, INT_MAX, > do_idl_partial_update_set_column, OVS_RO }, > + { "idl-table-column-check", NULL, 1, INT_MAX, It should be 2, I think, instead of 1, right? And you may remove the check for number of arguments from the function. > + do_idl_table_column_check, OVS_RO }, > { "help", NULL, 0, INT_MAX, do_help, OVS_RO }, > { NULL, NULL, 0, 0, NULL, OVS_RO }, > }; >
' On Wed, Aug 25, 2021 at 6:06 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 8/25/21 5:12 AM, numans@ovn.org wrote: > > From: Numan Siddique <nusiddiq@redhat.com> > > > > This patch adds 2 new APIs in the ovsdb-idl client library > > - ovsdb_idl_has_table() and ovsdb_idl_has_column_in_table() to > > query if a table and a column is present in the IDL or not. This > > is required for scenarios where the server schema is old and > > missing a table or column and the client (built with a new schema > > version) does a transaction with the missing table or column. This > > results in a continuous loop of transaction failures. > > > > OVN would require the API - ovsdb_idl_has_table() to address this issue > > when an old ovsdb-server is used (OVS 2.11) which has the 'datapath' > > table missing. A recent commit in OVN creates a 'datapath' row > > in the local ovsdb-server. ovsdb_idl_has_column_in_table() would be > > useful to have. > > > > Related issue: https://bugzilla.redhat.com/show_bug.cgi?id=1992705 > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > > --- > > lib/ovsdb-idl-provider.h | 4 +++ > > lib/ovsdb-idl.c | 36 ++++++++++++++++++++ > > lib/ovsdb-idl.h | 3 ++ > > tests/ovsdb-idl.at | 38 +++++++++++++++++++++ > > tests/test-ovsdb.c | 73 ++++++++++++++++++++++++++++++++++++++++ > > 5 files changed, 154 insertions(+) > > > > diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h > > index 0f38f9b34..0f122e23c 100644 > > --- a/lib/ovsdb-idl-provider.h > > +++ b/lib/ovsdb-idl-provider.h > > @@ -24,6 +24,7 @@ > > #include "ovsdb-set-op.h" > > #include "ovsdb-types.h" > > #include "openvswitch/shash.h" > > +#include "sset.h" > > #include "uuid.h" > > > > #ifdef __cplusplus > > @@ -117,9 +118,12 @@ struct ovsdb_idl_table { > > bool need_table; /* Monitor table even if no columns are selected > > * for replication. */ > > struct shash columns; /* Contains "const struct ovsdb_idl_column *"s. */ > > + struct sset schema_columns; /* Column names from schema. */ > > Why did you decide to go with sset instead of 'in_server_schema' > boolean for the struct ovsdb_idl_column? Two reasons: 1. The ovsdb_idl_column instances for each table columns are generated automatically by ovsdb/ovsdb-idlc.in 2. 'struct ovsdb_idl_column *columns' is const in 'struct ovsdb_idl_table_class' https://github.com/openvswitch/ovs/blob/master/lib/ovsdb-idl.c#L722 > > > struct hmap rows; /* Contains "struct ovsdb_idl_row"s. */ > > struct ovsdb_idl *idl; /* Containing IDL instance. */ > > unsigned int change_seqno[OVSDB_IDL_CHANGE_MAX]; > > + bool in_server_schema; /* Indicates if this table is in the server schema > > + * or not. */ > > struct ovs_list indexes; /* Contains "struct ovsdb_idl_index"s */ > > struct ovs_list track_list; /* Tracked rows (ovsdb_idl_row.track_node). */ > > }; > > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > > index 2198c69c6..b2dfff46c 100644 > > --- a/lib/ovsdb-idl.c > > +++ b/lib/ovsdb-idl.c > > @@ -287,6 +287,8 @@ ovsdb_idl_create_unconnected(const struct ovsdb_idl_class *class, > > = table->change_seqno[OVSDB_IDL_CHANGE_MODIFY] > > = table->change_seqno[OVSDB_IDL_CHANGE_DELETE] = 0; > > table->idl = idl; > > + table->in_server_schema = true; /* Assume it's in server schema. */ > > Hmm. Why is that? We do not make assumptions about columns, > we should, probably, not do that for tables too. What do > you think? I was thinking of the scenario where user calls ovsdb_has_table() at the very beginning. I'll remove it in v5. > > > + sset_init(&table->schema_columns); > > } > > > > return idl; > > @@ -337,6 +339,7 @@ ovsdb_idl_destroy(struct ovsdb_idl *idl) > > struct ovsdb_idl_table *table = &idl->tables[i]; > > ovsdb_idl_destroy_indexes(table); > > shash_destroy(&table->columns); > > + sset_destroy(&table->schema_columns); > > hmap_destroy(&table->rows); > > free(table->modes); > > } > > @@ -718,6 +721,7 @@ ovsdb_idl_compose_monitor_request(const struct json *schema_json, void *idl_) > > > > struct json *columns > > = table->need_table ? json_array_create_empty() : NULL; > > + sset_clear(&table->schema_columns); > > for (size_t j = 0; j < tc->n_columns; j++) { > > const struct ovsdb_idl_column *column = &tc->columns[j]; > > bool idl_has_column = (table_schema && > > @@ -741,6 +745,7 @@ ovsdb_idl_compose_monitor_request(const struct json *schema_json, void *idl_) > > } > > json_array_add(columns, json_string_create(column->name)); > > } > > + sset_add(&table->schema_columns, column->name); > > } > > > > if (columns) { > > @@ -749,7 +754,12 @@ ovsdb_idl_compose_monitor_request(const struct json *schema_json, void *idl_) > > "(database needs upgrade?)", > > idl->class_->database, table->class_->name); > > json_destroy(columns); > > + /* Set 'table->in_server_schema' to false so that this can be > > + * excluded from transactions. */ > > This comment seems redundant, since the functionality is in > a different patch. Ack. > > > + table->in_server_schema = false; > > continue; > > + } else if (schema && table_schema) { > > + table->in_server_schema = true; > > } > > > > monitor_request = json_object_create(); > > @@ -4256,3 +4266,29 @@ ovsdb_idl_loop_commit_and_wait(struct ovsdb_idl_loop *loop) > > > > return retval; > > } > > + > > +static struct ovsdb_idl_table* > > +ovsdb_idl_get_table(struct ovsdb_idl *idl, const char *table_name) > > +{ > > + struct ovsdb_idl_table *table = shash_find_data(&idl->table_by_name, > > + table_name); > > + return table && table->in_server_schema ? table : NULL; > > It's better to parenthesize the condition, the priority of > operations is not obvious. Ack. > > > +} > > + > > +bool > > +ovsdb_idl_has_table(struct ovsdb_idl *idl, const char *table_name) > > +{ > > + return ovsdb_idl_get_table(idl, table_name) ? true: false; > > Missing space before the ':'. Ack. > > > +} > > + > > +bool > > +ovsdb_idl_has_column_in_table(struct ovsdb_idl *idl, const char *table_name, > > + const char *column_name) > > +{ > > + struct ovsdb_idl_table *table = ovsdb_idl_get_table(idl, table_name); > > Empty line here would be nice to have. Ack. > > > + if (table && sset_find(&table->schema_columns, column_name)) { > > + return true; > > + } > > + > > + return false; > > +} > > diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h > > index d93483245..48425b39a 100644 > > --- a/lib/ovsdb-idl.h > > +++ b/lib/ovsdb-idl.h > > @@ -474,6 +474,9 @@ void ovsdb_idl_cursor_next_eq(struct ovsdb_idl_cursor *); > > > > struct ovsdb_idl_row *ovsdb_idl_cursor_data(struct ovsdb_idl_cursor *); > > > > +bool ovsdb_idl_has_table(struct ovsdb_idl *, const char *table_name); > > +bool ovsdb_idl_has_column_in_table(struct ovsdb_idl *, const char *table_name, > > + const char *column_name); > > This is a section of a header related to indexes and iteration. > These functions should be defined somewhere close to > ovsdb_idl_check_consistency() or ovsdb_idl_get_class(). Ack. > > > #ifdef __cplusplus > > } > > #endif > > diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at > > index 1386f1377..b11fabe70 100644 > > --- a/tests/ovsdb-idl.at > > +++ b/tests/ovsdb-idl.at > > @@ -2372,3 +2372,41 @@ OVSDB_CHECK_CLUSTER_IDL([simple idl, initially empty, force reconnect], > > [], > > [], > > reconnect.*waiting .* seconds before reconnect) > > + > > +AT_SETUP([idl table and column presence check]) > > +AT_KEYWORDS([ovsdb server idl table column check]) > > +AT_CHECK([ovsdb_start_idltest "" "$abs_srcdir/idltest2.ovsschema"]) > > + > > +ovsdb-tool create db2 $abs_srcdir/idltest.ovsschema > > +ovsdb-server -vconsole:warn --log-file=ovsdb-server2.log --detach --no-chdir --pidfile=ovsdb-server2.pid --remote=punix:socket2 db2 > > Above two commands should be executed with AT_CHECK. > It also would be nice to wrap the long line with 'dnl' inside AT_CHECK. Ack. > > > +on_exit 'kill `cat ovsdb-server2.pid`' > > + > > +# In this test, test-ovsdb first connects to the server with schema > > +# idltest2.ovsschema and outputs the presence of tables and columns. > > +# And then it connectes to the server with the schema idltest.ovsschema > > +# and does the same. > > +AT_CHECK([test-ovsdb -vconsole:off -t10 idl-table-column-check unix:socket unix:socket2 \ > > It's better to use 'dnl' for breaking lines, so it can be properly > printed in the testsuite log. Ack. > > > +simple link1 link2 simple5 foo simple5:irefmap simple5:foo link1:l2], > > +[0], [dnl > > +table simple is present > > +table link1 is present > > +table link2 is not present > > +table simple5 is not present > > +table foo is not present > > +table simple5 is not present > > +table simple5 is not present > > +column l2 in table link1 is not present > > +--- remote 1 done --- > > +table simple is present > > +table link1 is present > > +table link2 is present > > +table simple5 is present > > +table foo is not present > > +column irefmap in table simple5 is present > > +column foo in table simple5 is not present > > +column l2 in table link1 is present > > +--- remote 2 done --- > > +]) > > + > > +OVSDB_SERVER_SHUTDOWN > > +AT_CLEANUP > > diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c > > index daa55dab7..462d2e57e 100644 > > --- a/tests/test-ovsdb.c > > +++ b/tests/test-ovsdb.c > > @@ -3268,6 +3268,77 @@ do_idl_compound_index(struct ovs_cmdl_context *ctx) > > printf("%03d: done\n", step); > > } > > > > +static void > > +do_idl_table_column_check(struct ovs_cmdl_context *ctx) > > +{ > > + struct jsonrpc *rpc; > > + struct ovsdb_idl *idl; > > + unsigned int seqno = 0; > > + int error; > > + int i; > > + > > + if (ctx->argc < 3) { > > + exit(1); > > + } > > + > > + idl = ovsdb_idl_create(ctx->argv[1], &idltest_idl_class, true, true); > > + ovsdb_idl_set_leader_only(idl, false); > > + struct stream *stream; > > + > > + error = stream_open_block(jsonrpc_stream_open(ctx->argv[1], &stream, > > + DSCP_DEFAULT), -1, &stream); > > + if (error) { > > + ovs_fatal(error, "failed to connect to \"%s\"", ctx->argv[1]); > > + } > > + rpc = jsonrpc_open(stream); > > + > > + for (int r = 1; r <= 2; r++) { > > + ovsdb_idl_set_remote(idl, ctx->argv[r], true); > > + ovsdb_idl_force_reconnect(idl); > > + > > + /* Wait for update. */ > > + for (;;) { > > + ovsdb_idl_run(idl); > > + ovsdb_idl_check_consistency(idl); > > + if (ovsdb_idl_get_seqno(idl) != seqno) { > > + break; > > + } > > + jsonrpc_run(rpc); > > + > > + ovsdb_idl_wait(idl); > > + jsonrpc_wait(rpc); > > + poll_block(); > > + } > > + > > + seqno = ovsdb_idl_get_seqno(idl); > > + > > + for (i = 3; i < ctx->argc; i++) { > > + char *save_ptr2 = NULL; > > + char *arg = xstrdup(ctx->argv[i]); > > + char *table_name = strtok_r(arg, ":", &save_ptr2); > > + char *column_name = strtok_r(NULL, ":", &save_ptr2); > > + > > + bool table_present = ovsdb_idl_has_table(idl, table_name); > > + if (!table_present || !column_name) { > > + printf("table %s %s present\n", table_name, > > + table_present ? "is" : "is not"); > > Please shif this line to be on the same level with the text inside > the '()' above. Ack. > > > + } > > + if (table_present && column_name) { > > + printf("column %s in table %s %s present\n", column_name, > > + table_name, > > + ovsdb_idl_has_column_in_table(idl, table_name, > > + column_name) ? > > + "is" : "is not"); > > Maybe: > > bool in = ovsdb_idl_has_column_in_table(idl, table_name, > column_name); > printf("column %s in table %s %s present\n", > column_name, table_name, in ? "is" : "is not"); > ? > Ack. > > + } > > + free(arg); > > + } > > + printf("--- remote %d done ---\n", r); > > + } > > + > > + jsonrpc_close(rpc); > > + ovsdb_idl_destroy(idl); > > +} > > + > > 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 }, > > @@ -3306,6 +3377,8 @@ static struct ovs_cmdl_command all_commands[] = { > > do_idl_partial_update_map_column, OVS_RO }, > > { "idl-partial-update-set-column", NULL, 1, INT_MAX, > > do_idl_partial_update_set_column, OVS_RO }, > > + { "idl-table-column-check", NULL, 1, INT_MAX, > > It should be 2, I think, instead of 1, right? And you may remove > the check for number of arguments from the function. Correct. I'll fix in v5. Thanks for the review. Numan > > > + do_idl_table_column_check, 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 >
On 8/26/21 1:44 AM, Numan Siddique wrote: > ' > > On Wed, Aug 25, 2021 at 6:06 PM Ilya Maximets <i.maximets@ovn.org> wrote: >> >> On 8/25/21 5:12 AM, numans@ovn.org wrote: >>> From: Numan Siddique <nusiddiq@redhat.com> >>> >>> This patch adds 2 new APIs in the ovsdb-idl client library >>> - ovsdb_idl_has_table() and ovsdb_idl_has_column_in_table() to >>> query if a table and a column is present in the IDL or not. This >>> is required for scenarios where the server schema is old and >>> missing a table or column and the client (built with a new schema >>> version) does a transaction with the missing table or column. This >>> results in a continuous loop of transaction failures. >>> >>> OVN would require the API - ovsdb_idl_has_table() to address this issue >>> when an old ovsdb-server is used (OVS 2.11) which has the 'datapath' >>> table missing. A recent commit in OVN creates a 'datapath' row >>> in the local ovsdb-server. ovsdb_idl_has_column_in_table() would be >>> useful to have. >>> >>> Related issue: https://bugzilla.redhat.com/show_bug.cgi?id=1992705 >>> Signed-off-by: Numan Siddique <nusiddiq@redhat.com> >>> --- >>> lib/ovsdb-idl-provider.h | 4 +++ >>> lib/ovsdb-idl.c | 36 ++++++++++++++++++++ >>> lib/ovsdb-idl.h | 3 ++ >>> tests/ovsdb-idl.at | 38 +++++++++++++++++++++ >>> tests/test-ovsdb.c | 73 ++++++++++++++++++++++++++++++++++++++++ >>> 5 files changed, 154 insertions(+) >>> >>> diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h >>> index 0f38f9b34..0f122e23c 100644 >>> --- a/lib/ovsdb-idl-provider.h >>> +++ b/lib/ovsdb-idl-provider.h >>> @@ -24,6 +24,7 @@ >>> #include "ovsdb-set-op.h" >>> #include "ovsdb-types.h" >>> #include "openvswitch/shash.h" >>> +#include "sset.h" >>> #include "uuid.h" >>> >>> #ifdef __cplusplus >>> @@ -117,9 +118,12 @@ struct ovsdb_idl_table { >>> bool need_table; /* Monitor table even if no columns are selected >>> * for replication. */ >>> struct shash columns; /* Contains "const struct ovsdb_idl_column *"s. */ >>> + struct sset schema_columns; /* Column names from schema. */ >> >> Why did you decide to go with sset instead of 'in_server_schema' >> boolean for the struct ovsdb_idl_column? > > Two reasons: > 1. The ovsdb_idl_column instances for each table columns are > generated automatically by ovsdb/ovsdb-idlc.in > 2. 'struct ovsdb_idl_column *columns' is const in 'struct > ovsdb_idl_table_class' > https://github.com/openvswitch/ovs/blob/master/lib/ovsdb-idl.c#L722 Hmm. I see. We're allocating array of tables per idl instance, but we're reusing same columns from the table class, so they will be shared across idl instances and we can't modify them. OK. I the future, if we'll need more than one mutable parameter for a column, we would want to clone the column object for each idl instance, but for now the sset seems fine. > > >> >>> struct hmap rows; /* Contains "struct ovsdb_idl_row"s. */ >>> struct ovsdb_idl *idl; /* Containing IDL instance. */ >>> unsigned int change_seqno[OVSDB_IDL_CHANGE_MAX]; >>> + bool in_server_schema; /* Indicates if this table is in the server schema >>> + * or not. */ >>> struct ovs_list indexes; /* Contains "struct ovsdb_idl_index"s */ >>> struct ovs_list track_list; /* Tracked rows (ovsdb_idl_row.track_node). */ >>> }; >>> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c >>> index 2198c69c6..b2dfff46c 100644 >>> --- a/lib/ovsdb-idl.c >>> +++ b/lib/ovsdb-idl.c >>> @@ -287,6 +287,8 @@ ovsdb_idl_create_unconnected(const struct ovsdb_idl_class *class, >>> = table->change_seqno[OVSDB_IDL_CHANGE_MODIFY] >>> = table->change_seqno[OVSDB_IDL_CHANGE_DELETE] = 0; >>> table->idl = idl; >>> + table->in_server_schema = true; /* Assume it's in server schema. */ >> >> Hmm. Why is that? We do not make assumptions about columns, >> we should, probably, not do that for tables too. What do >> you think? > > I was thinking of the scenario where user calls ovsdb_has_table() at > the very beginning. It doesn't seem logical to check for existence if the idl was never connected. Maybe you can also add this prerequisite to the comments of the new API functions? > I'll remove it in v5. OK. > > >> >>> + sset_init(&table->schema_columns); >>> } >>> >>> return idl; >>> @@ -337,6 +339,7 @@ ovsdb_idl_destroy(struct ovsdb_idl *idl) >>> struct ovsdb_idl_table *table = &idl->tables[i]; >>> ovsdb_idl_destroy_indexes(table); >>> shash_destroy(&table->columns); >>> + sset_destroy(&table->schema_columns); >>> hmap_destroy(&table->rows); >>> free(table->modes); >>> } >>> @@ -718,6 +721,7 @@ ovsdb_idl_compose_monitor_request(const struct json *schema_json, void *idl_) >>> >>> struct json *columns >>> = table->need_table ? json_array_create_empty() : NULL; >>> + sset_clear(&table->schema_columns); >>> for (size_t j = 0; j < tc->n_columns; j++) { >>> const struct ovsdb_idl_column *column = &tc->columns[j]; >>> bool idl_has_column = (table_schema && >>> @@ -741,6 +745,7 @@ ovsdb_idl_compose_monitor_request(const struct json *schema_json, void *idl_) >>> } >>> json_array_add(columns, json_string_create(column->name)); >>> } >>> + sset_add(&table->schema_columns, column->name); >>> } >>> >>> if (columns) { >>> @@ -749,7 +754,12 @@ ovsdb_idl_compose_monitor_request(const struct json *schema_json, void *idl_) >>> "(database needs upgrade?)", >>> idl->class_->database, table->class_->name); >>> json_destroy(columns); >>> + /* Set 'table->in_server_schema' to false so that this can be >>> + * excluded from transactions. */ >> >> This comment seems redundant, since the functionality is in >> a different patch. > > Ack. > >> >>> + table->in_server_schema = false; >>> continue; >>> + } else if (schema && table_schema) { >>> + table->in_server_schema = true; >>> } >>> >>> monitor_request = json_object_create(); >>> @@ -4256,3 +4266,29 @@ ovsdb_idl_loop_commit_and_wait(struct ovsdb_idl_loop *loop) >>> >>> return retval; >>> } >>> + >>> +static struct ovsdb_idl_table* >>> +ovsdb_idl_get_table(struct ovsdb_idl *idl, const char *table_name) >>> +{ >>> + struct ovsdb_idl_table *table = shash_find_data(&idl->table_by_name, >>> + table_name); >>> + return table && table->in_server_schema ? table : NULL; >> >> It's better to parenthesize the condition, the priority of >> operations is not obvious. > > Ack. > >> >>> +} >>> + >>> +bool >>> +ovsdb_idl_has_table(struct ovsdb_idl *idl, const char *table_name) >>> +{ >>> + return ovsdb_idl_get_table(idl, table_name) ? true: false; >> >> Missing space before the ':'. > > Ack. > > >> >>> +} >>> + >>> +bool >>> +ovsdb_idl_has_column_in_table(struct ovsdb_idl *idl, const char *table_name, >>> + const char *column_name) >>> +{ >>> + struct ovsdb_idl_table *table = ovsdb_idl_get_table(idl, table_name); >> >> Empty line here would be nice to have. > > Ack. > > >> >>> + if (table && sset_find(&table->schema_columns, column_name)) { >>> + return true; >>> + } >>> + >>> + return false; >>> +} >>> diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h >>> index d93483245..48425b39a 100644 >>> --- a/lib/ovsdb-idl.h >>> +++ b/lib/ovsdb-idl.h >>> @@ -474,6 +474,9 @@ void ovsdb_idl_cursor_next_eq(struct ovsdb_idl_cursor *); >>> >>> struct ovsdb_idl_row *ovsdb_idl_cursor_data(struct ovsdb_idl_cursor *); >>> >>> +bool ovsdb_idl_has_table(struct ovsdb_idl *, const char *table_name); >>> +bool ovsdb_idl_has_column_in_table(struct ovsdb_idl *, const char *table_name, >>> + const char *column_name); >> >> This is a section of a header related to indexes and iteration. >> These functions should be defined somewhere close to >> ovsdb_idl_check_consistency() or ovsdb_idl_get_class(). > > Ack. > > >> >>> #ifdef __cplusplus >>> } >>> #endif >>> diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at >>> index 1386f1377..b11fabe70 100644 >>> --- a/tests/ovsdb-idl.at >>> +++ b/tests/ovsdb-idl.at >>> @@ -2372,3 +2372,41 @@ OVSDB_CHECK_CLUSTER_IDL([simple idl, initially empty, force reconnect], >>> [], >>> [], >>> reconnect.*waiting .* seconds before reconnect) >>> + >>> +AT_SETUP([idl table and column presence check]) >>> +AT_KEYWORDS([ovsdb server idl table column check]) >>> +AT_CHECK([ovsdb_start_idltest "" "$abs_srcdir/idltest2.ovsschema"]) >>> + >>> +ovsdb-tool create db2 $abs_srcdir/idltest.ovsschema >>> +ovsdb-server -vconsole:warn --log-file=ovsdb-server2.log --detach --no-chdir --pidfile=ovsdb-server2.pid --remote=punix:socket2 db2 >> >> Above two commands should be executed with AT_CHECK. >> It also would be nice to wrap the long line with 'dnl' inside AT_CHECK. > > Ack. > >> >>> +on_exit 'kill `cat ovsdb-server2.pid`' >>> + >>> +# In this test, test-ovsdb first connects to the server with schema >>> +# idltest2.ovsschema and outputs the presence of tables and columns. >>> +# And then it connectes to the server with the schema idltest.ovsschema >>> +# and does the same. >>> +AT_CHECK([test-ovsdb -vconsole:off -t10 idl-table-column-check unix:socket unix:socket2 \ >> >> It's better to use 'dnl' for breaking lines, so it can be properly >> printed in the testsuite log. > > Ack. > >> >>> +simple link1 link2 simple5 foo simple5:irefmap simple5:foo link1:l2], >>> +[0], [dnl >>> +table simple is present >>> +table link1 is present >>> +table link2 is not present >>> +table simple5 is not present >>> +table foo is not present >>> +table simple5 is not present >>> +table simple5 is not present >>> +column l2 in table link1 is not present >>> +--- remote 1 done --- >>> +table simple is present >>> +table link1 is present >>> +table link2 is present >>> +table simple5 is present >>> +table foo is not present >>> +column irefmap in table simple5 is present >>> +column foo in table simple5 is not present >>> +column l2 in table link1 is present >>> +--- remote 2 done --- >>> +]) >>> + >>> +OVSDB_SERVER_SHUTDOWN >>> +AT_CLEANUP >>> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c >>> index daa55dab7..462d2e57e 100644 >>> --- a/tests/test-ovsdb.c >>> +++ b/tests/test-ovsdb.c >>> @@ -3268,6 +3268,77 @@ do_idl_compound_index(struct ovs_cmdl_context *ctx) >>> printf("%03d: done\n", step); >>> } >>> >>> +static void >>> +do_idl_table_column_check(struct ovs_cmdl_context *ctx) >>> +{ >>> + struct jsonrpc *rpc; >>> + struct ovsdb_idl *idl; >>> + unsigned int seqno = 0; >>> + int error; >>> + int i; >>> + >>> + if (ctx->argc < 3) { >>> + exit(1); >>> + } >>> + >>> + idl = ovsdb_idl_create(ctx->argv[1], &idltest_idl_class, true, true); >>> + ovsdb_idl_set_leader_only(idl, false); >>> + struct stream *stream; >>> + >>> + error = stream_open_block(jsonrpc_stream_open(ctx->argv[1], &stream, >>> + DSCP_DEFAULT), -1, &stream); >>> + if (error) { >>> + ovs_fatal(error, "failed to connect to \"%s\"", ctx->argv[1]); >>> + } >>> + rpc = jsonrpc_open(stream); >>> + >>> + for (int r = 1; r <= 2; r++) { >>> + ovsdb_idl_set_remote(idl, ctx->argv[r], true); >>> + ovsdb_idl_force_reconnect(idl); >>> + >>> + /* Wait for update. */ >>> + for (;;) { >>> + ovsdb_idl_run(idl); >>> + ovsdb_idl_check_consistency(idl); >>> + if (ovsdb_idl_get_seqno(idl) != seqno) { >>> + break; >>> + } >>> + jsonrpc_run(rpc); >>> + >>> + ovsdb_idl_wait(idl); >>> + jsonrpc_wait(rpc); >>> + poll_block(); >>> + } >>> + >>> + seqno = ovsdb_idl_get_seqno(idl); >>> + >>> + for (i = 3; i < ctx->argc; i++) { >>> + char *save_ptr2 = NULL; >>> + char *arg = xstrdup(ctx->argv[i]); >>> + char *table_name = strtok_r(arg, ":", &save_ptr2); >>> + char *column_name = strtok_r(NULL, ":", &save_ptr2); >>> + >>> + bool table_present = ovsdb_idl_has_table(idl, table_name); >>> + if (!table_present || !column_name) { >>> + printf("table %s %s present\n", table_name, >>> + table_present ? "is" : "is not"); >> >> Please shif this line to be on the same level with the text inside >> the '()' above. > > Ack. > >> >>> + } >>> + if (table_present && column_name) { >>> + printf("column %s in table %s %s present\n", column_name, >>> + table_name, >>> + ovsdb_idl_has_column_in_table(idl, table_name, >>> + column_name) ? >>> + "is" : "is not"); >> >> Maybe: >> >> bool in = ovsdb_idl_has_column_in_table(idl, table_name, >> column_name); >> printf("column %s in table %s %s present\n", >> column_name, table_name, in ? "is" : "is not"); >> ? >> > > Ack. > > >>> + } >>> + free(arg); >>> + } >>> + printf("--- remote %d done ---\n", r); >>> + } >>> + >>> + jsonrpc_close(rpc); >>> + ovsdb_idl_destroy(idl); >>> +} >>> + >>> 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 }, >>> @@ -3306,6 +3377,8 @@ static struct ovs_cmdl_command all_commands[] = { >>> do_idl_partial_update_map_column, OVS_RO }, >>> { "idl-partial-update-set-column", NULL, 1, INT_MAX, >>> do_idl_partial_update_set_column, OVS_RO }, >>> + { "idl-table-column-check", NULL, 1, INT_MAX, >> >> It should be 2, I think, instead of 1, right? And you may remove >> the check for number of arguments from the function. > > Correct. I'll fix in v5. > > Thanks for the review. > > Numan > >> >>> + do_idl_table_column_check, 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 >>
On 8/26/21 11:39 AM, Ilya Maximets wrote: > On 8/26/21 1:44 AM, Numan Siddique wrote: >> ' >> >> On Wed, Aug 25, 2021 at 6:06 PM Ilya Maximets <i.maximets@ovn.org> wrote: >>> >>> On 8/25/21 5:12 AM, numans@ovn.org wrote: >>>> From: Numan Siddique <nusiddiq@redhat.com> >>>> >>>> This patch adds 2 new APIs in the ovsdb-idl client library >>>> - ovsdb_idl_has_table() and ovsdb_idl_has_column_in_table() to >>>> query if a table and a column is present in the IDL or not. This >>>> is required for scenarios where the server schema is old and >>>> missing a table or column and the client (built with a new schema >>>> version) does a transaction with the missing table or column. This >>>> results in a continuous loop of transaction failures. >>>> >>>> OVN would require the API - ovsdb_idl_has_table() to address this issue >>>> when an old ovsdb-server is used (OVS 2.11) which has the 'datapath' >>>> table missing. A recent commit in OVN creates a 'datapath' row >>>> in the local ovsdb-server. ovsdb_idl_has_column_in_table() would be >>>> useful to have. >>>> >>>> Related issue: https://bugzilla.redhat.com/show_bug.cgi?id=1992705 >>>> Signed-off-by: Numan Siddique <nusiddiq@redhat.com> >>>> --- >>>> lib/ovsdb-idl-provider.h | 4 +++ >>>> lib/ovsdb-idl.c | 36 ++++++++++++++++++++ >>>> lib/ovsdb-idl.h | 3 ++ >>>> tests/ovsdb-idl.at | 38 +++++++++++++++++++++ >>>> tests/test-ovsdb.c | 73 ++++++++++++++++++++++++++++++++++++++++ >>>> 5 files changed, 154 insertions(+) >>>> >>>> diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h >>>> index 0f38f9b34..0f122e23c 100644 >>>> --- a/lib/ovsdb-idl-provider.h >>>> +++ b/lib/ovsdb-idl-provider.h >>>> @@ -24,6 +24,7 @@ >>>> #include "ovsdb-set-op.h" >>>> #include "ovsdb-types.h" >>>> #include "openvswitch/shash.h" >>>> +#include "sset.h" >>>> #include "uuid.h" >>>> >>>> #ifdef __cplusplus >>>> @@ -117,9 +118,12 @@ struct ovsdb_idl_table { >>>> bool need_table; /* Monitor table even if no columns are selected >>>> * for replication. */ >>>> struct shash columns; /* Contains "const struct ovsdb_idl_column *"s. */ >>>> + struct sset schema_columns; /* Column names from schema. */ >>> >>> Why did you decide to go with sset instead of 'in_server_schema' >>> boolean for the struct ovsdb_idl_column? >> >> Two reasons: >> 1. The ovsdb_idl_column instances for each table columns are >> generated automatically by ovsdb/ovsdb-idlc.in >> 2. 'struct ovsdb_idl_column *columns' is const in 'struct >> ovsdb_idl_table_class' >> https://github.com/openvswitch/ovs/blob/master/lib/ovsdb-idl.c#L722 > > Hmm. I see. We're allocating array of tables per idl instance, > but we're reusing same columns from the table class, so they will > be shared across idl instances and we can't modify them. > OK. I the future, if we'll need more than one mutable parameter > for a column, we would want to clone the column object for each > idl instance, but for now the sset seems fine. > >> >> >>> >>>> struct hmap rows; /* Contains "struct ovsdb_idl_row"s. */ >>>> struct ovsdb_idl *idl; /* Containing IDL instance. */ >>>> unsigned int change_seqno[OVSDB_IDL_CHANGE_MAX]; >>>> + bool in_server_schema; /* Indicates if this table is in the server schema >>>> + * or not. */ >>>> struct ovs_list indexes; /* Contains "struct ovsdb_idl_index"s */ >>>> struct ovs_list track_list; /* Tracked rows (ovsdb_idl_row.track_node). */ >>>> }; >>>> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c >>>> index 2198c69c6..b2dfff46c 100644 >>>> --- a/lib/ovsdb-idl.c >>>> +++ b/lib/ovsdb-idl.c >>>> @@ -287,6 +287,8 @@ ovsdb_idl_create_unconnected(const struct ovsdb_idl_class *class, >>>> = table->change_seqno[OVSDB_IDL_CHANGE_MODIFY] >>>> = table->change_seqno[OVSDB_IDL_CHANGE_DELETE] = 0; >>>> table->idl = idl; >>>> + table->in_server_schema = true; /* Assume it's in server schema. */ >>> >>> Hmm. Why is that? We do not make assumptions about columns, >>> we should, probably, not do that for tables too. What do >>> you think? >> >> I was thinking of the scenario where user calls ovsdb_has_table() at >> the very beginning. > > It doesn't seem logical to check for existence if the idl was never > connected. Maybe you can also add this prerequisite to the comments > of the new API functions? > >> I'll remove it in v5. > > OK. > >> >> >>> >>>> + sset_init(&table->schema_columns); >>>> } >>>> >>>> return idl; >>>> @@ -337,6 +339,7 @@ ovsdb_idl_destroy(struct ovsdb_idl *idl) >>>> struct ovsdb_idl_table *table = &idl->tables[i]; >>>> ovsdb_idl_destroy_indexes(table); >>>> shash_destroy(&table->columns); >>>> + sset_destroy(&table->schema_columns); >>>> hmap_destroy(&table->rows); >>>> free(table->modes); >>>> } >>>> @@ -718,6 +721,7 @@ ovsdb_idl_compose_monitor_request(const struct json *schema_json, void *idl_) >>>> >>>> struct json *columns >>>> = table->need_table ? json_array_create_empty() : NULL; >>>> + sset_clear(&table->schema_columns); >>>> for (size_t j = 0; j < tc->n_columns; j++) { >>>> const struct ovsdb_idl_column *column = &tc->columns[j]; >>>> bool idl_has_column = (table_schema && >>>> @@ -741,6 +745,7 @@ ovsdb_idl_compose_monitor_request(const struct json *schema_json, void *idl_) >>>> } >>>> json_array_add(columns, json_string_create(column->name)); >>>> } >>>> + sset_add(&table->schema_columns, column->name); Also, shouldn't this be conditional? We're iterating over columns in a table class, which is on idl's side, so if !idl_has_column, we should not add column to the set. The test fails if I'm ading ovsdb_idl_omit(idl, &idltest_link1_col_l2); after creation of the idl instance, i.e. it thinks that it has l2 column, while it doesn't. Would be great to have this case covered by a test, I think. Not strictly related, but 'idl_has_column' seems to be a misleading name, should it be 'server_has_column' instead? It would also be good to have autogenerated wrappers for new functions, so users can check like this: if (idltest_has_link1_table(idl)) ... if (idltest_link1_has_l2_column(idl)) ... This also means changing the API of other functions, i.e. to have bool ovsdb_idl_has_table(const struct ovsdb_idl *, const struct ovsdb_idl_table_class *); bool ovsdb_idl_has_column(const struct ovsdb_idl *, const struct ovsdb_idl_column *); Even without wrappers, it seems like more conventional API. And wrappers will look like this: bool idltest_link1_has_l2_column(const struct ovsdb_idl *idl) { return ovsdb_idl_has_column(idl, &idltest_link1_col_l2); } The test will need to be hardcoded, but this should not be a big problem. On a bright side, with a hardcoded test it should be easier to test the ovsdb_idl_omit() case. What do you think? Best regards, Ilya Maximets.
On Thu, Aug 26, 2021 at 7:45 AM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 8/26/21 11:39 AM, Ilya Maximets wrote: > > On 8/26/21 1:44 AM, Numan Siddique wrote: > >> ' > >> > >> On Wed, Aug 25, 2021 at 6:06 PM Ilya Maximets <i.maximets@ovn.org> wrote: > >>> > >>> On 8/25/21 5:12 AM, numans@ovn.org wrote: > >>>> From: Numan Siddique <nusiddiq@redhat.com> > >>>> > >>>> This patch adds 2 new APIs in the ovsdb-idl client library > >>>> - ovsdb_idl_has_table() and ovsdb_idl_has_column_in_table() to > >>>> query if a table and a column is present in the IDL or not. This > >>>> is required for scenarios where the server schema is old and > >>>> missing a table or column and the client (built with a new schema > >>>> version) does a transaction with the missing table or column. This > >>>> results in a continuous loop of transaction failures. > >>>> > >>>> OVN would require the API - ovsdb_idl_has_table() to address this issue > >>>> when an old ovsdb-server is used (OVS 2.11) which has the 'datapath' > >>>> table missing. A recent commit in OVN creates a 'datapath' row > >>>> in the local ovsdb-server. ovsdb_idl_has_column_in_table() would be > >>>> useful to have. > >>>> > >>>> Related issue: https://bugzilla.redhat.com/show_bug.cgi?id=1992705 > >>>> Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > >>>> --- > >>>> lib/ovsdb-idl-provider.h | 4 +++ > >>>> lib/ovsdb-idl.c | 36 ++++++++++++++++++++ > >>>> lib/ovsdb-idl.h | 3 ++ > >>>> tests/ovsdb-idl.at | 38 +++++++++++++++++++++ > >>>> tests/test-ovsdb.c | 73 ++++++++++++++++++++++++++++++++++++++++ > >>>> 5 files changed, 154 insertions(+) > >>>> > >>>> diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h > >>>> index 0f38f9b34..0f122e23c 100644 > >>>> --- a/lib/ovsdb-idl-provider.h > >>>> +++ b/lib/ovsdb-idl-provider.h > >>>> @@ -24,6 +24,7 @@ > >>>> #include "ovsdb-set-op.h" > >>>> #include "ovsdb-types.h" > >>>> #include "openvswitch/shash.h" > >>>> +#include "sset.h" > >>>> #include "uuid.h" > >>>> > >>>> #ifdef __cplusplus > >>>> @@ -117,9 +118,12 @@ struct ovsdb_idl_table { > >>>> bool need_table; /* Monitor table even if no columns are selected > >>>> * for replication. */ > >>>> struct shash columns; /* Contains "const struct ovsdb_idl_column *"s. */ > >>>> + struct sset schema_columns; /* Column names from schema. */ > >>> > >>> Why did you decide to go with sset instead of 'in_server_schema' > >>> boolean for the struct ovsdb_idl_column? > >> > >> Two reasons: > >> 1. The ovsdb_idl_column instances for each table columns are > >> generated automatically by ovsdb/ovsdb-idlc.in > >> 2. 'struct ovsdb_idl_column *columns' is const in 'struct > >> ovsdb_idl_table_class' > >> https://github.com/openvswitch/ovs/blob/master/lib/ovsdb-idl.c#L722 > > > > Hmm. I see. We're allocating array of tables per idl instance, > > but we're reusing same columns from the table class, so they will > > be shared across idl instances and we can't modify them. > > OK. I the future, if we'll need more than one mutable parameter > > for a column, we would want to clone the column object for each > > idl instance, but for now the sset seems fine. > > > >> > >> > >>> > >>>> struct hmap rows; /* Contains "struct ovsdb_idl_row"s. */ > >>>> struct ovsdb_idl *idl; /* Containing IDL instance. */ > >>>> unsigned int change_seqno[OVSDB_IDL_CHANGE_MAX]; > >>>> + bool in_server_schema; /* Indicates if this table is in the server schema > >>>> + * or not. */ > >>>> struct ovs_list indexes; /* Contains "struct ovsdb_idl_index"s */ > >>>> struct ovs_list track_list; /* Tracked rows (ovsdb_idl_row.track_node). */ > >>>> }; > >>>> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > >>>> index 2198c69c6..b2dfff46c 100644 > >>>> --- a/lib/ovsdb-idl.c > >>>> +++ b/lib/ovsdb-idl.c > >>>> @@ -287,6 +287,8 @@ ovsdb_idl_create_unconnected(const struct ovsdb_idl_class *class, > >>>> = table->change_seqno[OVSDB_IDL_CHANGE_MODIFY] > >>>> = table->change_seqno[OVSDB_IDL_CHANGE_DELETE] = 0; > >>>> table->idl = idl; > >>>> + table->in_server_schema = true; /* Assume it's in server schema. */ > >>> > >>> Hmm. Why is that? We do not make assumptions about columns, > >>> we should, probably, not do that for tables too. What do > >>> you think? > >> > >> I was thinking of the scenario where user calls ovsdb_has_table() at > >> the very beginning. > > > > It doesn't seem logical to check for existence if the idl was never > > connected. Maybe you can also add this prerequisite to the comments > > of the new API functions? I sent out v6 addressing the review comments. But I somehow missed this. If the v6 patch 1 looks good to you apart from this comment, can you please add the missing comment before applying ? > > > >> I'll remove it in v5. > > > > OK. > > > >> > >> > >>> > >>>> + sset_init(&table->schema_columns); > >>>> } > >>>> > >>>> return idl; > >>>> @@ -337,6 +339,7 @@ ovsdb_idl_destroy(struct ovsdb_idl *idl) > >>>> struct ovsdb_idl_table *table = &idl->tables[i]; > >>>> ovsdb_idl_destroy_indexes(table); > >>>> shash_destroy(&table->columns); > >>>> + sset_destroy(&table->schema_columns); > >>>> hmap_destroy(&table->rows); > >>>> free(table->modes); > >>>> } > >>>> @@ -718,6 +721,7 @@ ovsdb_idl_compose_monitor_request(const struct json *schema_json, void *idl_) > >>>> > >>>> struct json *columns > >>>> = table->need_table ? json_array_create_empty() : NULL; > >>>> + sset_clear(&table->schema_columns); > >>>> for (size_t j = 0; j < tc->n_columns; j++) { > >>>> const struct ovsdb_idl_column *column = &tc->columns[j]; > >>>> bool idl_has_column = (table_schema && > >>>> @@ -741,6 +745,7 @@ ovsdb_idl_compose_monitor_request(const struct json *schema_json, void *idl_) > >>>> } > >>>> json_array_add(columns, json_string_create(column->name)); > >>>> } > >>>> + sset_add(&table->schema_columns, column->name); > > Also, shouldn't this be conditional? > We're iterating over columns in a table class, which is on idl's side, > so if !idl_has_column, we should not add column to the set. > The test fails if I'm ading ovsdb_idl_omit(idl, &idltest_link1_col_l2); > after creation of the idl instance, i.e. it thinks that it has l2 > column, while it doesn't. Would be great to have this case covered > by a test, I think. > As we discussed offline, the function ovsdb_idl_server_has_column() will return true If server schema has the column and returns false otherwise irrespective of the omit or other monitor related things. > Not strictly related, but 'idl_has_column' seems to be a misleading name, > should it be 'server_has_column' instead? Ack. I've changed the names now. > > It would also be good to have autogenerated wrappers for new functions, > so users can check like this: Addressed in v6. > > if (idltest_has_link1_table(idl)) ... > if (idltest_link1_has_l2_column(idl)) ... > > This also means changing the API of other functions, i.e. to have > > bool ovsdb_idl_has_table(const struct ovsdb_idl *, > const struct ovsdb_idl_table_class *); > bool ovsdb_idl_has_column(const struct ovsdb_idl *, > const struct ovsdb_idl_column *); > > Even without wrappers, it seems like more conventional API. > > And wrappers will look like this: Ack. Done in v6. > > bool > idltest_link1_has_l2_column(const struct ovsdb_idl *idl) > { > return ovsdb_idl_has_column(idl, &idltest_link1_col_l2); > } > > The test will need to be hardcoded, but this should not be a big problem. > On a bright side, with a hardcoded test it should be easier to test the > ovsdb_idl_omit() case. > > What do you think? Ack. Done in v6. Thanks Numan > > Best regards, Ilya Maximets. > _______________________________________________ > 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 0f38f9b34..0f122e23c 100644 --- a/lib/ovsdb-idl-provider.h +++ b/lib/ovsdb-idl-provider.h @@ -24,6 +24,7 @@ #include "ovsdb-set-op.h" #include "ovsdb-types.h" #include "openvswitch/shash.h" +#include "sset.h" #include "uuid.h" #ifdef __cplusplus @@ -117,9 +118,12 @@ struct ovsdb_idl_table { bool need_table; /* Monitor table even if no columns are selected * for replication. */ struct shash columns; /* Contains "const struct ovsdb_idl_column *"s. */ + struct sset schema_columns; /* Column names from schema. */ struct hmap rows; /* Contains "struct ovsdb_idl_row"s. */ struct ovsdb_idl *idl; /* Containing IDL instance. */ unsigned int change_seqno[OVSDB_IDL_CHANGE_MAX]; + bool in_server_schema; /* Indicates if this table is in the server schema + * or not. */ struct ovs_list indexes; /* Contains "struct ovsdb_idl_index"s */ struct ovs_list track_list; /* Tracked rows (ovsdb_idl_row.track_node). */ }; diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index 2198c69c6..b2dfff46c 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -287,6 +287,8 @@ ovsdb_idl_create_unconnected(const struct ovsdb_idl_class *class, = table->change_seqno[OVSDB_IDL_CHANGE_MODIFY] = table->change_seqno[OVSDB_IDL_CHANGE_DELETE] = 0; table->idl = idl; + table->in_server_schema = true; /* Assume it's in server schema. */ + sset_init(&table->schema_columns); } return idl; @@ -337,6 +339,7 @@ ovsdb_idl_destroy(struct ovsdb_idl *idl) struct ovsdb_idl_table *table = &idl->tables[i]; ovsdb_idl_destroy_indexes(table); shash_destroy(&table->columns); + sset_destroy(&table->schema_columns); hmap_destroy(&table->rows); free(table->modes); } @@ -718,6 +721,7 @@ ovsdb_idl_compose_monitor_request(const struct json *schema_json, void *idl_) struct json *columns = table->need_table ? json_array_create_empty() : NULL; + sset_clear(&table->schema_columns); for (size_t j = 0; j < tc->n_columns; j++) { const struct ovsdb_idl_column *column = &tc->columns[j]; bool idl_has_column = (table_schema && @@ -741,6 +745,7 @@ ovsdb_idl_compose_monitor_request(const struct json *schema_json, void *idl_) } json_array_add(columns, json_string_create(column->name)); } + sset_add(&table->schema_columns, column->name); } if (columns) { @@ -749,7 +754,12 @@ ovsdb_idl_compose_monitor_request(const struct json *schema_json, void *idl_) "(database needs upgrade?)", idl->class_->database, table->class_->name); json_destroy(columns); + /* Set 'table->in_server_schema' to false so that this can be + * excluded from transactions. */ + table->in_server_schema = false; continue; + } else if (schema && table_schema) { + table->in_server_schema = true; } monitor_request = json_object_create(); @@ -4256,3 +4266,29 @@ ovsdb_idl_loop_commit_and_wait(struct ovsdb_idl_loop *loop) return retval; } + +static struct ovsdb_idl_table* +ovsdb_idl_get_table(struct ovsdb_idl *idl, const char *table_name) +{ + struct ovsdb_idl_table *table = shash_find_data(&idl->table_by_name, + table_name); + return table && table->in_server_schema ? table : NULL; +} + +bool +ovsdb_idl_has_table(struct ovsdb_idl *idl, const char *table_name) +{ + return ovsdb_idl_get_table(idl, table_name) ? true: false; +} + +bool +ovsdb_idl_has_column_in_table(struct ovsdb_idl *idl, const char *table_name, + const char *column_name) +{ + struct ovsdb_idl_table *table = ovsdb_idl_get_table(idl, table_name); + if (table && sset_find(&table->schema_columns, column_name)) { + return true; + } + + return false; +} diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h index d93483245..48425b39a 100644 --- a/lib/ovsdb-idl.h +++ b/lib/ovsdb-idl.h @@ -474,6 +474,9 @@ void ovsdb_idl_cursor_next_eq(struct ovsdb_idl_cursor *); struct ovsdb_idl_row *ovsdb_idl_cursor_data(struct ovsdb_idl_cursor *); +bool ovsdb_idl_has_table(struct ovsdb_idl *, const char *table_name); +bool ovsdb_idl_has_column_in_table(struct ovsdb_idl *, const char *table_name, + const char *column_name); #ifdef __cplusplus } #endif diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at index 1386f1377..b11fabe70 100644 --- a/tests/ovsdb-idl.at +++ b/tests/ovsdb-idl.at @@ -2372,3 +2372,41 @@ OVSDB_CHECK_CLUSTER_IDL([simple idl, initially empty, force reconnect], [], [], reconnect.*waiting .* seconds before reconnect) + +AT_SETUP([idl table and column presence check]) +AT_KEYWORDS([ovsdb server idl table column check]) +AT_CHECK([ovsdb_start_idltest "" "$abs_srcdir/idltest2.ovsschema"]) + +ovsdb-tool create db2 $abs_srcdir/idltest.ovsschema +ovsdb-server -vconsole:warn --log-file=ovsdb-server2.log --detach --no-chdir --pidfile=ovsdb-server2.pid --remote=punix:socket2 db2 +on_exit 'kill `cat ovsdb-server2.pid`' + +# In this test, test-ovsdb first connects to the server with schema +# idltest2.ovsschema and outputs the presence of tables and columns. +# And then it connectes to the server with the schema idltest.ovsschema +# and does the same. +AT_CHECK([test-ovsdb -vconsole:off -t10 idl-table-column-check unix:socket unix:socket2 \ +simple link1 link2 simple5 foo simple5:irefmap simple5:foo link1:l2], +[0], [dnl +table simple is present +table link1 is present +table link2 is not present +table simple5 is not present +table foo is not present +table simple5 is not present +table simple5 is not present +column l2 in table link1 is not present +--- remote 1 done --- +table simple is present +table link1 is present +table link2 is present +table simple5 is present +table foo is not present +column irefmap in table simple5 is present +column foo in table simple5 is not present +column l2 in table link1 is present +--- remote 2 done --- +]) + +OVSDB_SERVER_SHUTDOWN +AT_CLEANUP diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c index daa55dab7..462d2e57e 100644 --- a/tests/test-ovsdb.c +++ b/tests/test-ovsdb.c @@ -3268,6 +3268,77 @@ do_idl_compound_index(struct ovs_cmdl_context *ctx) printf("%03d: done\n", step); } +static void +do_idl_table_column_check(struct ovs_cmdl_context *ctx) +{ + struct jsonrpc *rpc; + struct ovsdb_idl *idl; + unsigned int seqno = 0; + int error; + int i; + + if (ctx->argc < 3) { + exit(1); + } + + idl = ovsdb_idl_create(ctx->argv[1], &idltest_idl_class, true, true); + ovsdb_idl_set_leader_only(idl, false); + struct stream *stream; + + error = stream_open_block(jsonrpc_stream_open(ctx->argv[1], &stream, + DSCP_DEFAULT), -1, &stream); + if (error) { + ovs_fatal(error, "failed to connect to \"%s\"", ctx->argv[1]); + } + rpc = jsonrpc_open(stream); + + for (int r = 1; r <= 2; r++) { + ovsdb_idl_set_remote(idl, ctx->argv[r], true); + ovsdb_idl_force_reconnect(idl); + + /* Wait for update. */ + for (;;) { + ovsdb_idl_run(idl); + ovsdb_idl_check_consistency(idl); + if (ovsdb_idl_get_seqno(idl) != seqno) { + break; + } + jsonrpc_run(rpc); + + ovsdb_idl_wait(idl); + jsonrpc_wait(rpc); + poll_block(); + } + + seqno = ovsdb_idl_get_seqno(idl); + + for (i = 3; i < ctx->argc; i++) { + char *save_ptr2 = NULL; + char *arg = xstrdup(ctx->argv[i]); + char *table_name = strtok_r(arg, ":", &save_ptr2); + char *column_name = strtok_r(NULL, ":", &save_ptr2); + + bool table_present = ovsdb_idl_has_table(idl, table_name); + if (!table_present || !column_name) { + printf("table %s %s present\n", table_name, + table_present ? "is" : "is not"); + } + if (table_present && column_name) { + printf("column %s in table %s %s present\n", column_name, + table_name, + ovsdb_idl_has_column_in_table(idl, table_name, + column_name) ? + "is" : "is not"); + } + free(arg); + } + printf("--- remote %d done ---\n", r); + } + + jsonrpc_close(rpc); + ovsdb_idl_destroy(idl); +} + 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 }, @@ -3306,6 +3377,8 @@ static struct ovs_cmdl_command all_commands[] = { do_idl_partial_update_map_column, OVS_RO }, { "idl-partial-update-set-column", NULL, 1, INT_MAX, do_idl_partial_update_set_column, OVS_RO }, + { "idl-table-column-check", NULL, 1, INT_MAX, + do_idl_table_column_check, OVS_RO }, { "help", NULL, 0, INT_MAX, do_help, OVS_RO }, { NULL, NULL, 0, 0, NULL, OVS_RO }, };