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,
> + * 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?
>
Sounds good to me, thanks!
Regards,
Dumitru
> Best regards, Ilya Maximets.
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
