[ovs-dev] [PATCH v3] ovsdb-idl: Avoid inconsistent IDL state with OVSDB_MONITOR_V3.
Dumitru Ceara
dceara at redhat.com
Thu Apr 9 00:01:08 UTC 2020
Assuming an ovsdb client connected to a database using OVSDB_MONITOR_V3
(i.e., "monitor_cond_since" method) with the initial monitor condition
MC1.
Assuming the following two transactions are executed on the
ovsdb-server:
TXN1: "insert record R1 in table T1"
TXN2: "insert record R2 in table T2"
If the client's monitor condition MC1 for table T2 matches R2 then the
client will receive the following update3 message:
method="update3", "insert record R2 in table T2", last-txn-id=TXN2
At this point, if the presence of the new record R2 in the IDL triggers
the client to update its monitor condition to MC2 and add a clause for
table T1 which matches R1, a monitor_cond_change message is sent to the
server:
method="monitor_cond_change", "clauses from MC2"
In normal operation the ovsdb-server will reply with a new update3
message of the form:
method="update3", "insert record R1 in table T1", last-txn-id=TXN2
However, if the connection drops in the meantime, this last update might
get lost.
It might happen that during the reconnect a new transaction happens
that modifies the original record R1:
TXN3: "modify record R1 in table T1"
When the client reconnects, it will try to perform a fast resync by
sending:
method="monitor_cond_since", "clauses from MC2", last-txn-id=TXN2
Because TXN2 is still in the ovsdb-server transaction history, the
server replies with the changes from the most recent transactions only,
i.e., TXN3:
result="true", last-txbb-id=TXN3, "modify record R1 in table T1"
This causes the IDL on the client in to end up in an inconsistent
state because it has never seen the update that created R1.
Such a scenario is described in:
https://bugzilla.redhat.com/show_bug.cgi?id=1808580#c22
To avoid hitting this issue we now:
- clear db->last_id whenever an IDL retry happens while there is a
monitor_cond_change request in flight.
- clear db->last_id whenever a monitor condition is changed while the
IDL is not in state IDL_S_MONITORING.
This ensures that updates of type "insert" that happened before the last
transaction known by the IDL but didn't match old monitor conditions are
sent upon reconnect if the monitor condition has changed to include them
in the meantime.
This commit also adds a generic recovery mechanism which triggers an IDL
retry with fast resync disabled in case the IDL has detected that it
ended up in an inconsistent state due to other bugs in the
ovsdb-server/ovsdb-idl implementation.
CC: Han Zhou <hzhou at ovn.org>
CC: Andy Zhou <azhou at ovn.org>
Fixes: 403a6a0cb003 ("ovsdb-idl: Fast resync from server when connection reset.")
Fixes: db2b5757328c ("lib: add monitor2 support in ovsdb-idl.")
Signed-off-by: Dumitru Ceara <dceara at redhat.com>
---
V3:
- Change commit title.
- Update commit message.
- Fix monitor_cond_since ovsdb-idl implementation.
V2:
- Address Mark's comments:
- change the error log message to reflect the action taken.
- use ovsdb_error() instead of ovsdb_syntax_error().
---
lib/ovsdb-idl.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 64 insertions(+), 4 deletions(-)
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 1535ad7..cf5db41 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -324,7 +324,8 @@ static bool ovsdb_idl_process_update(struct ovsdb_idl_table *,
static bool ovsdb_idl_process_update2(struct ovsdb_idl_table *,
const struct uuid *,
const char *operation,
- const struct json *row);
+ const struct json *row,
+ bool *inconsistent);
static void ovsdb_idl_insert_row(struct ovsdb_idl_row *, const struct json *);
static void ovsdb_idl_delete_row(struct ovsdb_idl_row *);
static bool ovsdb_idl_modify_row(struct ovsdb_idl_row *, const struct json *);
@@ -655,6 +656,19 @@ ovsdb_idl_state_to_string(enum ovsdb_idl_state state)
static void
ovsdb_idl_retry_at(struct ovsdb_idl *idl, const char *where)
{
+ /* If there's an outstanding request of type monitor_cond_change and
+ * we're in monitor_cond_since mode then we can't trust that all relevant
+ * updates from transaction idl->data.last_id have been received as we
+ * might have relaxed the monitor condition with our last request and
+ * might be missing previously not monitored records.
+ *
+ * Clear last_id to make sure that the next time monitor_cond_since is
+ * sent (i.e., after reconnect) we get the complete view of the database.
+ */
+ if (idl->request_id &&
+ idl->data.monitoring == OVSDB_IDL_MONITORING_COND_SINCE) {
+ memset(&idl->data.last_id, 0, sizeof idl->data.last_id);
+ }
ovsdb_idl_force_reconnect(idl);
ovsdb_idl_transition_at(idl, IDL_S_RETRY, where);
}
@@ -1518,6 +1532,24 @@ ovsdb_idl_db_set_condition(struct ovsdb_idl_db *db,
ovsdb_idl_condition_clone(&table->condition, condition);
db->cond_changed = table->cond_changed = true;
poll_immediate_wake();
+
+ /* If the change happens while the IDL is not in state
+ * IDL_S_MONITORING there's no guarantee that the updated
+ * condition doesn't match records that were inserted before the
+ * transaction id received with the most recent update3
+ * (i.e., db->last_id). If the new condition would match such a
+ * record, when the reconnect is successful and "monitor_cond_since"
+ * is be sent, the server will not include the updates that
+ * originally created the record.
+ *
+ * Clear last_id to make sure that the next time monitor_cond_since
+ * is sent (i.e., after reconnect) we get the complete view of the
+ * database.
+ */
+ if (ovsdb_idl_has_ever_connected(db->idl) &&
+ db->idl->state != IDL_S_MONITORING) {
+ memset(&db->last_id, 0, sizeof db->last_id);
+ }
return seqno + 1;
}
@@ -2318,10 +2350,26 @@ ovsdb_idl_db_parse_update__(struct ovsdb_idl_db *db,
row = shash_find_data(json_object(row_update), operation);
if (row) {
+ bool inconsistent = false;
+
if (ovsdb_idl_process_update2(table, &uuid, operation,
- row)) {
+ row, &inconsistent)) {
db->change_seqno++;
}
+
+ /* If the db is in an inconsistent state, clear the
+ * db->last_id and retry to get the complete view of
+ * the database.
+ */
+ if (inconsistent) {
+ memset(&db->last_id, 0, sizeof db->last_id);
+ ovsdb_idl_retry(db->idl);
+ return ovsdb_error(NULL,
+ "<row_update2> received for "
+ "inconsistent IDL: "
+ "reconnecting IDL with "
+ "fast resync disabled");
+ }
break;
}
}
@@ -2445,16 +2493,26 @@ ovsdb_idl_process_update(struct ovsdb_idl_table *table,
}
/* Returns true if a column with mode OVSDB_IDL_MODE_RW changed, false
- * otherwise. */
+ * otherwise.
+ *
+ * NOTE: When processing the "modify" updates, the IDL can determine that
+ * previous updates were missed (e.g., due to bugs) and that rows that don't
+ * exist locally should be updated. This indicates that the
+ * IDL is in an inconsistent state and, unlike in ovsdb_idl_process_update(),
+ * the missing rows cannot be inserted. If this is the case, 'inconsistent'
+ * is set to true to indicate the catastrophic failure.
+ */
static bool
ovsdb_idl_process_update2(struct ovsdb_idl_table *table,
const struct uuid *uuid,
const char *operation,
- const struct json *json_row)
+ const struct json *json_row,
+ bool *inconsistent)
{
struct ovsdb_idl_row *row;
row = ovsdb_idl_get_row(table, uuid);
+ *inconsistent = false;
if (!strcmp(operation, "delete")) {
/* Delete row. */
if (row && !ovsdb_idl_row_is_orphan(row)) {
@@ -2486,11 +2544,13 @@ ovsdb_idl_process_update2(struct ovsdb_idl_table *table,
VLOG_WARN_RL(&semantic_rl, "cannot modify missing but "
"referenced row "UUID_FMT" in table %s",
UUID_ARGS(uuid), table->class_->name);
+ *inconsistent = true;
return false;
}
} else {
VLOG_WARN_RL(&semantic_rl, "cannot modify missing row "UUID_FMT" "
"in table %s", UUID_ARGS(uuid), table->class_->name);
+ *inconsistent = true;
return false;
}
} else {
--
1.8.3.1
More information about the dev
mailing list