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]> > ---
Hi Ilya, Looks good to me, it's way nicer to do the condition synchronization the way your patch does. Thanks for taking the time to dig into this! There's a tiny typo (twice) in the commit message, "minitor", but that can be taken care of when you merge the patch I guess. Acked-by: Dumitru Ceara <[email protected]> Regards, Dumitru > > 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(-) > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
