MB-21587: Rollback: Don't use CAS when rolling back values in HashTable 78/69478/6
authorDave Rigby <daver@couchbase.com>
Fri, 4 Nov 2016 14:00:10 +0000 (14:00 +0000)
committerDave Rigby <daver@couchbase.com>
Fri, 11 Nov 2016 02:05:37 +0000 (02:05 +0000)
When ep-engine performs a rollback, if a key needs to be reverted to a
previous value (it has been modified since the rollbackSeqno), the
HashTable update fails due to being called incorrectly - it uses a set
with CAS (where the CAS used is the one for the previous item),
whereas the element in the HashTable will have a newer CAS (by
defintion as we are trying to roll it back).

The effect of this is that after rollback, old values for keys still
exist in memory (disk is correct) on the replica vBucket(s). In the
event of a subsequent failover (and promotion of the replica ->
active), incorrect values will be returned for such keys.

Fix by changing the HashTable::set to a non-CAS one.

As part of testing, made use of recently-added equality and output
operators for Item and related classes:

    [ RUN      ] RollbackTest.MB21587_RollbackAfterMutation
    source/ep-engine/tests/module_tests/evp_store_rollback_test.cc:70: Failure
    Value of: *result.getValue()
      Actual: Item[0x10441d860] with key:a
            value:Blob[0x104543180] with size:5 extMetaLen:1 age:0 data: <01 01 n e w>
            metadata:ItemMetaData[0x10441d868] with cas:5407a59650006 revSeqno:2 flags:0 exptime:0
            bySeqno:7 queuedTime:0 vbucketId:0 op:set nru:2
    Expected: item_v1
    Which is: Item[0x7fff5f181ce0] with key:a
            value:Blob[0x104543160] with size:5 extMetaLen:1 age:0 data: <01 01 o l d>
            metadata:ItemMetaData[0x7fff5f181ce8] with cas:5407a59650005 revSeqno:1 flags:0 exptime:0
            bySeqno:6 queuedTime:0 vbucketId:0 op:set nru:2
    Fetched item after rollback should match item_v1
    [  FAILED  ] RollbackTest.MB21587_RollbackAfterMutation (22 ms)

Change-Id: I32577afd0f6f9c79122575f84a76acd00fb6f89a
Reviewed-on: http://review.couchbase.org/69478
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Daniel Owen <owend@couchbase.com>
Reviewed-by: Manu Dhundi <manu@couchbase.com>
CMakeLists.txt
src/ep.cc
tests/module_tests/evp_store_rollback_test.cc [new file with mode: 0644]
tests/module_tests/evp_store_test.cc
tests/module_tests/evp_store_test.h

index 840ffde..7952f66 100644 (file)
@@ -190,6 +190,7 @@ ADD_EXECUTABLE(ep-engine_ep_unit_tests
   tests/module_tests/ep_unit_tests_main.cc
   tests/module_tests/dcp_test.cc
   tests/module_tests/evp_engine_test.cc
+  tests/module_tests/evp_store_rollback_test.cc
   tests/module_tests/evp_store_test.cc
   tests/module_tests/evp_store_single_threaded_test.cc
   tests/module_tests/mutation_log_test.cc
index 360d22d..dc097cb 100644 (file)
--- a/src/ep.cc
+++ b/src/ep.cc
@@ -3929,7 +3929,7 @@ public:
                     setStatus(ENGINE_SUCCESS);
                 }
             } else {
-                mutation_type_t mtype = vb->ht.set(*it, it->getCas(),
+                mutation_type_t mtype = vb->ht.set(*it, /*cas*/0,
                                                    true, true,
                                                    engine.getEpStore()->
                                                         getItemEvictionPolicy(),
diff --git a/tests/module_tests/evp_store_rollback_test.cc b/tests/module_tests/evp_store_rollback_test.cc
new file mode 100644 (file)
index 0000000..13f022d
--- /dev/null
@@ -0,0 +1,73 @@
+/* -*- Mode: C++; tab-width: 4; c-basic-offset: 4; indent-tabs-mode: nil -*- */
+/*
+ *     Copyright 2016 Couchbase, Inc
+ *
+ *   Licensed under the Apache License, Version 2.0 (the "License");
+ *   you may not use this file except in compliance with the License.
+ *   You may obtain a copy of the License at
+ *
+ *       http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *   Unless required by applicable law or agreed to in writing, software
+ *   distributed under the License is distributed on an "AS IS" BASIS,
+ *   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *   See the License for the specific language governing permissions and
+ *   limitations under the License.
+ */
+
+/**
+ * Tests for Rollback functionality in EPStore.
+ */
+
+#include "evp_store_test.h"
+
+class RollbackTest : public EventuallyPersistentStoreTest
+{
+    void SetUp() override {
+        EventuallyPersistentStoreTest::SetUp();
+        // Start vbucket as active to allow us to store items directly to it.
+        store->setVBucketState(vbid, vbucket_state_active, false);
+
+        // For any rollback tests which actually want to rollback, we need
+        // to ensure that we don't rollback more than 50% of the seqno count
+        // as then the VBucket is just cleared (it'll instead expect a resync
+        // from zero.
+        // Therefore create 10 dummy items which we don't otherwise care
+        // about (most of the Rollback test only work with a couple of
+        // "active" items.
+        const auto dummy_elements = size_t{5};
+        for (size_t ii = 1; ii <= dummy_elements; ii++) {
+            auto res = store_item(vbid, "dummy" + std::to_string(ii),
+                                  "dummy");
+            ASSERT_EQ(ii, res.getBySeqno());
+        }
+        ASSERT_EQ(dummy_elements, store->flushVBucket(vbid));
+        initial_seqno = dummy_elements;
+    }
+
+protected:
+    int64_t initial_seqno;
+};
+
+// Test rollback after modifying an item - regression test for MB-21587.
+TEST_F(RollbackTest, MB21587_RollbackAfterMutation) {
+
+    // Setup: Store an item then flush the vBucket (creating a checkpoint);
+    // then update the item with a new value and create a second checkpoint.
+    auto item_v1 = store_item(vbid, "a", "old");
+    ASSERT_EQ(initial_seqno + 1, item_v1.getBySeqno());
+    ASSERT_EQ(1, store->flushVBucket(vbid));
+
+    auto item2 = store_item(vbid, "a", "new");
+    ASSERT_EQ(initial_seqno + 2, item2.getBySeqno());
+    ASSERT_EQ(1, store->flushVBucket(vbid));
+
+    // Test - rollback to seqno of item_v1 and verify that the previous value
+    // of the item has been restored.
+    ASSERT_EQ(ENGINE_SUCCESS, store->rollback(vbid, initial_seqno + 1));
+    auto result = store->get("a", vbid, nullptr, {});
+    EXPECT_EQ(ENGINE_SUCCESS, result.getStatus());
+    EXPECT_EQ(item_v1, *result.getValue())
+        << "Fetched item after rollback should match item_v1";
+    delete result.getValue();
+}
index 28d8b23..d5c1f66 100644 (file)
@@ -147,13 +147,17 @@ void EventuallyPersistentStoreTest::TearDown() {
     ExecutorPool::shutdown();
 }
 
-void EventuallyPersistentStoreTest::store_item(uint16_t vbid,
+Item EventuallyPersistentStoreTest::store_item(uint16_t vbid,
                                                const std::string& key,
                                                const std::string& value) {
+    uint8_t ext_meta[EXT_META_LEN] = {PROTOCOL_BINARY_DATATYPE_JSON};
+
     Item item(key.c_str(), key.size(), /*flags*/0, /*exp*/0, value.c_str(),
-              value.size());
+              value.size(), ext_meta, sizeof(ext_meta));
     item.setVBucketId(vbid);
     EXPECT_EQ(ENGINE_SUCCESS, store->set(item, nullptr));
+
+    return item;
 }
 
 
index 1119685..ac803d2 100644 (file)
@@ -91,8 +91,8 @@ protected:
 
     void TearDown() override;
 
-    /* Stores an item into the given vbucket. */
-    void store_item(uint16_t vbid, const std::string& key,
+    // Stores an item into the given vbucket. Returns the item stored.
+    Item store_item(uint16_t vbid, const std::string& key,
                     const std::string& value);
 
     static const char test_dbname[];