Improve ReplaceMemberRollback UT

- Don't check rdev being destroyed on removed member, because there is a known issue that if the removed member can not catch up within 5*HB since respond leave_cluster_request, it has no chance to detect itself removed from the group.
- Disable complete_replace_member in ReplaceMemberRollback UT.

Author: yuwmao

conanfile.py

@@ -9,7 +9,7 @@
 
 class HomestoreConan(ConanFile):
     name = "homestore"
-    version = "7.1.2"
+    version = "7.1.3"
 
     homepage = "https://github.com/eBay/Homestore"
     description = "HomeStore Storage Engine"

src/lib/replication/service/raft_repl_service.cpp

@@ -14,6 +14,7 @@
  *********************************************************************************/
 #include <sisl/logging/logging.h>
 #include <iomgr/io_environment.hpp>
+#include <iomgr/iomgr_flip.hpp>
 #include <chrono>
 
 #include <boost/uuid/string_generator.hpp>
@@ -750,6 +751,12 @@ void RaftReplService::flush_durable_commit_lsn() {
 
 void RaftReplService::monitor_replace_member_replication_status() {
     std::unique_lock lg(m_rd_map_mtx);
+#ifdef _PRERELEASE
+    if (iomgr_flip::instance()->test_flip("disable_monitor_replace_member_replication_status")) {
+        LOGINFOMOD(replication, "flip disable_monitor_replace_member_replication_status triggered");
+        return;
+    }
+#endif
     for (auto& rdev_parent : m_rd_map) {
         auto rdev = std::dynamic_pointer_cast< RaftReplDev >(rdev_parent.second);
         rdev->monitor_replace_member_replication_status();

src/tests/test_common/homestore_test_common.hpp

@@ -250,7 +250,7 @@ class HSTestHelper {
         freq.set_count(count);
         freq.set_percent(percent);
         m_fc.inject_noreturn_flip(flip_name, {null_cond}, freq);
-        LOGDEBUG("Flip {} set", flip_name);
+        LOGINFO("Flip {} set", flip_name);
     }
 
     void set_delay_flip(const std::string flip_name, uint64_t delay_usec, uint32_t count = 1, uint32_t percent = 100) {

src/tests/test_raft_repl_dev_dynamic.cpp

@@ -25,8 +25,10 @@ class ReplDevDynamicTest : public RaftReplDevTestBase {
     }
 };
 
+#ifdef _PRERELEASE
 TEST_F(ReplDevDynamicTest, ReplaceMember) {
     LOGINFO("ReplaceMember test started replica={}", g_helper->replica_num());
+    g_helper->set_basic_flip("disable_monitor_replace_member_replication_status", 1);
     // Write some IO's, replace a member, validate all members data except which is out.
     auto db = dbs_.back();
     auto num_replicas = SISL_OPTIONS["replicas"].as< uint32_t >();
@@ -65,6 +67,10 @@ TEST_F(ReplDevDynamicTest, ReplaceMember) {
         std::string new_task_id = "mismatched_task_id";
         replace_member(db, new_task_id, g_helper->replica_id(member_out), g_helper->replica_id(member_in), 0,
                        ReplServiceError::REPLACE_MEMBER_TASK_MISMATCH);
+        auto& raft_repl_svc = dynamic_cast< RaftReplService& >(hs()->repl_service());
+        // trigger complete_replace_member, default laggy threshold=2000, so complete_replace_member should be
+        // triggered.
+        raft_repl_svc.monitor_replace_member_replication_status();
     });
     if (is_replica_num_in({0, 1, member_in})) {
         // Skip the member which is going to be replaced. Validate data on all other replica's.
@@ -93,13 +99,14 @@ TEST_F(ReplDevDynamicTest, ReplaceMember) {
             check_replace_member_status(db, task_id, g_helper->replica_id(member_out), g_helper->replica_id(member_in)),
             ReplaceMemberStatus::COMPLETED);
     });
+    g_helper->remove_flip("disable_monitor_replace_member_replication_status");
     LOGINFO("ReplaceMember test done replica={}", g_helper->replica_num());
 }
 
-// After replace member is in progress, rollback replace member operation before complete_replace_member is
-// called(triggered after 60s).
+// After replace member is in progress, rollback replace member operation(complete_replace_member will be disabled)
 TEST_F(ReplDevDynamicTest, ReplaceMemberRollback) {
     LOGINFO("ReplaceMember test started replica={}", g_helper->replica_num());
+    g_helper->set_basic_flip("disable_monitor_replace_member_replication_status", 1);
     // Write some IO's, replace a member, validate all members data except which is out.
     auto db = dbs_.back();
     auto num_replicas = SISL_OPTIONS["replicas"].as< uint32_t >();
@@ -125,6 +132,12 @@ TEST_F(ReplDevDynamicTest, ReplaceMemberRollback) {
             LOGDEBUG("Not added to group yet")
             std::this_thread::sleep_for(std::chrono::microseconds(300));
         }
+        // Need to wait for log being caught up. There is a known issue that if the removed member can not catch up
+        // within 5*HB since respond leave_cluster_request, it has no chance to detect itself removed from the group.
+        // Unlike other tests, what we remove is a normal member who has all logs, here we remove new member which is
+        // very likely to be behind.
+        wait_for_commits(num_io_entries);
+        LOGINFO("Member in got all commits");
     }
 
     g_helper->sync_for_verify_start(num_members);
@@ -141,9 +154,8 @@ TEST_F(ReplDevDynamicTest, ReplaceMemberRollback) {
 
     g_helper->sync_for_verify_start(num_members);
     LOGINFO("rollback triggered, sync_for_verify_start replica={} ", g_helper->replica_num());
-    // verify member_in is removed from group.
+
     if (g_helper->replica_num() == member_in) {
-        // The member_in will have the repl dev destroyed.
         auto repl_dev = std::dynamic_pointer_cast< RaftReplDev >(db->repl_dev());
         while (repl_dev && !repl_dev->is_destroyed()) {
             std::this_thread::sleep_for(std::chrono::seconds(1));
@@ -164,12 +176,13 @@ TEST_F(ReplDevDynamicTest, ReplaceMemberRollback) {
     }
 
     g_helper->sync_for_cleanup_start(num_members);
+    g_helper->remove_flip("disable_monitor_replace_member_replication_status");
     LOGINFO("ReplaceMember test done replica={}", g_helper->replica_num());
 }
 
 TEST_F(ReplDevDynamicTest, TwoMemberDown) {
     LOGINFO("TwoMemberDown test started replica={}", g_helper->replica_num());
-
+    g_helper->set_basic_flip("disable_monitor_replace_member_replication_status", 1);
     // Make two members down in a group and leader cant reach a quorum.
     // We set the custom quorum size to 1 and call replace member.
     // Leader should do some writes to validate it has reach quorum size.
@@ -238,6 +251,7 @@ TEST_F(ReplDevDynamicTest, TwoMemberDown) {
     }
 
     g_helper->sync_for_cleanup_start(num_members);
+    g_helper->remove_flip("disable_monitor_replace_member_replication_status");
     LOGINFO("TwoMemberDown test done replica={}", g_helper->replica_num());
 }
 
@@ -246,6 +260,7 @@ TEST_F(ReplDevDynamicTest, OutMemberDown) {
     // replica0 should be able to baseline resync to replica4(new member).
     // Write some IO's, replace a member, validate all members data except which is out.
     LOGINFO("OutMemberDown test started replica={}", g_helper->replica_num());
+    g_helper->set_basic_flip("disable_monitor_replace_member_replication_status", 1);
     auto db = dbs_.back();
     auto num_replicas = SISL_OPTIONS["replicas"].as< uint32_t >();
     auto num_members = SISL_OPTIONS["replicas"].as< uint32_t >() + SISL_OPTIONS["spare_replicas"].as< uint32_t >();
@@ -333,10 +348,12 @@ TEST_F(ReplDevDynamicTest, OutMemberDown) {
         });
     }
     g_helper->sync_for_cleanup_start(num_members);
+    g_helper->remove_flip("disable_monitor_replace_member_replication_status");
     LOGINFO("OutMemberDown test done replica={}", g_helper->replica_num());
 }
 
 TEST_F(ReplDevDynamicTest, LeaderReplace) {
+    g_helper->set_basic_flip("disable_monitor_replace_member_replication_status", 1);
     // replica0(leader) and replica1 and replica2 is up. Replace replica0(leader) with replica3.
     // replica0 will yield leadership and any other replica will be come leader  and leader
     // will do baseline resync to replica4(new member).
@@ -389,6 +406,8 @@ TEST_F(ReplDevDynamicTest, LeaderReplace) {
         ASSERT_EQ(
             check_replace_member_status(db, task_id, g_helper->replica_id(member_out), g_helper->replica_id(member_in)),
             ReplaceMemberStatus::IN_PROGRESS);
+        auto& raft_repl_svc = dynamic_cast< RaftReplService& >(hs()->repl_service());
+        raft_repl_svc.monitor_replace_member_replication_status();
     });
     if (g_helper->replica_num() == member_out) {
         // The out member will have the repl dev destroyed.
@@ -409,6 +428,7 @@ TEST_F(ReplDevDynamicTest, LeaderReplace) {
             check_replace_member_status(db, task_id, g_helper->replica_id(member_out), g_helper->replica_id(member_in)),
             ReplaceMemberStatus::COMPLETED);
     });
+    g_helper->remove_flip("disable_monitor_replace_member_replication_status");
     LOGINFO("LeaderReplace test done replica={}", g_helper->replica_num());
 }
 
@@ -479,6 +499,7 @@ TEST_F(ReplDevDynamicTest, OneMemberRestart) {
     });
     LOGINFO("OneMemberRestart test done replica={}", g_helper->replica_num());
 }
+#endif
 
 TEST_F(ReplDevDynamicTest, ValidateRequest) {
     LOGINFO("ValidateRequest test started replica={}", g_helper->replica_num());