Currently, there is no convenient way to know what are the constraints for a particular column in the server's schema in C IDL. This is a problem, because clients may want to know how many elements are allowed in a certain column. For example, we recently increased the allowed number of prefixes configured in the Flow_Table table in OVS, but the client (ovn-controller) has no good way to know how many prefixes are actually supported in the schema of the currently running ovsdb-server. The IDL's code is generated from one schema version, while the actual server may be using newer or older one. If the client specifies too many prefixes, the transaction will fail, and there is also no good way to tell from the ovn-controller why exactly transaction failed.
Currently used solution is to create another database connection just to intercept schema changes and parse the schema JSON manually inside the ovn-controller: https://github.com/ovn-org/ovn/commit/89e43f7528b067b1bc9f6c5fd67857b39ebc518d While this approach works, it's not a clean solution. We have the server's schema on the CS level and we can provide the types to the application via IDL functions. This will allow ovn-controller to just use ovsrec_flow_table_prefixes_server_type(idl)->n_max instead of all the awkward schema parsing. Python IDL is more dynamic and has a different way of connecting where the user first obtains the schema and then initializes IDL with that schema. The parsed schema object with all the types is also available through the get_idl_schema() method. So, it is already possible to check the types there. Signed-off-by: Ilya Maximets <i.maxim...@ovn.org> --- NEWS | 4 +++ lib/ovsdb-cs.c | 64 +++++++++++++++++++++++++--------- lib/ovsdb-idl-provider.h | 3 +- lib/ovsdb-idl.c | 74 ++++++++++++++++++++++++++++++++++------ lib/ovsdb-idl.h | 3 ++ ovsdb/ovsdb-idlc.in | 12 +++++++ tests/ovsdb-idl.at | 10 +++--- tests/test-ovsdb.c | 33 +++++++++++++----- 8 files changed, 162 insertions(+), 41 deletions(-) diff --git a/NEWS b/NEWS index d7231fabc..7f2d3462a 100644 --- a/NEWS +++ b/NEWS @@ -8,6 +8,10 @@ Post-v3.5.0 * New debug appctl command 'dpdk/get-memzone-stats'. * Removed upper limit for the number of rx/tx descriptors (n_r/txq_desc). * OVS validated with DPDK 24.11.2. + - OVSDB-IDL: + * New functions <db>_<table>_<column>_server_type() that allow checking + the server-side type of a particular column. Can be used for checking + type constraints when the server schema is older or newer than client's. - ovs-ctl: * Added a new option, --oom-score=<score>, to set the daemons' Linux Out-Of-Memory (OOM) killer score. diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c index 87c6211bf..2d78bb84a 100644 --- a/lib/ovsdb-cs.c +++ b/lib/ovsdb-cs.c @@ -33,7 +33,6 @@ #include "ovsdb-parser.h" #include "ovsdb-session.h" #include "ovsdb-types.h" -#include "sset.h" #include "svec.h" #include "util.h" #include "uuid.h" @@ -2050,7 +2049,7 @@ ovsdb_cs_compose_server_monitor_request(const struct json *schema_json, struct json *monitor_requests = json_object_create(); const char *table_name = "Database"; - const struct sset *table_schema + const struct shash *table_schema = schema ? shash_find_data(schema, table_name) : NULL; if (!table_schema) { VLOG_WARN("%s database lacks %s table " @@ -2062,7 +2061,7 @@ ovsdb_cs_compose_server_monitor_request(const struct json *schema_json, for (size_t j = 0; j < N_SERVER_COLUMNS; j++) { const struct server_column *column = &server_columns[j]; bool db_has_column = (table_schema && - sset_contains(table_schema, column->name)); + shash_find(table_schema, column->name)); if (table_schema && !db_has_column) { VLOG_WARN("%s table in %s database lacks %s column " "(database needs upgrade?)", @@ -2096,9 +2095,9 @@ log_error(struct ovsdb_error *error) /* Parses 'schema_json', an OVSDB schema in JSON format as described in RFC * 7047, to obtain the names of its rows and columns. If successful, returns - * an shash whose keys are table names and whose values are ssets, where each - * sset contains the names of its table's columns. On failure (due to a parse - * error), returns NULL. + * an shash whose keys are table names and whose values are shashes, where each + * shash contains the names of its table's columns as keys and an ovsdb_type as + * data. On failure (due to a parse error), returns NULL. * * It would also be possible to use the general-purpose OVSDB schema parser in * ovsdb-server, but that's overkill, possibly too strict for the current use @@ -2109,9 +2108,9 @@ ovsdb_cs_parse_schema(const struct json *schema_json) { struct ovsdb_parser parser; const struct json *tables_json; + struct shash *schema = NULL; struct ovsdb_error *error; struct shash_node *node; - struct shash *schema; ovsdb_parser_init(&parser, schema_json, "database schema"); tables_json = ovsdb_parser_member(&parser, "tables", OP_OBJECT); @@ -2133,22 +2132,49 @@ ovsdb_cs_parse_schema(const struct json *schema_json) columns_json = ovsdb_parser_member(&parser, "columns", OP_OBJECT); error = ovsdb_parser_destroy(&parser); if (error) { - log_error(error); - ovsdb_cs_free_schema(schema); - return NULL; + goto error; } - struct sset *columns = xmalloc(sizeof *columns); - sset_init(columns); + struct shash *columns = xmalloc(sizeof *columns); + shash_init(columns); struct shash_node *node2; SHASH_FOR_EACH (node2, json_object(columns_json)) { + const struct json *column_json = node2->data; const char *column_name = node2->name; - sset_add(columns, column_name); + const struct json *type_json; + struct ovsdb_type *type; + + ovsdb_parser_init(&parser, column_json, + "schema for column %s", column_name); + type_json = ovsdb_parser_member(&parser, "type", + OP_STRING | OP_OBJECT); + error = ovsdb_parser_destroy(&parser); + if (error) { + goto error; + } + + type = xzalloc(sizeof *type); + error = ovsdb_type_from_json(type, type_json); + if (error) { + free(type); + goto error; + } + + if (!shash_add_once(columns, column_name, type)) { + /* Should never happen, but just in case. */ + ovsdb_type_destroy(type); + free(type); + } } shash_add(schema, table_name, columns); } return schema; + +error: + log_error(error); + ovsdb_cs_free_schema(schema); + return NULL; } /* Frees 'schema', which is in the format returned by @@ -2160,9 +2186,15 @@ ovsdb_cs_free_schema(struct shash *schema) struct shash_node *node; SHASH_FOR_EACH_SAFE (node, schema) { - struct sset *sset = node->data; - sset_destroy(sset); - free(sset); + struct shash *columns = node->data; + struct shash_node *node2; + + SHASH_FOR_EACH (node2, columns) { + ovsdb_type_destroy(node2->data); + } + shash_destroy_free_data(columns); + free(columns); + shash_delete(schema, node); } shash_destroy(schema); diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h index 8d2b7d6b9..6cf32fb50 100644 --- a/lib/ovsdb-idl-provider.h +++ b/lib/ovsdb-idl-provider.h @@ -121,7 +121,8 @@ 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 shash schema_columns; /* Contains "const struct ovsdb_type *" per + * column as defined in the server 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]; diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index 226a4b475..1c160558b 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -297,7 +297,7 @@ ovsdb_idl_create_unconnected(const struct ovsdb_idl_class *class, = table->change_seqno[OVSDB_IDL_CHANGE_DELETE] = 0; table->idl = idl; table->in_server_schema = false; - sset_init(&table->schema_columns); + shash_init(&table->schema_columns); } return idl; @@ -340,6 +340,17 @@ ovsdb_idl_reset_min_index(struct ovsdb_idl *idl) ovsdb_cs_reset_min_index(idl->cs); } +static void +ovsdb_idl_schema_columns_clear(struct shash *schema_columns) +{ + struct shash_node *node; + + SHASH_FOR_EACH (node, schema_columns) { + ovsdb_type_destroy(node->data); + } + shash_clear_free_data(schema_columns); +} + /* Destroys 'idl' and all of the data structures that it manages. */ void ovsdb_idl_destroy(struct ovsdb_idl *idl) @@ -354,9 +365,13 @@ ovsdb_idl_destroy(struct ovsdb_idl *idl) ovsdb_cs_destroy(idl->cs); for (size_t i = 0; i < idl->class_->n_tables; i++) { struct ovsdb_idl_table *table = &idl->tables[i]; + ovsdb_idl_destroy_indexes(table); shash_destroy(&table->columns); - sset_destroy(&table->schema_columns); + + ovsdb_idl_schema_columns_clear(&table->schema_columns); + shash_destroy(&table->schema_columns); + hmap_destroy(&table->rows); free(table->modes); } @@ -776,19 +791,26 @@ ovsdb_idl_compose_monitor_request(const struct json *schema_json, void *idl_) struct ovsdb_idl_table *table = &idl->tables[i]; const struct ovsdb_idl_table_class *tc = table->class_; struct json *monitor_request; - const struct sset *table_schema + struct shash *table_schema = schema ? shash_find_data(schema, table->class_->name) : NULL; struct json *columns = table->need_table ? json_array_create_empty() : NULL; - sset_clear(&table->schema_columns); + + ovsdb_idl_schema_columns_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 && - sset_contains(table_schema, column->name)); - - if (idl_has_column) { - sset_add(&table->schema_columns, column->name); + bool idl_has_column = false; + struct shash_node *node; + + if (table_schema) { + node = shash_find(table_schema, column->name); + if (node) { + idl_has_column = true; + shash_add(&table->schema_columns, + column->name, node->data); + shash_delete(table_schema, node); + } } if (column->is_synthetic) { @@ -995,8 +1017,38 @@ ovsdb_idl_server_has_column(const struct ovsdb_idl *idl, const struct ovsdb_idl_table *table = ovsdb_idl_table_from_column(idl, column); - return (table->in_server_schema && sset_find(&table->schema_columns, - column->name)); + return (table->in_server_schema && shash_find(&table->schema_columns, + column->name)); +} + +/* Returns the type of a 'column' as defined in the schema reported + * by the server if the 'idl' has seen the 'column' in that schema. + * Returns NULL otherwise. + * + * Always returns NULL if idl has never been connected. + * + * Please see ovsdb_idl_compose_monitor_request() which sets + * 'struct ovsdb_idl_table *'->schema_columns accordingly. + * + * Usually this function is used indirectly through one of the + * "server_column_type" functions generated by ovsdb-idlc. + * + * Having different types between the server and the client in many + * cases will result in complete communication breakdown, so this + * function is mostly useful for checking for type constraints (enum, + * n_max) in case the server's schema is older or newer. */ +const struct ovsdb_type * +ovsdb_idl_server_column_type(const struct ovsdb_idl *idl, + const struct ovsdb_idl_column *column) +{ + const struct ovsdb_idl_table *table = + ovsdb_idl_table_from_column(idl, column); + + if (!table->in_server_schema) { + return NULL; + } + + return shash_find_data(&table->schema_columns, column->name); } /* A single clause within an ovsdb_idl_condition. */ diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h index 86fd2bd36..0d4864b6f 100644 --- a/lib/ovsdb-idl.h +++ b/lib/ovsdb-idl.h @@ -55,6 +55,7 @@ struct ovsdb_idl_row; struct ovsdb_idl_column; struct ovsdb_idl_table; struct ovsdb_idl_table_class; +struct ovsdb_type; struct simap; struct uuid; @@ -103,6 +104,8 @@ bool ovsdb_idl_server_has_table(const struct ovsdb_idl *, const struct ovsdb_idl_table_class *); bool ovsdb_idl_server_has_column(const struct ovsdb_idl *, const struct ovsdb_idl_column *); +const struct ovsdb_type *ovsdb_idl_server_column_type( + const struct ovsdb_idl *, const struct ovsdb_idl_column *); /* Choosing columns and tables to replicate. * diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in index 9a54f06a1..42a56dd05 100755 --- a/ovsdb/ovsdb-idlc.in +++ b/ovsdb/ovsdb-idlc.in @@ -204,6 +204,9 @@ extern "C" { %(hDecls)s''' % {'prefix': prefix.upper(), 'hDecls': schema.hDecls}) + print("struct ovsdb_type;") + print("") + for tableName, table in sorted(schema.tables.items()): structName = "%s%s" % (prefix, tableName.lower()) @@ -241,6 +244,9 @@ extern "C" { print("bool %(p)sserver_has_%(t)s_table_col_%(c)s(const struct ovsdb_idl *); " % { 'p': prefix, 't': tableName.lower(), 'c': columnName}) + print("const struct ovsdb_type *%(p)s%(t)s_%(c)s_server_type(const struct ovsdb_idl *); " % { + 'p': prefix, 't': tableName.lower(), + 'c': columnName}) print(''' bool %(p)sserver_has_%(t)s_table(const struct ovsdb_idl *); @@ -520,6 +526,12 @@ bool { return ovsdb_idl_server_has_column(idl, &%(s)s_col_%(c)s); } + +const struct ovsdb_type * +%(p)s%(t)s_%(c)s_server_type(const struct ovsdb_idl *idl) +{ + return ovsdb_idl_server_column_type(idl, &%(s)s_col_%(c)s); +} ''' % {'s': structName, 'p': prefix, 't': tableName.lower(), 'P': prefix.upper(), 'T': tableName.upper(), 'c': columnName}) diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at index 9f189f3f6..8dcc4c75f 100644 --- a/tests/ovsdb-idl.at +++ b/tests/ovsdb-idl.at @@ -2708,17 +2708,17 @@ unix:socket remote doesn't have table link2 unix:socket remote doesn't have table simple5 unix:socket remote doesn't have col irefmap in table simple5 unix:socket remote doesn't have col l2 in table link1 -unix:socket remote has col i in table link1 +unix:socket remote has col i in table link1, type: integer unix:socket remote doesn't have col id in table simple7 --- remote unix:socket done --- unix:socket2 remote has table simple unix:socket2 remote has table link1 unix:socket2 remote has table link2 unix:socket2 remote has table simple5 -unix:socket2 remote has col irefmap in table simple5 -unix:socket2 remote has col l2 in table link1 -unix:socket2 remote has col i in table link1 -unix:socket2 remote has col id in table simple7 +unix:socket2 remote has col irefmap in table simple5, type: map of (integer, uuid) pairs +unix:socket2 remote has col l2 in table link1, type: set of up to 1 uuids +unix:socket2 remote has col i in table link1, type: integer +unix:socket2 remote has col id in table simple7, type: string --- remote unix:socket2 done --- ], [stderr]) diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c index 52438e677..97ddc1d07 100644 --- a/tests/test-ovsdb.c +++ b/tests/test-ovsdb.c @@ -3600,20 +3600,37 @@ do_idl_table_column_check(struct ovs_cmdl_context *ctx) ctx->argv[r], has_table ? "has" : "doesn't have"); bool has_col = idltest_server_has_simple5_table_col_irefmap(idl); - printf("%s remote %s col irefmap in table simple5\n", - ctx->argv[r], has_col ? "has" : "doesn't have"); + const struct ovsdb_type *type = + idltest_simple5_irefmap_server_type(idl); + char *type_s = type ? ovsdb_type_to_english(type) : NULL; + printf("%s remote %s col irefmap in table simple5%s%s\n", + ctx->argv[r], has_col ? "has" : "doesn't have", + type ? ", type: " : "", type ? type_s : ""); + free(type_s); has_col = idltest_server_has_link1_table_col_l2(idl); - printf("%s remote %s col l2 in table link1\n", - ctx->argv[r], has_col ? "has" : "doesn't have"); + type = idltest_link1_l2_server_type(idl); + type_s = type ? ovsdb_type_to_english(type) : NULL; + printf("%s remote %s col l2 in table link1%s%s\n", + ctx->argv[r], has_col ? "has" : "doesn't have", + type ? ", type: " : "", type ? type_s : ""); + free(type_s); has_col = idltest_server_has_link1_table_col_i(idl); - printf("%s remote %s col i in table link1\n", - ctx->argv[r], has_col ? "has" : "doesn't have"); + type = idltest_link1_i_server_type(idl); + type_s = type ? ovsdb_type_to_english(type) : NULL; + printf("%s remote %s col i in table link1%s%s\n", + ctx->argv[r], has_col ? "has" : "doesn't have", + type ? ", type: " : "", type ? type_s : ""); + free(type_s); has_col = idltest_server_has_simple7_table_col_id(idl); - printf("%s remote %s col id in table simple7\n", - ctx->argv[r], has_col ? "has" : "doesn't have"); + type = idltest_simple7_id_server_type(idl); + type_s = type ? ovsdb_type_to_english(type) : NULL; + printf("%s remote %s col id in table simple7%s%s\n", + ctx->argv[r], has_col ? "has" : "doesn't have", + type ? ", type: " : "", type ? type_s : ""); + free(type_s); printf("--- remote %s done ---\n", ctx->argv[r]); } -- 2.49.0 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
