[ovs-dev] [PATCH v2] ovsdb-server: Replace in-memory DB contents at raft install_snapshot.

Dumitru Ceara dceara at redhat.com
Wed Aug 5 19:40:51 UTC 2020


Every time a follower has to install a snapshot received from the
leader, it should also replace the data in memory. Right now this only
happens when snapshots are installed that also change the schema.

This can lead to inconsistent DB data on follower nodes and the snapshot
may fail to get applied.

CC: Han Zhou <hzhou at ovn.org>
Fixes: bda1f6b60588 ("ovsdb-server: Don't disconnect clients after raft install_snapshot.")
Acked-by: Han Zhou <hzhou at ovn.org>
Signed-off-by: Dumitru Ceara <dceara at redhat.com>
---
v2:
- Added Han's Ack.
- Made test more resilient as suggested by Han.
---
 ovsdb/ovsdb-server.c    | 21 +++++++++++++--------
 tests/idltest.ovsschema |  9 +++++++++
 tests/ovsdb-cluster.at  | 32 +++++++++++++++++++++++++++++---
 tests/ovsdb-idl.at      |  1 +
 4 files changed, 52 insertions(+), 11 deletions(-)

diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index ef4e996..fd7891a 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -543,13 +543,14 @@ parse_txn(struct server_config *config, struct db *db,
           const struct ovsdb_schema *schema, const struct json *txn_json,
           const struct uuid *txnid)
 {
-    if (schema && (!db->db->schema || strcmp(schema->version,
-                                             db->db->schema->version))) {
+    if (schema) {
         /* We're replacing the schema (and the data).  Destroy the database
          * (first grabbing its storage), then replace it with the new schema.
          * The transaction must also include the replacement data.
          *
-         * Only clustered database schema changes go through this path. */
+         * Only clustered database schema changes and snapshot installs
+         * go through this path.
+         */
         ovs_assert(txn_json);
         ovs_assert(ovsdb_storage_is_clustered(db->db->storage));
 
@@ -559,11 +560,15 @@ parse_txn(struct server_config *config, struct db *db,
             return error;
         }
 
-        ovsdb_jsonrpc_server_reconnect(
-            config->jsonrpc, false,
-            (db->db->schema
-             ? xasprintf("database %s schema changed", db->db->name)
-             : xasprintf("database %s connected to storage", db->db->name)));
+        if (!db->db->schema ||
+            strcmp(schema->version, db->db->schema->version)) {
+            ovsdb_jsonrpc_server_reconnect(
+                config->jsonrpc, false,
+                (db->db->schema
+                ? xasprintf("database %s schema changed", db->db->name)
+                : xasprintf("database %s connected to storage",
+                            db->db->name)));
+        }
 
         ovsdb_replace(db->db, ovsdb_create(ovsdb_schema_clone(schema), NULL));
 
diff --git a/tests/idltest.ovsschema b/tests/idltest.ovsschema
index e02b975..e04755e 100644
--- a/tests/idltest.ovsschema
+++ b/tests/idltest.ovsschema
@@ -54,6 +54,15 @@
       },
       "isRoot" : true
     },
+    "indexed": {
+      "columns": {
+        "i": {
+          "type": "integer"
+        }
+      },
+      "indexes": [["i"]],
+      "isRoot" : true
+    },
     "simple": {
       "columns": {
         "b": {
diff --git a/tests/ovsdb-cluster.at b/tests/ovsdb-cluster.at
index 9714545..e0758e9 100644
--- a/tests/ovsdb-cluster.at
+++ b/tests/ovsdb-cluster.at
@@ -332,13 +332,29 @@ for i in `seq $n`; do
     AT_CHECK([ovsdb_client_wait unix:s$i.ovsdb $schema_name connected])
 done
 
+AT_CHECK([ovsdb-client transact unix:s1.ovsdb '[["idltest",
+      {"op": "insert",
+       "table": "indexed",
+       "row": {"i": 0}}]]'], [0], [ignore], [ignore])
+
 # Kill one follower (s2) and write some data to cluster, so that the follower is falling behind
 printf "\ns2: stopping\n"
 OVS_APP_EXIT_AND_WAIT_BY_TARGET([`pwd`/s2], [s2.pid])
 
+# Delete "i":0 and readd it to get a different UUID for it.
+AT_CHECK([ovsdb-client transact unix:s1.ovsdb '[["idltest",
+      {"op": "delete",
+       "table": "indexed",
+       "where": [["i", "==", 0]]}]]'], [0], [ignore], [ignore])
+
 AT_CHECK([ovsdb-client transact unix:s1.ovsdb '[["idltest",
       {"op": "insert",
-       "table": "simple",
+       "table": "indexed",
+       "row": {"i": 0}}]]'], [0], [ignore], [ignore])
+
+AT_CHECK([ovsdb-client transact unix:s1.ovsdb '[["idltest",
+      {"op": "insert",
+       "table": "indexed",
        "row": {"i": 1}}]]'], [0], [ignore], [ignore])
 
 # Compact leader online to generate snapshot
@@ -355,8 +371,18 @@ AT_CHECK([ovsdb_client_wait unix:s2.ovsdb $schema_name connected])
 # succeed.
 AT_CHECK([ovsdb-client transact unix:s2.ovsdb '[["idltest",
       {"op": "insert",
-       "table": "simple",
-       "row": {"i": 1}}]]'], [0], [ignore], [ignore])
+       "table": "indexed",
+       "row": {"i": 2}}]]'], [0], [ignore], [ignore])
+
+# The snapshot should overwrite the in-memory contents of the DB on S2
+# without generating any constraint violations. All tree records (0, 1, 2)
+# should be in the DB at this point.
+AT_CHECK([ovsdb-client --no-headings dump unix:s2.ovsdb idltest indexed | uuidfilt | sort -k 2], [0], [dnl
+<0> 0
+<1> 1
+<2> 2
+indexed table
+])
 
 for i in `seq $n`; do
     OVS_APP_EXIT_AND_WAIT_BY_TARGET([`pwd`/s$i], [s$i.pid])
diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
index 4efed88..789ae23 100644
--- a/tests/ovsdb-idl.at
+++ b/tests/ovsdb-idl.at
@@ -954,6 +954,7 @@ AT_CHECK([sort stdout | uuidfilt], [0],
 
 # Check that ovsdb-idl figured out that table link2 and column l2 are missing.
 AT_CHECK([grep ovsdb_idl stderr | sort], [0], [dnl
+test-ovsdb|ovsdb_idl|idltest database lacks indexed table (database needs upgrade?)
 test-ovsdb|ovsdb_idl|idltest database lacks link2 table (database needs upgrade?)
 test-ovsdb|ovsdb_idl|idltest database lacks simple5 table (database needs upgrade?)
 test-ovsdb|ovsdb_idl|idltest database lacks singleton table (database needs upgrade?)
-- 
1.8.3.1



More information about the dev mailing list