> On 6/23/26 2:56 PM, Dumitru Ceara wrote:
> > On 6/23/26 1:51 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, Ilya,
> >
> > I have a few minor comments down below. But I'd also be OK if this
> > exact version gets merged. I'll leave it up to you to decide if you
> > want to integrate my feedback or not.
> >
> > In any case:
> >
> > Acked-by: Dumitru Ceara <[email protected]>
> >
> > Thanks for all the work on this, it will make it a bit easier for
> > clients (OVN) to use the IDL IMO.
> >
> > Regards,
> > Dumitru
> >
> >> Changes in v6:
> >> - Rebase on top of ovs main branch.
> >> - cosmetics.
> >> Changes in v5:
> >> - Add self-test for last_id management in raft mode.
> >> - Use a pointer for req_cond instead of a struct.
> >> - Fix typos.
> >> - Do not reset server_schema_received in ovsdb_idl_run()
> >> - Move hmap_is_empty() after in_server_schema check in
> >> ovsdb_idl_set_condition__().
> >> - Add for coverage tests.
> >> 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 | 50 +++++++---
> >> lib/ovsdb-cs.h | 2 +
> >> lib/ovsdb-idl-provider.h | 3 +
> >> lib/ovsdb-idl.c | 111 ++++++++++++++++++++++-
> >> tests/ovsdb-idl.at | 142 ++++++++++++++++++++++++++++-
> >> tests/test-ovsdb.c | 191 ++++++++++++++++++++++++++++++++++++---
> >> 6 files changed, 468 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c
> >> index d96a97599..aaa1611c3 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,47 @@ 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)
> >
> > Nit: The function name reads a bit weird. The comment speaks about
> > destroying the table, which is what the code does but the function name
> > is about clearing the condition.
> >
> > Should we call this "ovsdb_cs_destroy_table()"?
>
> I don't think so. The cs_table construct is entirely internal to ovsdb-cs
> module. The fact that CS module chooses to implement condition clearing
> as destruction of the internal table is an implementation detail that IDL
> layer should not know about, as it deals with conditions: it sets them and
> it clears them.
>
> Does that make sense?
>
> >
> >> +{
> >> + 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;
> >> + return;
> >> + }
> >> + }
> >> +}
> >> +
> >> +void
> >> +ovsdb_cs_reset_last_id(struct ovsdb_cs *cs)
> >> +{
> >> + cs->data.last_id = UUID_ZERO;
> >> +}
> >> +
> >> 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);
> >> }
> >> @@ -1501,6 +1525,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..98c921cd8 100644
> >> --- a/lib/ovsdb-cs.h
> >> +++ b/lib/ovsdb-cs.h
> >> @@ -144,6 +144,8 @@ 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);
> >> +void ovsdb_cs_reset_last_id(struct ovsdb_cs *);
> >>
> >> /* 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..2e0ab3003 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. */
> >> };
> >>
> >> struct ovsdb_idl_class {
> >> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> >> index fe90deda7..8d85a23f1 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 server. */
> >> };
> >>
> >> static struct ovsdb_cs_ops ovsdb_idl_cs_ops;
> >> @@ -205,6 +207,17 @@ 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 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 +279,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 +312,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;
> >> + table->req_cond = NULL;
> >> shash_init(&table->schema_columns);
> >> }
> >>
> >> @@ -372,6 +387,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);
> >> @@ -788,6 +804,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,8 +859,12 @@ 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) {
> >> + if (!table->in_server_schema) {
> >> + ovsdb_cs_reset_last_id(idl->cs);
> >> + }
> >
> > I initially got confused here and thought that this would cause issues
> > and always reset the last_id every time the IDL client reconnects to the
> > server.
> >
> > But that's actually not true. This is correct because:
> > - when the client starts up (last_id is already 0), no tables are in the
> > server schema
> > - then the schema is received so the tables that are actually on the
> > server side get 'in_server_schema': false->true and last_id is "reset"
> > (it was already 0 so that's fine)
> > - only if we reconnect and the server has upgraded the schema (more
> > tables are added) we reset last_id.
> >
> > So it's fine but it makes me wonder if we should add a tiny comment here
> > for it.
>
> This is also a defensive clearing here as under normal circumstances the
> server supposed to drop the transaction history on upgrades. But we should
> not ask for it in the first place. We could add a comment for that.
>
> >
> >> table->in_server_schema = true;
> >> }
> >>
> >> @@ -852,6 +873,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 (table->req_cond) {
> >> + /* 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 +1185,41 @@ 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);
> >> + free(table->req_cond);
> >> + table->req_cond = NULL;
> >> +}
> >> +
> >> +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);
> >> + table->req_cond = xzalloc(sizeof *table->req_cond);
> >> + 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 +1298,47 @@ 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) {
> >> + goto out;
> >> + }
> >> +
> >> + struct ovsdb_idl_table *t = ovsdb_idl_table_from_class(idl, tc);
> >> + if (!t || !t->in_server_schema) {
> >
> > nit: We _don't_ leak anything from 'fiter_cond' here but it looks weird
> > that we don't destroy it. I'm not sure if we should change this or not,
> > I'm on the fence with this one. I'm not against leaving it as is though.
>
> We could assing seqno from the current one and goto the end to make it
> easier to follow and let the destruction the be executed.
>
> Though the 'goto out' is a little confusing name here, I'd change it to
> 'goto set_condition'. And add a new 'exit' label to jump from here.
>
> I can fold in something like this before applying the change if you and
> Lorenzo agree:
ack, I am fine with the below changes. Thanks.
Regards,
Lorenzo
>
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index 8d85a23f1..9500d5327 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -863,6 +863,10 @@ ovsdb_idl_compose_monitor_request(const struct json
> *schema_json, void *idl_)
> continue;
> } else if (schema && table_schema) {
> if (!table->in_server_schema) {
> + /* The server didn't have this table before and now it
> + * does. It shoudl've dropped the transaction history,
> + * but it's better if we do not ask for it in the first
> + * place, as our conditions could be out of sync. */
> ovsdb_cs_reset_last_id(idl->cs);
> }
> table->in_server_schema = true;
> @@ -1309,18 +1313,25 @@ ovsdb_idl_set_condition__(struct ovsdb_idl *idl,
> unsigned int seqno;
>
> if (!idl->server_schema_received) {
> - goto out;
> + /* Cant' filter yet - pass through. Will be filtered once we know
> + * the server schema. */
> + goto set_condition;
> }
>
> 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);
> + /* Not on the server, should not set. */
> + seqno = ovsdb_idl_get_condition_seqno(idl);
> + goto exit;
> }
>
> if (hmap_is_empty(&condition->clauses)) {
> - goto out;
> + /* Trivial condition and the table is present on the server. */
> + goto set_condition;
> }
>
> + /* Non-trivial condition for a table that is in the server schema.
> + * Filter out columns that are not. */
> struct ovsdb_idl_clause *clause;
> HMAP_FOR_EACH (clause, hmap_node, &condition->clauses) {
> if (ovsdb_idl_server_has_column(idl, clause->column)) {
> @@ -1329,13 +1340,14 @@ ovsdb_idl_set_condition__(struct ovsdb_idl *idl,
> }
> }
> condition = &filter_cond;
> -out:
> +
> +set_condition:
> cond_json = ovsdb_idl_condition_to_json(condition);
> seqno = ovsdb_cs_set_condition(idl->cs, tc->name, cond_json);
> json_destroy(cond_json);
>
> +exit:
> ovsdb_idl_condition_destroy(&filter_cond);
> -
> return seqno;
> }
>
> ---
>
> WDYT?
>
> Best regards, Ilya Maximets.
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
