On 4/16/26 5:53 PM, Lorenzo Bianconi wrote:
> Filter outgoing monitor requests in the IDL layer based on the
> db-schema received from the server, skipping tables and columns not
> available in the schema.
> To support this, store the user-provided monitor condition in pre-JSON
> format in ovsdb_idl_table when ovsdb_idl_set_condition() is called,
> allowing ovsdb_idl_compose_monitor_request() to build monitor
> conditional requests that only include tables and columns present in
> the db-schema.
>
> Reported-at: https://issues.redhat.com/browse/FDP-3114
> Signed-off-by: Lorenzo Bianconi <[email protected]>
> ---
Hi, Lorenzo. Thanks for the update and sorry for delay.
See some comments below.
> Changes in v4:
> - Filter monitor requests in ovsdb_idl_set_condition__()
> - Cosmetics
> - Move ovsdb_cs_db_sync_condition() in ovsdb_cs_send_monitor_request() to
> properly update ack_cond json struct
> - Store original user monitor request in req_cond in ovsdb_idl_set_condition()
> - Add more uni-tests
> - Add more comments
> Changes in v3:
> - Fix seqno reporting when the filtered condition is empty.
> Changes in v2:
> - Add missing unit-test.
> - squash with patch 'ovsdb: Add ovsdb_cs_clear_condition routine to remove
> stable ovsdb_cs_db_table entries.'.
> - fix the error condition reported by Ilya.
> - Remove unnecessary ovsdb_cs_db_sync_condition() in
> ovsdb_cs_send_monitor_request().
> - cosmetics.
> ---
> lib/ovsdb-cs.c | 44 +++++++++++-----
> lib/ovsdb-cs.h | 1 +
> lib/ovsdb-idl-provider.h | 3 ++
> lib/ovsdb-idl.c | 109 +++++++++++++++++++++++++++++++++++++--
> tests/ovsdb-idl.at | 19 ++++++-
> tests/test-ovsdb.c | 19 ++++++-
> 6 files changed, 176 insertions(+), 19 deletions(-)
>
> diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c
> index df33a835d..a88eedb77 100644
> --- a/lib/ovsdb-cs.c
> +++ b/lib/ovsdb-cs.c
> @@ -412,12 +412,6 @@ ovsdb_cs_retry_at(struct ovsdb_cs *cs, const char *where)
> static void
> ovsdb_cs_restart_fsm(struct ovsdb_cs *cs)
> {
> - /* Resync data DB table conditions to avoid missing updates due to
> - * conditions that were in flight or changed locally while the connection
> - * was down.
> - */
> - ovsdb_cs_db_sync_condition(&cs->data);
> -
> ovsdb_cs_send_schema_request(cs, &cs->server);
> ovsdb_cs_transition(cs, CS_S_SERVER_SCHEMA_REQUESTED);
> cs->data.monitor_version = 0;
> @@ -912,17 +906,41 @@ ovsdb_cs_db_get_table(struct ovsdb_cs_db *db, const
> char *table)
> return t;
> }
>
> +static void
> +ovsdb_cs_db_destroy_table(struct ovsdb_cs_db_table *table,
> + struct ovsdb_cs_db *db)
> +{
> + json_destroy(table->ack_cond);
> + json_destroy(table->req_cond);
> + json_destroy(table->new_cond);
> + hmap_remove(&db->tables, &table->hmap_node);
> + free(table->name);
> + free(table);
> +}
> +
> +/* Destroy a given ovsdb_cs_db_table according to the table name. */
> +void
> +ovsdb_cs_clear_condition(struct ovsdb_cs *cs, const char *table)
> +{
> + uint32_t hash = hash_string(table, 0);
> + struct ovsdb_cs_db *db = &cs->data;
> +
> + struct ovsdb_cs_db_table *t;
> + HMAP_FOR_EACH_WITH_HASH (t, hmap_node, hash, &db->tables) {
> + if (!strcmp(t->name, table)) {
> + ovsdb_cs_db_destroy_table(t, db);
> + db->last_id = UUID_ZERO;
I think, we still need some test coverage for this to make sure that when
we're re-connecting to the same server that is missing some tables, we do
not drop the last_id and so we do not re-dowload the whole database.
> + return;
> + }
> + }
> +}
> +
> static void
> ovsdb_cs_db_destroy_tables(struct ovsdb_cs_db *db)
> {
> struct ovsdb_cs_db_table *table;
> HMAP_FOR_EACH_SAFE (table, hmap_node, &db->tables) {
> - json_destroy(table->ack_cond);
> - json_destroy(table->req_cond);
> - json_destroy(table->new_cond);
> - hmap_remove(&db->tables, &table->hmap_node);
> - free(table->name);
> - free(table);
> + ovsdb_cs_db_destroy_table(table, db);
> }
> hmap_destroy(&db->tables);
> }
> @@ -1511,6 +1529,8 @@ ovsdb_cs_send_monitor_request(struct ovsdb_cs *cs,
> struct ovsdb_cs_db *db,
> /* XXX handle failure */
> ovs_assert(mrs->type == JSON_OBJECT);
>
> + ovsdb_cs_db_sync_condition(db);
> +
> if (version > 1) {
> struct ovsdb_cs_db_table *table;
> HMAP_FOR_EACH (table, hmap_node, &db->tables) {
> diff --git a/lib/ovsdb-cs.h b/lib/ovsdb-cs.h
> index bcc3dcd71..0ac691352 100644
> --- a/lib/ovsdb-cs.h
> +++ b/lib/ovsdb-cs.h
> @@ -144,6 +144,7 @@ void ovsdb_cs_set_probe_interval(const struct ovsdb_cs *,
> int probe_interval);
> unsigned int ovsdb_cs_set_condition(struct ovsdb_cs *, const char *table,
> const struct json *condition);
> unsigned int ovsdb_cs_get_condition_seqno(const struct ovsdb_cs *);
> +void ovsdb_cs_clear_condition(struct ovsdb_cs *, const char *table);
>
> /* Database change awareness. */
> void ovsdb_cs_set_db_change_aware(struct ovsdb_cs *, bool
> set_db_change_aware);
> diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h
> index 6cf32fb50..9244bbcd5 100644
> --- a/lib/ovsdb-idl-provider.h
> +++ b/lib/ovsdb-idl-provider.h
> @@ -130,6 +130,9 @@ struct ovsdb_idl_table {
> * or not. */
> struct ovs_list indexes; /* Contains "struct ovsdb_idl_index"s */
> struct ovs_list track_list; /* Tracked rows (ovsdb_idl_row.track_node).
> */
> +
> + struct ovsdb_idl_condition req_cond; /* User requested monitor
> + * condition. */
This should probably be a pointer. More on that below.
> };
>
> struct ovsdb_idl_class {
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index fe90deda7..bc242f054 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -99,6 +99,8 @@ struct ovsdb_idl {
> struct ovs_list rows_to_reparse; /* Stores rows that might need to be
> * re-parsed due to insertion of a
> * referenced row. */
> + bool server_schema_received; /* Set to true when the IDL has received
> + * the DB schema from the servers. */
*server
> };
>
> static struct ovsdb_cs_ops ovsdb_idl_cs_ops;
> @@ -205,6 +207,18 @@ static void ovsdb_idl_add_to_indexes(const struct
> ovsdb_idl_row *);
> static void ovsdb_idl_remove_from_indexes(const struct ovsdb_idl_row *);
> static int ovsdb_idl_try_commit_loop_txn(struct ovsdb_idl_loop *loop,
> bool *may_need_wakeup);
> +static void ovsdb_idl_condition_clone(struct ovsdb_idl_condition *dest,
> + const struct ovsdb_idl_condition *);
> +static void ovsdb_idl_create_req_condition(
> + struct ovsdb_idl *,
> + const struct ovsdb_idl_table_class *,
> + const struct ovsdb_idl_condition *);
> +static void ovsdb_idl_destroy_req_condition(struct ovsdb_idl_table *);
> +static bool ovsdb_idl_condition_is_set(struct ovsdb_idl_condition *);
> +static unsigned int ovsdb_idl_set_condition__(
> + struct ovsdb_idl *,
> + const struct ovsdb_idl_table_class *,
> + const struct ovsdb_idl_condition *);
>
> static void add_tracked_change_for_references(struct ovsdb_idl_row *);
>
> @@ -266,6 +280,7 @@ ovsdb_idl_create_unconnected(const struct ovsdb_idl_class
> *class,
> .txn = NULL,
> .outstanding_txns = HMAP_INITIALIZER(&idl->outstanding_txns),
> .verify_write_only = false,
> + .server_schema_received = false,
> .deleted_untracked_rows
> = OVS_LIST_INITIALIZER(&idl->deleted_untracked_rows),
> .rows_to_reparse
> @@ -298,6 +313,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;
> + ovsdb_idl_condition_init(&table->req_cond);
> shash_init(&table->schema_columns);
> }
>
> @@ -372,6 +388,7 @@ ovsdb_idl_destroy(struct ovsdb_idl *idl)
>
> ovsdb_idl_schema_columns_clear(&table->schema_columns);
> shash_destroy(&table->schema_columns);
> + ovsdb_idl_destroy_req_condition(table);
>
> hmap_destroy(&table->rows);
> free(table->modes);
> @@ -467,6 +484,7 @@ ovsdb_idl_run(struct ovsdb_idl *idl)
> LIST_FOR_EACH_POP (event, list_node, &events) {
> switch (event->type) {
> case OVSDB_CS_EVENT_TYPE_RECONNECT:
> + idl->server_schema_received = false;
We're not clearing in_server_schema for tables, why should we clear the
server_schema_received?
> ovsdb_idl_txn_abort_all(idl);
> break;
>
> @@ -788,6 +806,7 @@ ovsdb_idl_compose_monitor_request(const struct json
> *schema_json, void *idl_)
> struct shash *schema = ovsdb_cs_parse_schema(schema_json);
> struct json *monitor_requests = json_object_create();
>
> + idl->server_schema_received = true;
> for (size_t i = 0; i < idl->class_->n_tables; i++) {
> struct ovsdb_idl_table *table = &idl->tables[i];
> const struct ovsdb_idl_table_class *tc = table->class_;
> @@ -842,6 +861,7 @@ ovsdb_idl_compose_monitor_request(const struct json
> *schema_json, void *idl_)
> idl->class_->database, table->class_->name);
> json_destroy(columns);
> table->in_server_schema = false;
> + ovsdb_cs_clear_condition(idl->cs, table->class_->name);
> continue;
> } else if (schema && table_schema) {
> table->in_server_schema = true;
> @@ -852,6 +872,14 @@ ovsdb_idl_compose_monitor_request(const struct json
> *schema_json, void *idl_)
> json_object_put(monitor_requests, tc->name,
> json_array_create_1(monitor_request));
> }
> +
> + if (!table->in_server_schema) {
> + ovsdb_cs_clear_condition(idl->cs, table->class_->name);
> + } else if (ovsdb_idl_condition_is_set(&table->req_cond)) {
The current version of ovsdb_idl_condition_is_set() will not pass the [false]
condition here even if the user requested it. So, I'm not sure we can rely
on the content of the 'req_cond'. It should likely be a pointer that gets
allocated on user requests, so we can check it here. This would also justify
the existence of the ovsdb_idl_destroy_req_condition(), which is currently
an unnecessary wrapper around ovsdb_idl_condition_destroy().
> + /* Update the monitor condition request according to the
> + * db schema. */
> + ovsdb_idl_set_condition__(idl, tc, &table->req_cond);
> + }
> }
> ovsdb_cs_free_schema(schema);
>
> @@ -1156,6 +1184,44 @@ ovsdb_idl_condition_add_clause__(struct
> ovsdb_idl_condition *condition,
> hmap_insert(&condition->clauses, &clause->hmap_node, hash);
> }
>
> +static void
> +ovsdb_idl_destroy_req_condition(struct ovsdb_idl_table *table)
> +{
> + ovsdb_idl_condition_destroy(&table->req_cond);
> +}
> +
> +static bool
> +ovsdb_idl_condition_is_set(struct ovsdb_idl_condition *condition)
> +{
> + return !hmap_is_empty(&condition->clauses) || condition->is_true;
> +}
> +
> +static void
> +ovsdb_idl_condition_clone(struct ovsdb_idl_condition *dest,
> + const struct ovsdb_idl_condition *source)
> +{
> + ovsdb_idl_condition_init(dest);
> +
> + struct ovsdb_idl_clause *clause;
> + HMAP_FOR_EACH (clause, hmap_node, &source->clauses) {
> + ovsdb_idl_condition_add_clause__(dest, clause,
> clause->hmap_node.hash);
> + }
> + dest->is_true = source->is_true;
> +}
> +
> +static void
> +ovsdb_idl_create_req_condition(struct ovsdb_idl *idl,
> + const struct ovsdb_idl_table_class *tc,
> + const struct ovsdb_idl_condition *condition)
> +{
> + struct ovsdb_idl_table *table = shash_find_data(&idl->table_by_name,
> + tc->name);
> + if (table) {
> + ovsdb_idl_destroy_req_condition(table);
> + ovsdb_idl_condition_clone(&table->req_cond, condition);
> + }
> +}
> +
> /* Adds a clause to the condition for replicating the table with class 'tc'
> in
> * 'idl'.
> *
> @@ -1234,6 +1300,43 @@ ovsdb_idl_condition_to_json(const struct
> ovsdb_idl_condition *cnd)
> return json_array_create(clauses, n);
> }
>
> +static unsigned int
> +ovsdb_idl_set_condition__(struct ovsdb_idl *idl,
> + const struct ovsdb_idl_table_class *tc,
> + const struct ovsdb_idl_condition *condition)
> +{
> + struct ovsdb_idl_condition filter_cond =
> + OVSDB_IDL_CONDITION_INIT(&filter_cond);
> + struct json *cond_json;
> + unsigned int seqno;
> +
> + if (!idl->server_schema_received || hmap_is_empty(&condition->clauses)) {
The hmap_is_empty() check should likely be moved after the in_server_schema
check. Otherwise, it would still be possble to get a true/false condition
for a non-existing table into the CS layer, which we're trying to avoid here.
The server_schema_received check should stay here though.
> + goto out;
> + }
> +
> + struct ovsdb_idl_table *t = ovsdb_idl_table_from_class(idl, tc);
> + if (!t || !t->in_server_schema) {
> + return ovsdb_idl_get_condition_seqno(idl);
> + }
> +
> + struct ovsdb_idl_clause *clause;
> + HMAP_FOR_EACH (clause, hmap_node, &condition->clauses) {
> + if (ovsdb_idl_server_has_column(idl, clause->column)) {
> + uint32_t hash = ovsdb_idl_clause_hash(clause);
> + ovsdb_idl_condition_add_clause__(&filter_cond, clause, hash);
clause->hmap_node.hash ?
> + }
> + }
> + condition = &filter_cond;
> +out:
> + cond_json = ovsdb_idl_condition_to_json(condition);
> + seqno = ovsdb_cs_set_condition(idl->cs, tc->name, cond_json);
> + json_destroy(cond_json);
> +
> + ovsdb_idl_condition_destroy(&filter_cond);
> +
> + return seqno;
> +}
> +
> /* Sets the replication condition for 'tc' in 'idl' to 'condition' and
> * arranges to send the new condition to the database server.
> *
> @@ -1245,10 +1348,8 @@ ovsdb_idl_set_condition(struct ovsdb_idl *idl,
> const struct ovsdb_idl_table_class *tc,
> const struct ovsdb_idl_condition *condition)
> {
> - struct json *cond_json = ovsdb_idl_condition_to_json(condition);
> - unsigned int seqno = ovsdb_cs_set_condition(idl->cs, tc->name,
> cond_json);
> - json_destroy(cond_json);
> - return seqno;
> + ovsdb_idl_create_req_condition(idl, tc, condition);
> + return ovsdb_idl_set_condition__(idl, tc, condition);
> }
>
> /* Turns off OVSDB_IDL_ALERT and OVSDB_IDL_TRACK for 'column' in 'idl'.
> diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
> index 728d761d4..af682aabf 100644
> --- a/tests/ovsdb-idl.at
> +++ b/tests/ovsdb-idl.at
> @@ -2701,7 +2701,7 @@ dnl idltest2.ovsschema and outputs the presence of
> tables and columns.
> dnl And then it connectes to the server with the schema idltest.ovsschema
> dnl and does the same.
> AT_CHECK([test-ovsdb '-vPATTERN:console:test-ovsdb|%c|%m' -vjsonrpc -t10 dnl
> - idl-table-column-check unix:socket unix:socket2], [0], [dnl
> + idl-table-column-check unix:socket unix:socket2
> unix:socket], [0], [dnl
> unix:socket remote has table simple
> unix:socket remote has table link1
> unix:socket remote doesn't have table link2
> @@ -2720,8 +2720,25 @@ 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 ---
> +unix:socket remote has table simple
> +unix:socket remote has table link1
> +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, type: integer
> +unix:socket remote doesn't have col id in table simple7
> +--- remote unix:socket done ---
> ], [stderr])
>
> +# Check we do not have any errors related to conditional monitoring.
> +AT_CHECK([grep -q "received error, error={\"details\":\"No column l2 in
> table link1.\"" stderr], [1])
> +AT_CHECK([grep -q "received error, error={\"details\":\"no table named
> link2\"" stderr], [1])
> +
> +# Check the monitor_cond_since request has properly formatted conditions
> connecting to the idltest2 server.
> +AT_CHECK([grep monitor_cond_since stderr | grep -qF
> '"l2","==",@<:@"uuid","00000000-0000-0000-0000-000000000000"@:>@'])
> +AT_CHECK([grep monitor_cond_since stderr | grep -qF '"i","==",1'])
Pleas, wrap the lines, so they are not that long.
> +
> OVSDB_SERVER_SHUTDOWN
> AT_CLEANUP
>
> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
> index 95290ae78..71c28cf25 100644
> --- a/tests/test-ovsdb.c
> +++ b/tests/test-ovsdb.c
> @@ -3583,6 +3583,18 @@ do_idl_table_column_check(struct ovs_cmdl_context *ctx)
> ovsdb_idl_omit(idl, &idltest_link1_col_i);
> ovsdb_idl_omit(idl, &idltest_simple7_col_id);
> ovsdb_idl_set_leader_only(idl, false);
> +
> + struct ovsdb_idl_condition cond_link1 =
> + OVSDB_IDL_CONDITION_INIT(&cond_link1);
> + struct uuid uuid = UUID_ZERO;
A more recognizeable UUID might be better.
> + idltest_link1_add_clause_l2(&cond_link1, OVSDB_F_EQ, &uuid);
> + idltest_link1_set_condition(idl, &cond_link1);
> +
> + struct ovsdb_idl_condition cond_link2 =
> + OVSDB_IDL_CONDITION_INIT(&cond_link2);
> + idltest_link2_add_clause_i(&cond_link2, OVSDB_F_EQ, 1);
> + idltest_link2_set_condition(idl, &cond_link2);
> +
> struct stream *stream;
>
> error = stream_open_block(jsonrpc_stream_open(ctx->argv[1], &stream,
> @@ -3592,7 +3604,7 @@ do_idl_table_column_check(struct ovs_cmdl_context *ctx)
> }
> rpc = jsonrpc_open(stream);
>
> - for (int r = 1; r <= 2; r++) {
> + for (int r = 1; r <= 3; r++) {
> ovsdb_idl_set_remote(idl, ctx->argv[r], true);
> ovsdb_idl_force_reconnect(idl);
>
> @@ -3664,6 +3676,9 @@ do_idl_table_column_check(struct ovs_cmdl_context *ctx)
> printf("--- remote %s done ---\n", ctx->argv[r]);
> }
>
> + ovsdb_idl_condition_destroy(&cond_link1);
> + ovsdb_idl_condition_destroy(&cond_link2);
> +
> jsonrpc_close(rpc);
> ovsdb_idl_destroy(idl);
> }
> @@ -3706,7 +3721,7 @@ 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, 2, 2,
> + { "idl-table-column-check", NULL, 3, 3,
> do_idl_table_column_check, OVS_RO },
> { "help", NULL, 0, INT_MAX, do_help, OVS_RO },
> { NULL, NULL, 0, 0, NULL, OVS_RO },
I think, we still need a bit more coverage in general. For example, the
code path where the conditions are set with the set_cndition() after the
successfull connection is not checked. We should also check the sequence
numbers retuned by these calls.
Feel free to add a new commands to the test-ovsdb, if needed.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
