On 4/8/26 10:23 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]>
> ---
> 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 | 51 ++++++++++++++----
> lib/ovsdb-cs.h | 2 +
> lib/ovsdb-idl-provider.h | 3 ++
> lib/ovsdb-idl.c | 110 +++++++++++++++++++++++++++++++++++++--
> tests/ovsdb-idl.at | 3 ++
> tests/test-ovsdb.c | 8 +++
> 6 files changed, 165 insertions(+), 12 deletions(-)
>
> diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c
> index df33a835d..93f59492e 100644
> --- a/lib/ovsdb-cs.c
> +++ b/lib/ovsdb-cs.c
> @@ -663,6 +663,18 @@ ovsdb_cs_wait(struct ovsdb_cs *cs)
>
> /* Network connection. */
>
> +bool
> +ovsdb_cs_is_remote_changed(struct ovsdb_cs *cs, const char *remote)
> +{
> + if (cs &&
> + ((remote != NULL) != (cs->remote != NULL) ||
> + (remote && cs->remote && strcmp(remote, cs->remote)))) {
> + return true;
> + }
> +
> + return false;
> +}
> +
> /* Changes the remote and creates a new session. Keeps existing connection
> * if current remote is still valid.
> *
> @@ -671,9 +683,7 @@ ovsdb_cs_wait(struct ovsdb_cs *cs)
> void
> ovsdb_cs_set_remote(struct ovsdb_cs *cs, const char *remote, bool retry)
> {
> - if (cs
> - && ((remote != NULL) != (cs->remote != NULL)
> - || (remote && cs->remote && strcmp(remote, cs->remote)))) {
> + if (ovsdb_cs_is_remote_changed(cs, remote)) {
> struct jsonrpc *rpc = NULL;
>
> /* Close the old session, if any. */
> @@ -912,17 +922,40 @@ 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)
> +{
> + 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);
We should probbaly clear the last_id here, so the monitor reply clears
the data from idl for the table that now doesn't exist. But need to be
careful and not actually clear the idl->server_schema_received or we
risk to break the monitor_cond_since functionality whenever the client
connects to an older database.
> + return;
> + }
> + }
> +}
> +
> 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);
> }
> diff --git a/lib/ovsdb-cs.h b/lib/ovsdb-cs.h
> index bcc3dcd71..a46b806c0 100644
> --- a/lib/ovsdb-cs.h
> +++ b/lib/ovsdb-cs.h
> @@ -122,6 +122,7 @@ void ovsdb_cs_run(struct ovsdb_cs *, struct ovs_list
> *events);
> void ovsdb_cs_wait(struct ovsdb_cs *);
>
> /* Network connection. */
> +bool ovsdb_cs_is_remote_changed(struct ovsdb_cs *, const char *remote);
> void ovsdb_cs_set_remote(struct ovsdb_cs *, const char *remote, bool retry);
>
> void ovsdb_cs_enable_reconnect(struct ovsdb_cs *);
> @@ -144,6 +145,7 @@ 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);
>
> /* 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..9244bbcd5 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..6ad0eb36d 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -99,6 +99,7 @@ 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;
Need to add a comment here.
> };
>
> static struct ovsdb_cs_ops ovsdb_idl_cs_ops;
> @@ -205,6 +206,19 @@ 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 *source);
> +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 bool ovsdb_idl_condition_is_set(struct ovsdb_idl_condition *);
> +static unsigned int
> +ovsdb_idl_set_condition__(struct ovsdb_idl *,
> + const struct ovsdb_idl_table_class *,
> + const struct ovsdb_idl_condition *);
ovsdb_idl_condition_clone, ovsdb_idl_create_req_condition and the
ovsdb_idl_set_condition__ should not be on new lines, as these are
prototypes and not implementations.
>
> static void add_tracked_change_for_references(struct ovsdb_idl_row *);
>
> @@ -266,6 +280,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 +313,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;
> + ovsdb_idl_condition_init(&table->req_cond);
> shash_init(&table->schema_columns);
> }
>
> @@ -311,6 +327,9 @@ ovsdb_idl_create_unconnected(const struct ovsdb_idl_class
> *class,
> void
> ovsdb_idl_set_remote(struct ovsdb_idl *idl, const char *remote, bool retry)
> {
> + if (ovsdb_cs_is_remote_changed(idl->cs, remote)) {
> + idl->server_schema_received = false;
> + }
I don't think this is needed. And if it was needed, then we would probbalyclear
while receiving the RECONNECT event instead. We're not clearing
'in_server_schema'
fileds, so should not clear this one as well. If the reconnection happens we
will
be seding a new monitor request that will get the schema, so everything should
work
fine without setting to false on reconnections or remote changes.
> ovsdb_cs_set_remote(idl->cs, remote, retry);
> }
>
> @@ -372,6 +391,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);
> @@ -784,6 +804,7 @@ static struct json *
> ovsdb_idl_compose_monitor_request(const struct json *schema_json, void *idl_)
> {
> struct ovsdb_idl *idl = idl_;
> + idl->server_schema_received = true;
Should add an empty line between these two, or set the value closer to the loop.
>
> struct shash *schema = ovsdb_cs_parse_schema(schema_json);
> struct json *monitor_requests = json_object_create();
> @@ -842,6 +863,7 @@ 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) {
> table->in_server_schema = true;
> @@ -852,6 +874,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 (ovsdb_idl_condition_is_set(&table->req_cond)) {
> + /* Update the monitor condition request according to the
> + * db schema. */
> + ovsdb_idl_set_condition__(idl, tc, &table->req_cond);
Hmm. The ovsdb_idl_set_condition__() function doesn't filter, it uses the old
filtered condition stored in the table->req_cond that was created before we
received the new schema. So, we can still set conditions here for tables that
do not exist in on the server.
> + }
> }
> ovsdb_cs_free_schema(schema);
>
> @@ -1156,6 +1186,45 @@ 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);
> +}
> +
> +static bool
> +ovsdb_idl_condition_is_set(struct ovsdb_idl_condition *condition)
> +{
> + return !hmap_is_empty(&condition->clauses) || condition->is_true;
> +}
> +
> +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) {
> + uint32_t hash = ovsdb_idl_clause_hash(clause);
Can use a hash of the current hmap node.
> + ovsdb_idl_condition_add_clause__(dest, clause, 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);
> + 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 +1303,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 +1325,33 @@ 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);
> + struct ovsdb_idl_condition filter_cond =
> + OVSDB_IDL_CONDITION_INIT(&filter_cond);
> +
> + if (idl->server_schema_received && hmap_count(&condition->clauses)) {
> + struct ovsdb_idl_clause *clause;
> + HMAP_FOR_EACH (clause, hmap_node, &condition->clauses) {
> + struct ovsdb_idl_table *t =
> + ovsdb_idl_table_from_column(idl, clause->column);
This shoudld be done outside of the loop. We can get the table directly
from the table class - ovsdb_idl_table_from_class().
> + if (!t || !t->in_server_schema) {
> + return ovsdb_idl_get_condition_seqno(idl);
We can't just return here, we still need to save these conditions, otherwise
we will not send them on reconnection when the server schema changes.
> + }
> +
> + if (ovsdb_idl_server_has_column(idl, clause->column)) {
> + uint32_t hash = ovsdb_idl_clause_hash(clause);
> + ovsdb_idl_condition_add_clause__(&filter_cond, clause, hash);
> + }
> + }
> + condition = &filter_cond;
> + }
> +
> + unsigned int seqno = ovsdb_idl_set_condition__(idl, tc, condition);
> + ovsdb_idl_create_req_condition(idl, tc, &filter_cond);
We should be saving the original conditions, not the filtered ones, because
we need to filter every time we compose a new monitor request.
> +
> + if (hmap_count(&filter_cond.clauses)) {
> + ovsdb_idl_condition_destroy(&filter_cond);
> + }
> +
> return seqno;
> }
>
> diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
> index 728d761d4..194afef13 100644
> --- a/tests/ovsdb-idl.at
> +++ b/tests/ovsdb-idl.at
> @@ -2722,6 +2722,9 @@ unix:socket2 remote has col id in table simple7, type:
> string
> --- remote unix:socket2 done ---
> ], [stderr])
>
> +# Check we do not have any errors related to conditional monitoring.
> +AT_CHECK([grep -q "received error, error={\"details\":\"no table named
> link2\",\"error\":\"syntax error\"}" stderr], [1])
> +
> OVSDB_SERVER_SHUTDOWN
> AT_CLEANUP
I think we need way more tests for this functionality, including a case where
we connect to a server with an older schema that doesn't have some tables and
some columns in the tables that exist, then re-connect to a server with the new
schema that has all, then re-connect back to one with the older schema. The
client sets conditions for all the tables before connecting to the first server
and doesn't change them afterwards. We need to make sure that all the
conditions
are properly filtered every time the cilent re-connects without touching the
conditions. Ant another test where client sets conditions for tables and
columns that exist and for those that do not exist after the succesful
connection
to make sure the filtering works outside of the monitor request. Also maybe
some tests to make sure the monitor-cond-since is respected anf the last-id is
properly cleared when it needs to be and not cleared when it doesn't need to
be cleared.
THe commit message also could use a larger description of the problem and what
this change is trying to achieve.
Best regards, Ilya Maximets.
>
> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
> index 95290ae78..037a78743 100644
> --- a/tests/test-ovsdb.c
> +++ b/tests/test-ovsdb.c
> @@ -3596,6 +3596,12 @@ do_idl_table_column_check(struct ovs_cmdl_context *ctx)
> ovsdb_idl_set_remote(idl, ctx->argv[r], true);
> ovsdb_idl_force_reconnect(idl);
>
> + struct ovsdb_idl_condition cond_link2 =
> + OVSDB_IDL_CONDITION_INIT(&cond_link2);
> + idltest_link2_add_clause_i(&cond_link2, OVSDB_F_EQ, 1);
> +
> + idltest_link2_set_condition(idl, &cond_link2);
> +
> /* Wait for update. */
> for (;;) {
> ovsdb_idl_run(idl);
> @@ -3661,6 +3667,8 @@ do_idl_table_column_check(struct ovs_cmdl_context *ctx)
> type ? ", type: " : "", type ? type_s : "");
> free(type_s);
>
> + ovsdb_idl_condition_destroy(&cond_link2);
> +
> printf("--- remote %s done ---\n", ctx->argv[r]);
> }
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
