On 3/3/26 6:08 PM, Lorenzo Bianconi wrote:
> Introduce idl_connect_notifier callback in idl codebase in order to notify IDL
> consumer (e.g. ovn-controller) when the connection to remote db has been
> completed and db schema has been downloaded.
> ovn-controller requires this info to properly manage condition monitoring for
> tables that are not present in old db schemas and keep backward compatibility.
>
> Reported-at: https://issues.redhat.com/browse/FDP-3114
> Signed-off-by: Lorenzo Bianconi <[email protected]>
> ---
> lib/ovsdb-cs.c | 10 ++++------
> lib/ovsdb-idl.c | 16 ++++++++++++++++
> lib/ovsdb-idl.h | 3 +++
> 3 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c
> index df33a835d..2ddda6837 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;
> @@ -1511,6 +1505,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
* consumer
> + * conditions in compose_monitor_requests callback. */
> + ovsdb_cs_db_sync_condition(&cs->data);
> +
> if (version > 1) {
> struct ovsdb_cs_db_table *table;
> HMAP_FOR_EACH (table, hmap_node, &db->tables) {
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index d8094458d..e8e9141d2 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -99,6 +99,9 @@ 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. */
> + void (*idl_connect_notifier)(void *idl); /* callback to notify consumer
Comments should start with a capital letter.
> + * the idl connects to the
> remote
> + * db successfully. */
> };
>
> static struct ovsdb_cs_ops ovsdb_idl_cs_ops;
> @@ -304,6 +307,13 @@ ovsdb_idl_create_unconnected(const struct
> ovsdb_idl_class *class,
> return idl;
> }
>
> +void
> +ovsdb_idl_set_notifier(struct ovsdb_idl *idl,
> + void (*connect_notifier)(void *idl_))
> +{
> + idl->idl_connect_notifier = connect_notifier;
> +}
> +
> /* Changes the remote and creates a new session.
> *
> * If 'retry' is true, the connection to the remote will automatically retry
> @@ -853,8 +863,14 @@ ovsdb_idl_compose_monitor_request(const struct json
> *schema_json, void *idl_)
> json_array_create_1(monitor_request));
> }
> }
> +
> ovsdb_cs_free_schema(schema);
>
> + if (idl->idl_connect_notifier) {
> + /* Notify consumers the idl connects to remote db successfully. */
> + idl->idl_connect_notifier(idl);
> + }
Hi, Lorenzo. thanks for working on this!
I was thinking more about how we could make it simpler and more generic, so the
application doesn't have to think much about which tables and columns are
present
and which are not. The IDL layer knows and checks the existence of them while
constructing the monitor request and the cs layer only deals with json and
doesn't
really know much about the database structure. At the same time only the cs
layer
can easily handle all the condition states across reconnections, which is
problematic if we want to filter them.
Moving the condition handling to ovsdb-idl would be hard and practically mean a
lot
more callbacks from cs to idl, which is not good. But could we maybe store the
latest requested conditions from the user in their original pre-JSON form in
the IDL
layer and then construct a proper JSON without the missing tables/columns and
call
ovsdb_cs_set_condition() here instead of making the callback to an application
to do
the same thing? It's logically should be exactly the same workflow, but we
handle
the filtering internally instead of asking the user to do that.
What do you think?
CC: Dumitru
Note: ovsdb_idl_set_condition() will still need to call ovsdb_cs_set_condition()
right away, since conditions can be changed not only during the monitor request.
But if we integrate filtering into ovsdb_idl_condition_to_json(), then it should
work just fine. And it's still OK to set them unfiltered before we know the
schema, since monitor request callback will filter them later. And application
should not need to care about missing tables/columns at all while setting
conditions in this case.
Best regards, Ilya Maximets.
> +
> return monitor_requests;
> }
>
> diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h
> index f6060e324..dbf99e57d 100644
> --- a/lib/ovsdb-idl.h
> +++ b/lib/ovsdb-idl.h
> @@ -502,6 +502,9 @@ void ovsdb_idl_cursor_next_eq(struct ovsdb_idl_cursor *);
>
> struct ovsdb_idl_row *ovsdb_idl_cursor_data(struct ovsdb_idl_cursor *);
>
> +void ovsdb_idl_set_notifier(struct ovsdb_idl *idl,
> + void (*connect_notifier)(void *idl_));
> +
> #ifdef __cplusplus
> }
> #endif
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
