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:
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
