On 6/26/26 9:40 AM, Dumitru Ceara wrote:
> Hi Ilya,
>
> On 6/25/26 6:45 PM, Ilya Maximets wrote:
>> 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?
>>
>
> It's subjective, but as I said in the previous email, I can live with
> the way it is in this patch.
>
>>>
>>>> +{
>>>> + 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.
>>
>
> Thanks!
>
>>>
>>>> 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:
>>
>> 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,
Typo: shoudl -> should
>> + * 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
Typo: Cant' -> Can't
>> + * 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?
>>
>
> Sounds good to me, thanks!
Thanks, Dumitru and Lorenzo! I folded in the diff with the typos fixed up
and applied the change.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
