On 3/12/26 4:01 PM, Lorenzo Bianconi wrote:
> Store the user monitor condition request in pre-json format in
> ovsdb_idl_table in order to use it in ovsdb_idl_compose_monitor_request
> routine and create a monitor condition request based on the
> tables/columns available in db-schema.
>
> Reported-at: https://issues.redhat.com/browse/FDP-3114
> Signed-off-by: Lorenzo Bianconi <[email protected]>
> ---
Hi Lorenzo,
Thanks for the patch!
Would it be an option to update the existing "idl table and column
presence check" test in ovsdb-idl.at to also check the monitor condition
requests?
I didn't try it out but maybe we can set an IDL condition on the "link2"
table in test-ovsdb's do_idl_table_column_check() and make sure the
client doesn't monitor "link2" at all for the first remote and that it
doesn't monitor "link2" without a condition being sent in the original
monitor request when it connects to the second remote?
> lib/ovsdb-cs.c | 4 +++
> lib/ovsdb-idl-provider.h | 3 ++
> lib/ovsdb-idl.c | 64 ++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 68 insertions(+), 3 deletions(-)
>
> diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c
> index df33a835d..dd96f0930 100644
> --- a/lib/ovsdb-cs.c
> +++ b/lib/ovsdb-cs.c
> @@ -1511,6 +1511,10 @@ ovsdb_cs_send_monitor_request(struct ovsdb_cs *cs,
> struct ovsdb_cs_db *db,
> /* XXX handle failure */
> ovs_assert(mrs->type == JSON_OBJECT);
>
> + /* We need to resync the condition since the IDL cosumer can update
> monitor
Typo: "cosumer"
> + * condition requests in compose_monitor_requests. */
> + ovsdb_cs_db_sync_condition(&cs->data);
I don't really follow. ovsdb_cs_db_sync_condition(), as mentioned by
its comment, is used to resync the conditions in the CS layer when the
state machine is restarted. That's not the case here.
Why is it even needed? Sure, the IDL condition might have been updated
in the compose_monitor_requests callback, but that one uses
ovsdb_idl_set_condition__() which calls into ovsdb_cs_set_condition().
> +
> if (version > 1) {
> struct ovsdb_cs_db_table *table;
> HMAP_FOR_EACH (table, hmap_node, &db->tables) {
> diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h
> index 6cf32fb50..c75aaeee1 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 *condition; /* User monitor condition request
> + * cache. */
Maybe just call the field 'req_cond' and change the comment to "User
requested monitor condition."?
Also, why a pointer? Why not a direct "struct ovsdb_idl_condition
req_cond;" that can be initialized with OVSDB_IDL_CONDITION_INIT()?
> };
>
> struct ovsdb_idl_class {
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index fe90deda7..beb2c94b5 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -205,6 +205,15 @@ 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_create_condition_cache(struct ovsdb_idl *,
> + const struct ovsdb_idl_table_class *,
> + const struct ovsdb_idl_condition *);
> +static void ovsdb_idl_destroy_condition_cache(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 *);
>
> @@ -298,6 +307,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->condition = NULL;
> shash_init(&table->schema_columns);
> }
>
> @@ -372,6 +382,7 @@ ovsdb_idl_destroy(struct ovsdb_idl *idl)
>
> ovsdb_idl_schema_columns_clear(&table->schema_columns);
> shash_destroy(&table->schema_columns);
> + ovsdb_idl_destroy_condition_cache(table);
>
> hmap_destroy(&table->rows);
> free(table->modes);
> @@ -852,6 +863,12 @@ 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->condition && table->in_server_schema) {
> + /* Update the monitor condition request according to the
> + * db schema. */
> + ovsdb_idl_set_condition__(idl, tc, table->condition);
> + }
> }
> ovsdb_cs_free_schema(schema);
>
> @@ -1156,6 +1173,37 @@ ovsdb_idl_condition_add_clause__(struct
> ovsdb_idl_condition *condition,
> hmap_insert(&condition->clauses, &clause->hmap_node, hash);
> }
>
> +static void
> +ovsdb_idl_destroy_condition_cache(struct ovsdb_idl_table *table)
> +{
ovsdb_idl_condition_destroy() works just fine on NULL pointers.
> + if (table->condition) {
> + ovsdb_idl_condition_destroy(table->condition);
> + free(table->condition);
> + table->condition = NULL;
> + }
> +}
> +
> +static void
> +ovsdb_idl_create_condition_cache(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_condition_cache(table);
> + table->condition = xmalloc(sizeof *table->condition);
> + ovsdb_idl_condition_init(table->condition);
> +
> + struct ovsdb_idl_clause *clause;
> + HMAP_FOR_EACH (clause, hmap_node, &condition->clauses) {
> + uint32_t hash = ovsdb_idl_clause_hash(clause);
> + ovsdb_idl_condition_add_clause__(table->condition, clause, hash);
> + }
> + table->condition->is_true = condition->is_true;
> + }
> +}
> +
> /* Adds a clause to the condition for replicating the table with class 'tc'
> in
> * 'idl'.
> *
> @@ -1234,6 +1282,17 @@ 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 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;
> +}
> +
> /* Sets the replication condition for 'tc' in 'idl' to 'condition' and
> * arranges to send the new condition to the database server.
> *
> @@ -1245,9 +1304,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);
> + unsigned int seqno = ovsdb_idl_set_condition__(idl, tc, condition);
> + ovsdb_idl_create_condition_cache(idl, tc, condition);
I think what we need is a ovsdb_idl_condition_clone(dest, source) helper
function. We're just storing a copy of the requested condition, while
technically it's a cache I think "clone" might be the better word.
So I think I'd just add a ovsdb_idl_condition_clone() function to do a
deep copy of a struct ovsdb_idl_condition record.
> return seqno;
> }
>
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
