kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From mpe...@apache.org
Subject kudu git commit: [raft_consensus-itest] proper fix for Test_KUDU_1735
Date Sat, 13 Jan 2018 03:19:11 GMT
Repository: kudu
Updated Branches:
  refs/heads/master 8058825ba -> dc0621d76


[raft_consensus-itest] proper fix for Test_KUDU_1735

Adapted the logic of the Test_KUDU_1735 scenario to work for both
3-2-3 and 3-4-3 replication schemes.

This is a follow-up for 8bcc80eec075321fa4444ef26b2a0ccac12ac9d7.

Change-Id: Ibd7695f2b78da9755eb109940b40bda2bc6ec02e
Reviewed-on: http://gerrit.cloudera.org:8080/8989
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mpercy@apache.org>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/dc0621d7
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/dc0621d7
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/dc0621d7

Branch: refs/heads/master
Commit: dc0621d7673723cd417aa9ffb7e59b2c279971b9
Parents: 8058825
Author: Alexey Serbin <aserbin@cloudera.com>
Authored: Tue Jan 9 13:16:36 2018 -0800
Committer: Mike Percy <mpercy@apache.org>
Committed: Sat Jan 13 03:18:56 2018 +0000

----------------------------------------------------------------------
 .../integration-tests/raft_consensus-itest.cc   | 114 +++++++++++++++----
 src/kudu/util/fault_injection.cc                |   2 +-
 2 files changed, 91 insertions(+), 25 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/dc0621d7/src/kudu/integration-tests/raft_consensus-itest.cc
----------------------------------------------------------------------
diff --git a/src/kudu/integration-tests/raft_consensus-itest.cc b/src/kudu/integration-tests/raft_consensus-itest.cc
index 281ef57..4cb0627 100644
--- a/src/kudu/integration-tests/raft_consensus-itest.cc
+++ b/src/kudu/integration-tests/raft_consensus-itest.cc
@@ -115,6 +115,7 @@ using kudu::itest::StartElection;
 using kudu::itest::TServerDetails;
 using kudu::itest::TabletServerMap;
 using kudu::itest::WAIT_FOR_LEADER;
+using kudu::itest::WaitForNumTabletsOnTS;
 using kudu::itest::WaitForOpFromCurrentTerm;
 using kudu::itest::WaitForReplicasReportedToMaster;
 using kudu::itest::WaitForServersToAgree;
@@ -2396,62 +2397,127 @@ TEST_F(RaftConsensusITest, TestChangeConfigRejectedUnlessNoopReplicated)
{
   ASSERT_STR_CONTAINS(s.ToString(), "Leader has not yet committed an operation in its own
term");
 }
 
+class RaftConsensusParamReplicationModesITest :
+    public RaftConsensusITest,
+    public ::testing::WithParamInterface<bool> {
+};
+INSTANTIATE_TEST_CASE_P(, RaftConsensusParamReplicationModesITest,
+                        ::testing::Bool());
+
 // Regression test for KUDU-1735, a crash in the case where a pending
 // config change operation is aborted during tablet deletion when that config change
 // was in fact already persisted to disk.
-TEST_F(RaftConsensusITest, Test_KUDU_1735) {
+TEST_P(RaftConsensusParamReplicationModesITest, Test_KUDU_1735) {
   const MonoDelta kTimeout = MonoDelta::FromSeconds(10);
+  const bool is_3_4_3 = GetParam();
   const vector<string> kTsFlags = {
     // This scenario uses 'manual election' mode and assumes no leader change.
     "--enable_leader_failure_detection=false",
-    // This test is specific for the 3-2-3 replica management scheme.
-    "--raft_prepare_replacement_before_eviction=false",
+    // The test runs in both 3-2-3 and 3-4-3 replication modes.
+    Substitute("--raft_prepare_replacement_before_eviction=$0", is_3_4_3),
   };
   const vector<string> kMasterFlags = {
-    // This scenario uses 'manual election' mode and assumes no leader change.
+    // This scenario uses 'manual election' mode.
     "--catalog_manager_wait_for_new_tablets_to_elect_leader=false",
-    // This test is specific for the 3-2-3 replica management scheme.
-    "--raft_prepare_replacement_before_eviction=false",
+    // The test runs in both 3-2-3 and 3-4-3 replication modes.
+    Substitute("--raft_prepare_replacement_before_eviction=$0", is_3_4_3),
   };
 
   NO_FATALS(BuildAndStart(kTsFlags, kMasterFlags));
 
-  vector<TServerDetails*> tservers;
-  vector<ExternalTabletServer*> external_tservers;
-  AppendValuesFromMap(tablet_servers_, &tservers);
-  for (TServerDetails* ts : tservers) {
-    external_tservers.push_back(cluster_->tablet_server_by_uuid(ts->uuid()));
-  }
-
-  // Elect server 0 as leader and wait for log index 1 to propagate to all servers.
-  TServerDetails* leader_tserver = tservers[0];
+  TServerDetails* leader_tserver =
+      tablet_servers_[cluster_->tablet_server(0)->uuid()];
+  TServerDetails* evicted_tserver =
+      tablet_servers_[cluster_->tablet_server(1)->uuid()];
+  // Elect leader replica.
   ASSERT_OK(StartElection(leader_tserver, tablet_id_, kTimeout));
   ASSERT_OK(WaitUntilLeader(leader_tserver, tablet_id_, kTimeout));
+
+  // Wait for log index 1 to propagate to all involved tablet servers.
   ASSERT_OK(WaitForServersToAgree(MonoDelta::FromSeconds(10), tablet_servers_,
                                   tablet_id_, 1));
 
   // Make follower tablet servers crash before writing a commit message.
-  for (int i = 1; i < cluster_->num_tablet_servers(); i++) {
-    ASSERT_OK(cluster_->SetFlag(external_tservers[i], "fault_crash_before_append_commit",
"1.0"));
+  for (const auto& e : tablet_servers_) {
+    const auto& server_uuid = e.first;
+    if (server_uuid == leader_tserver->uuid()) {
+      continue;
+    }
+    auto* ts = cluster_->tablet_server_by_uuid(server_uuid);
+    ASSERT_OK(cluster_->SetFlag(ts, "fault_crash_before_append_commit", "1.0"));
   }
 
   // Run a config change. This will cause the other servers to crash with pending config
   // change operations due to the above fault injection.
-  ASSERT_OK(RemoveServer(leader_tserver, tablet_id_, tservers[1], kTimeout));
-  for (int i = 1; i < cluster_->num_tablet_servers(); i++) {
-    ASSERT_OK(external_tservers[i]->WaitForInjectedCrash(kTimeout));
+  auto affected_servers = 0;
+  ASSERT_OK(RemoveServer(leader_tserver, tablet_id_, evicted_tserver, kTimeout));
+  for (const auto& e : tablet_servers_) {
+    const auto& server_uuid = e.first;
+    if (server_uuid == leader_tserver->uuid()) {
+      // The tablet server with the leader replica will not crash.
+      continue;
+    }
+    if (server_uuid == evicted_tserver->uuid() && is_3_4_3) {
+      // The removed server with follower replica will not receive any
+      // eviction-related configuration requests since it's not a part of the
+      // resulting Raft configuration. However, the server will receive config
+      // change request when a new voter replica is added to replace the removed
+      // one, and the behavior is different between 3-2-3 and 3-4-3 schemes.
+      //
+      // In case of the 3-2-3 scheme, the gone-and-back server will crash while
+      // trying to add the COMMIT entry into the WAL.  That change corresponds
+      // to the transaction which adds the server as a voting member of the
+      // resulting Raft configuration. Two out of three voting replicas are
+      // present, so that transaction is committed by the leader.
+      //
+      // In case of the 3-4-3 scheme, the replica is added back as a non-voter.
+      // Only one (out of two required) voting replica is available:
+      // the other replica crashes on an attempt to add a log entry about
+      // committing the configuration change on this server's eviction.
+      // So, since the configuration change on adding the server back as
+      // a non-voter replica cannot be committed, there isn't an attempt to add
+      // a commit entry into this server's WAL, so it should not crash.
+      continue;
+    }
+
+    // The remaining follower will crash while trying to commit the config
+    // change evicting the other follower.
+    auto* ts = cluster_->tablet_server_by_uuid(server_uuid);
+    ASSERT_OK(ts->WaitForInjectedCrash(kTimeout));
+    ++affected_servers;
+  }
+  ASSERT_GE(affected_servers, 1);
+
+  if (is_3_4_3) {
+    // In case of the 3-4-3 scheme, make sure the configuration change to
+    // add the removed server back as a non-voter hasn't been committed.
+    ASSERT_EVENTUALLY([&] {
+      for (auto* srv : { leader_tserver, evicted_tserver }) {
+        consensus::ConsensusStatePB cstate;
+        ASSERT_OK(itest::GetConsensusState(srv, tablet_id_, kTimeout, &cstate));
+        ASSERT_TRUE(cstate.has_pending_config());
+      }
+    });
   }
 
   // Delete the table, so that when we restart the crashed servers, they'll get RPCs to
   // delete tablets while config changes are pending.
   ASSERT_OK(client_->DeleteTable(kTableId));
 
-  // Restart the crashed tservers and wait for them to delete their replicas.
-  for (int i = 1; i < cluster_->num_tablet_servers(); i++) {
-    auto* ts = external_tservers[i];
+  // Restart tablet servers with follower replicas and wait for them to delete
+  // their replicas.
+  for (const auto& e : tablet_servers_) {
+    const auto& server_uuid = e.first;
+    if (server_uuid == leader_tserver->uuid()) {
+      // The leader should not crash, no need to restart it.
+      continue;
+    }
+    auto* ts = cluster_->tablet_server_by_uuid(server_uuid);
     ts->Shutdown();
     ASSERT_OK(ts->Restart());
-    ASSERT_OK(WaitForNumTabletsOnTS(tservers[i], 0, kTimeout, nullptr));
+  }
+  for (const auto& e : tablet_servers_) {
+    ASSERT_OK(WaitForNumTabletsOnTS(e.second, 0, kTimeout, nullptr));
   }
 }
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/dc0621d7/src/kudu/util/fault_injection.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/fault_injection.cc b/src/kudu/util/fault_injection.cc
index 61b1437..6638bb6 100644
--- a/src/kudu/util/fault_injection.cc
+++ b/src/kudu/util/fault_injection.cc
@@ -56,7 +56,7 @@ void DoMaybeFault(const char* fault_str, double fraction) {
   }
   LOG(ERROR) << "Injecting fault: " << fault_str << " (process will exit)";
   // _exit will exit the program without running atexit handlers. This more
-  // accurately simiulates a crash.
+  // accurately simulates a crash.
   _exit(kExitStatus);
 }
 


Mime
View raw message