> 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]>
Acked-by: Lorenzo Bianconi <[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(-) > > 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. */ > + db->cond_changed = false; > + } else { > + /* No in-flight changes and a non-zero 'last_id'. Send acknowledged > + * first, then follow up with the new, if any. */ > + HMAP_FOR_EACH (table, hmap_node, &db->tables) { > + if (table->new_cond) { > db->cond_changed = true; > - > - /* 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. > - */ > - db->last_id = UUID_ZERO; > + break; > } > } > } > diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py > index 40e0ab600..99b8362e6 100644 > --- a/python/ovs/db/idl.py > +++ b/python/ovs/db/idl.py > @@ -130,15 +130,6 @@ class ConditionState(object): > if self._new_cond is not None: > self._req_cond, self._new_cond = (self._new_cond, None) > > - def reset(self): > - """Reset a requested condition change back to new""" > - if self._req_cond is not None: > - if self._new_cond is None: > - self._new_cond = self._req_cond > - self._req_cond = None > - return True > - return False > - > > class IdlTable(object): > def __init__(self, idl, table): > @@ -368,12 +359,10 @@ class Idl(object): > def sync_conditions(self): > """Synchronize condition state when the FSM is restarted > > - 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 IDL 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 > - and the IDL client didn't set new conditions, then reset the > requested > - conditions to new to trigger a follow-up monitor_cond_change request. > + 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 > @@ -387,19 +376,29 @@ class Idl(object): > 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. > - """ > - ack_all = self.last_id == str(uuid.UUID(int=0)) > - if ack_all: > - self.cond_changed = False > > + 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 client even if they > + weren't acked yet, since the local cache will be cleared anyway. > + """ > for table in self.tables.values(): > - if ack_all: > + if table.condition_state.requested is not None: > + # There was an in-flight condition change - reset. > + self.last_id = str(uuid.UUID(int=0)) > + break > + > + if self.last_id == str(uuid.UUID(int=0)): > + # No 'last_id' - use the latest conditions for the monitor > request. > + for table in self.tables.values(): > table.condition_state.request() > table.condition_state.ack() > - else: > - if table.condition_state.reset(): > - self.last_id = str(uuid.UUID(int=0)) > + else: > + # No in-flight changes and a non-zero 'last_id'. Send > acknowledged > + # first, then follow up with the new, if any. > + for table in self.tables.values(): > + if table.condition_state.new is not None: > self.cond_changed = True > + break > > def restart_fsm(self): > # Resync data DB table conditions to avoid missing updated due to > -- > 2.54.0 >
_______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
