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(-)
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
