On Jun 01, Ilya Maximets wrote:
> 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.
Hi Ilya,
thx for the review.
>
[...]
> > +/* 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.
ack, I will add it in v5.
>
> > + 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.
ack, I will fix it in v5.
>
> > };
> >
> > 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?
ack, I will fix it in v5.
>
> > 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().
ack
>
> > + /* 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.
ack, I will fix it in v5.
>
> > + 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 ?
ack, I will fix it in v5.
>
> > + }
> > + }
> > + 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.
ack.
>
> > +
> > 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.
ack.
>
> > + 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.
ack, I will add them in v5.
Regards,
Lorenzo
>
> 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
