[ovs-dev] [PATCH v4 1/2] ovsdb-idl : Add APIs to query if a table and a column is present or not.

Ilya Maximets i.maximets at ovn.org
Thu Aug 26 09:39:53 UTC 2021


On 8/26/21 1:44 AM, Numan Siddique wrote:
> '
> 
> On Wed, Aug 25, 2021 at 6:06 PM Ilya Maximets <i.maximets at ovn.org> wrote:
>>
>> On 8/25/21 5:12 AM, numans at ovn.org wrote:
>>> From: Numan Siddique <nusiddiq at 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 at 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 at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>



More information about the dev mailing list