On 6/22/26 9:49 PM, Ilya Maximets wrote:
> Looking at the 'monitor_cond_since, cluster disconnect' test scenario,
> the test creates a new connection without monitoring anything (false),
> then it updates conditions to monitor rows with i == 2, then i == 1,
> then it reconnects and checks that the right data is monitored.
>
> The problem is that it doesn't wait for 'i == 2' to be acknowledged
> before reconnecting. So, there are two cases here in practice:
>
> new req ack
> 1. - set_conditions(i == 2) i == 2
> - minitor_cond_change > i == 2
> - update3
> - ack > i == 2
> - set_conditions(i == 1) i == 1 i == 2
> - monitor_cond_change > i == 1 i == 2
> - disconnect
> - sync_conditions() i == 1 < i == 2
> Here the requested is downgraded back to new and the last-id
> is reset to zero to avoid data inconsistency.
> - connect
> sync_conditions() > i == 1
> The 'new' condition is promoted to 'acked', since there is no
> harm in doing that, as the last-id is zero and the cache will
> be cleared anyway.
>
> new req ack
> 2. - set_conditions(i == 2) i == 2
> - minitor_cond_change > i == 2
> - update3
> Ack didn't arrive in time.
> - set_conditions(i == 1) i == 1 i == 2
> Previous condition is still in-flight, so this one is 'new'
> and the monitor_cond_change is not sent out yet.
> - disconnect
> - sync_conditions() i == 1 x
> Here the requested condition is just dropped because there is
> no point sending it again. But the last-id is still cleared.
> - connect
> sync_conditions() > i == 1
> The 'new' condition is promoted to 'acked', since there is no
> harm in doing that, as the last-id is zero and the cache will
> be cleared anyway.
>
> The end result is the same: i == 1 sent in the next monitor request,
> but the path is different.
>
> In both cases the last_id is cleared though, so we get the full
> snapshot of the data with the new condition with the old cache
> cleared, so there is no data integrity issue.
>
> However, this double synchronization on disconnect and then on connect
> is tricky and hard to follow. Also, python code doesn't do the same
> thing. In python the second sync doesn't happen and we first send the
> acked conditions inside the monitor and then follow up with the new as
> a condition change request.
>
> Let's unify the logic and get rid of the two stages entirely:
>
> - If there are any in-flight requests, the last-id must be cleared to
> clear the cache.
>
> - If the last-id is zero or it is getting cleared, it's always safe to
> just use the latest conditions, even if not acked, since the cache
> will be cleared anyway.
>
> - Only if the last-id is non-zero and stays non-zero, then we need to
> first send the acked and then follow up with the new.
>
> Applying the same logic to python and C implementation to make them
> both the same and work in one stage. After this change calling
> sync_conditions() multiple times in a row doesn't change the state.
>
> This change is necessary for the upcoming functionality that will move
> the condition synchronization out of the direct FSM restart path.
>
> No new tests added as there should be no user-visible changes.
> At least for the C version. And the existing tests cover the high
> level logic of the reconnect, e.g., the test mentioned above.
>
> Signed-off-by: Ilya Maximets <[email protected]>
> ---
>
> Required for:
>
> https://patchwork.ozlabs.org/project/openvswitch/patch/b0b0aa8aaa80941d62d0de6ffaeca8cb3f6bf741.1780647734.git.lorenzo.bianc...@redhat.com/
>
> lib/ovsdb-cs.c | 88 ++++++++++++++++++++------------------------
> python/ovs/db/idl.py | 41 ++++++++++-----------
> 2 files changed, 59 insertions(+), 70 deletions(-)
Thanks, Dumitru and Lorenzo for review!
>
> diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c
> index 3920c2e5c..d96a97599 100644
> --- a/lib/ovsdb-cs.c
> +++ b/lib/ovsdb-cs.c
> @@ -1072,31 +1072,44 @@ ovsdb_cs_db_ack_condition(struct ovsdb_cs_db *db)
>
> /* Should be called when the CS fsm is restarted and resyncs table conditions
> * based on the state the DB is in:
> - * - if a non-zero last_id is available for the DB then upon reconnect
> + * - If a non-zero 'last_id' is available for the DB, then upon reconnect
> * the CS should first request acked conditions to avoid missing updates
> * about records that were added before the transaction with
> - * txn-id == last_id. If there were requested condition changes in flight
> - * (i.e., req_cond not NULL) and the CS client didn't set new conditions
> - * (i.e., new_cond is NULL) then move req_cond to new_cond to trigger a
> - * follow up monitor_cond_change request.
> - * - if there's no last_id available for the DB then it's safe to use the
> - * latest conditions set by the CS client even if they weren't acked yet.
> + * txn-id == last_id.
> + *
> + * - If there were changes in flight then there are two cases:
> + * a. either the server already processed the requested monitor condition
> + * change but the FSM was restarted before the client was notified.
> + * In this case the client should clear its local cache because it's
> + * out of sync with the monitor view on the server side.
> + *
> + * b. OR the server hasn't processed the requested monitor condition
> + * change yet.
> + *
> + * As there's no easy way to differentiate between the two, and given that
> + * this condition should be rare, reset the 'last_id', essentially flushing
> + * the local cached DB contents.
> + *
> + * - If there's no 'last_id' available for the DB or it was reset, then it's
> + * safe to use the latest conditions set by the CS client even if they
> + * weren't acked yet, since the local cache will be cleared anyway.
> */
> static void
> ovsdb_cs_db_sync_condition(struct ovsdb_cs_db *db)
> {
> - bool ack_all = uuid_is_zero(&db->last_id);
> - if (ack_all) {
> - db->cond_changed = false;
> - }
> -
> struct ovsdb_cs_db_table *table;
> +
> HMAP_FOR_EACH (table, hmap_node, &db->tables) {
> - /* When monitor_cond_since requests will be issued, the
> - * table->ack_cond condition will be added to the "where" clause".
> - * Follow up monitor_cond_change requests will use table->new_cond.
> - */
> - if (ack_all) {
> + if (table->req_cond) {
> + /* There was an in-flight condition change - reset. */
> + db->last_id = UUID_ZERO;
> + break;
> + }
> + }
> +
> + if (uuid_is_zero(&db->last_id)) {
> + /* No 'last_id' - use the latest conditions for the monitor request.
> */
> + HMAP_FOR_EACH (table, hmap_node, &db->tables) {
> if (table->new_cond) {
> json_destroy(table->req_cond);
> table->req_cond = table->new_cond;
> @@ -1108,39 +1121,16 @@ ovsdb_cs_db_sync_condition(struct ovsdb_cs_db *db)
> table->ack_cond = table->req_cond;
> table->req_cond = NULL;
> }
> - } else {
> - if (table->req_cond) {
> - /* There was an in-flight monitor_cond_change request. It's
> no
> - * longer relevant in the restarted FSM, so clear it. */
> - if (table->new_cond) {
> - /* We will send a new monitor_cond_change with the new
> - * condition. The previously in-flight condition is
> - * irrelevant and we can just forget about it. */
> - json_destroy(table->req_cond);
> - } else {
> - /* The restarted FSM needs to again send a request for
> the
> - * previously in-flight condition. */
> - table->new_cond = table->req_cond;
> - }
> - table->req_cond = NULL;
> + }
> + /* Nothig to send after the initial monitor request. */
There is one more typo here: 'Nothig' -> 'Nothing'.
> + db->cond_changed = false;
And I also missed this line in the python code. It's inconsequential, since
there are no 'new' conditions to send at this point anyway, but we should be
consistent.
So, I added this line to the python code, fixed the typos here and in the
commit message and applied the patch.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
