kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ale...@apache.org
Subject kudu git commit: [raft_consensus-itest] fix TestElectionMetrics flake
Date Wed, 11 Jul 2018 19:45:54 GMT
Repository: kudu
Updated Branches:
  refs/heads/master 03a3c6812 -> 077fe5762


[raft_consensus-itest] fix TestElectionMetrics flake

This patch fixes flakiness in the RaftConsensusITest.TestElectionMetrics
scenario.  The original TestElectionMetrics scenario was split into
two parts TestElectionMetricsPart[1,2] to separate tests that assume
no leader election happen and depend on the leader failure detection
mechanism, correspondingly.

Prior to this patch, the TestElectionMetrics scenario could fail if a
leader election happened inadvertently.

Before (1 out of 12 failed):
  http://dist-test.cloudera.org/job?job_id=aserbin.1530681412.7635

After  (not a single failure in 1K run):
  http://dist-test.cloudera.org/job?job_id=aserbin.1530918429.120803

Change-Id: I073c9989a6d5d5dc1eb104120a89d38cfce2ac6e
Reviewed-on: http://gerrit.cloudera.org:8080/10887
Tested-by: Alexey Serbin <aserbin@cloudera.com>
Reviewed-by: Will Berkeley <wdberkeley@gmail.com>
Reviewed-by: Attila Bukor <abukor@cloudera.com>


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

Branch: refs/heads/master
Commit: 077fe576288bfaa1aadc42596b3594c65ecf16e2
Parents: 03a3c68
Author: Alexey Serbin <aserbin@cloudera.com>
Authored: Thu Jul 5 13:39:06 2018 -0700
Committer: Alexey Serbin <aserbin@cloudera.com>
Committed: Wed Jul 11 19:42:22 2018 +0000

----------------------------------------------------------------------
 src/kudu/consensus/raft_consensus.cc            |  10 ++
 .../integration-tests/raft_consensus-itest.cc   | 136 +++++++++++++++----
 2 files changed, 120 insertions(+), 26 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/077fe576/src/kudu/consensus/raft_consensus.cc
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/raft_consensus.cc b/src/kudu/consensus/raft_consensus.cc
index c70abd9..f81637e 100644
--- a/src/kudu/consensus/raft_consensus.cc
+++ b/src/kudu/consensus/raft_consensus.cc
@@ -1320,6 +1320,16 @@ Status RaftConsensus::UpdateReplica(const ConsensusRequestPB* request,
 
     last_leader_communication_time_micros_ = GetMonoTimeMicros();
 
+    // Reset the 'failed_elections_since_stable_leader' metric now that we've
+    // accepted an update from the established leader. This is done in addition
+    // to the reset of the value in SetLeaderUuidUnlocked() because there is
+    // a potential race between resetting the failed elections count in
+    // SetLeaderUuidUnlocked() and incrementing after a failed election
+    // if another replica was elected leader in an election concurrent with
+    // the one called by this replica.
+    failed_elections_since_stable_leader_ = 0;
+    num_failed_elections_metric_->set_value(failed_elections_since_stable_leader_);
+
     // We update the lag metrics here in addition to after appending to the queue so the
     // metrics get updated even when the operation is rejected.
     queue_->UpdateLastIndexAppendedToLeader(request->last_idx_appended_to_leader());

http://git-wip-us.apache.org/repos/asf/kudu/blob/077fe576/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 d1425d0..f7be807 100644
--- a/src/kudu/integration-tests/raft_consensus-itest.cc
+++ b/src/kudu/integration-tests/raft_consensus-itest.cc
@@ -2670,61 +2670,145 @@ int64_t GetTimeSinceLastLeaderHeartbeat(const ExternalTabletServer*
ets,
   return ret;
 }
 
-TEST_F(RaftConsensusITest, TestElectionMetrics) {
+// Test for election-related metrics with the leader failure detection disabled.
+// This avoids inadvertent leader elections that might race with the tests
+// of the metric's behavior.
+TEST_F(RaftConsensusITest, TestElectionMetricsFailureDetectionDisabled) {
+  constexpr auto kNumReplicas = 3;
+  constexpr auto kNumTservers = 3;
   const vector<string> kTsFlags = {
     // Make leader elections faster so we can test
     // failed_elections_since_stable_leader faster.
     "--raft_heartbeat_interval_ms=100",
+
+    // For stability reasons, this scenario uses the 'manual election' mode
+    // and assumes no leader change before restarting tablet servers later on.
+    "--enable_leader_failure_detection=false",
+  };
+  const vector<string> kMasterFlags = {
+    // Corresponding flag for the tserver's '--enable_leader_failure_detection'.
+    "--catalog_manager_wait_for_new_tablets_to_elect_leader=false",
   };
+  const auto kTimeout = MonoDelta::FromSeconds(30);
 
-  FLAGS_num_replicas = 3;
-  FLAGS_num_tablet_servers = 3;
-  NO_FATALS(BuildAndStart(kTsFlags));
+  FLAGS_num_replicas = kNumReplicas;
+  FLAGS_num_tablet_servers = kNumTservers;
+  NO_FATALS(BuildAndStart(kTsFlags, kMasterFlags));
 
   vector<TServerDetails*> tservers;
   AppendValuesFromMap(tablet_servers_, &tservers);
 
-  ASSERT_OK(StartElection(tservers[0], tablet_id_, MonoDelta::FromSeconds(10)));
-  ASSERT_OK(WaitForServersToAgree(MonoDelta::FromSeconds(10), tablet_servers_, tablet_id_,
1));
+  const auto* leader = tablet_servers_.begin()->second;
+  const auto& leader_uuid = leader->uuid();
+  ASSERT_EVENTUALLY([&]() {
+    const auto kLeaderTimeout = MonoDelta::FromSeconds(10);
+    ASSERT_OK(StartElection(leader, tablet_id_, kLeaderTimeout));
+    TServerDetails* elected_leader;
+    ASSERT_OK(WaitForLeaderWithCommittedOp(
+        tablet_id_, kLeaderTimeout, &elected_leader));
+    ASSERT_EQ(leader->uuid(), elected_leader->uuid());
+  });
+  ASSERT_OK(WaitForServersToAgree(kTimeout, tablet_servers_, tablet_id_, 1));
 
-  int num_tablet_servers = cluster_->num_tablet_servers();
-  int leader_idx = cluster_->tablet_server_index_by_uuid(tservers[0]->uuid());
-  ExternalTabletServer* leader = cluster_->tablet_server(leader_idx);
-  ExternalTabletServer* follower = cluster_->tablet_server((leader_idx + 1) % num_tablet_servers);
+  auto* leader_srv = cluster_->tablet_server_by_uuid(leader_uuid);
+  auto* follower_srv = cluster_->tablet_server(
+      (cluster_->tablet_server_index_by_uuid(leader_uuid) + 1) % kNumTservers);
 
   // Leader should always report 0 since last leader heartbeat.
-  ASSERT_EQ(0, GetTimeSinceLastLeaderHeartbeat(leader, tablet_id_));
-  ASSERT_EQ(0, GetFailedElectionsSinceStableLeader(leader, tablet_id_));
+  EXPECT_EQ(0, GetTimeSinceLastLeaderHeartbeat(leader_srv, tablet_id_));
+  EXPECT_EQ(0, GetFailedElectionsSinceStableLeader(leader_srv, tablet_id_));
 
-  // Let's shut down all tablet servers except our chosen follower to make sure we don't
-  // have a leader.
-  for (auto i = 0; i < num_tablet_servers; ++i) {
-    if (cluster_->tablet_server(i) != follower) {
+  // Let's shut down all tablet servers except our chosen follower to make sure
+  // we don't have a leader.
+  for (auto i = 0; i < kNumTservers; ++i) {
+    if (cluster_->tablet_server(i) != follower_srv) {
       cluster_->tablet_server(i)->Shutdown();
     }
   }
 
   // Get two measurements with 500 ms sleep between them and see if the
   // difference between them is at least 500ms.
-  int64_t time_before_wait = GetTimeSinceLastLeaderHeartbeat(follower, tablet_id_);
+  const int64_t time_before_wait = GetTimeSinceLastLeaderHeartbeat(follower_srv,
+                                                                   tablet_id_);
   SleepFor(MonoDelta::FromMilliseconds(500));
-  int64_t time_after_wait = GetTimeSinceLastLeaderHeartbeat(follower, tablet_id_);
-  ASSERT_TRUE(time_after_wait >= time_before_wait + 500);
+  const int64_t time_after_wait = GetTimeSinceLastLeaderHeartbeat(follower_srv,
+                                                                  tablet_id_);
+  ASSERT_TRUE(time_after_wait >= time_before_wait + 500)
+      << "time_before_wait: " << time_before_wait << "; "
+      << "time_after_wait: " << time_after_wait;
+
+  // The follower doesn't start elections on itself because of
+  // --enable_leader_failure_detection=false flag, so the metric should read 0.
+  ASSERT_EQ(0, GetFailedElectionsSinceStableLeader(follower_srv, tablet_id_));
+}
+
+// Test for election-related metrics with the leader failure detection enabled.
+TEST_F(RaftConsensusITest, TestElectionMetricsFailureDetectionEnabled) {
+  constexpr auto kNumReplicas = 3;
+  constexpr auto kNumTservers = 3;
+  const vector<string> kTsFlags = {
+    // Make leader elections faster so we can test
+    // failed_elections_since_stable_leader faster.
+    "--raft_heartbeat_interval_ms=100",
+
+    // For the stability of the test, make leader election less likely in case
+    // of intermittent network failures and latency spikes. Otherwise, in case
+    // of on-going re-elections the metric value would not stabilize for a long
+    // time.
+    "--leader_failure_max_missed_heartbeat_periods=20",
+  };
+  const auto kTimeout = MonoDelta::FromSeconds(30);
+
+  FLAGS_num_replicas = kNumReplicas;
+  FLAGS_num_tablet_servers = kNumTservers;
+  NO_FATALS(BuildAndStart(kTsFlags));
+  ASSERT_OK(WaitForServersToAgree(kTimeout, tablet_servers_, tablet_id_, 1));
+
+  // Verify failed_elections_since_stable_leader is reset to 0 in the majority
+  // of all replicas after the cluster is in stable state, i.e. the leader
+  // is elected and functional.
+  ASSERT_EVENTUALLY([&]() {
+    for (auto i = 0; i < kNumTservers; ++i) {
+      ASSERT_EQ(0, GetFailedElectionsSinceStableLeader(
+          cluster_->tablet_server(i), tablet_id_));
+    }
+  });
+
+  auto* srv_ts = tablet_servers_.begin()->second;
+  auto* srv_ext = cluster_->tablet_server_by_uuid(srv_ts->uuid());
+  for (auto i = 0; i < kNumTservers; ++i) {
+    auto* s = cluster_->tablet_server(i);
+    if (s == srv_ext) {
+      continue;
+    }
+    s->Shutdown();
+  }
+
+  // If the server which is left alive hosts the leader replica for the tablet,
+  // make it step down. Otherwise, it's a no-op.
+  ignore_result(LeaderStepDown(srv_ts, tablet_id_, kTimeout));
 
   // Verify failed_elections_since_stable_leader is advanced eventually.
   ASSERT_EVENTUALLY([&]() {
-    ASSERT_TRUE(GetFailedElectionsSinceStableLeader(follower, tablet_id_) > 0);
+    ASSERT_GT(GetFailedElectionsSinceStableLeader(srv_ext, tablet_id_), 0);
   });
 
-  // Start the servers back up and verify failed_elections_since_stable_leader
-  // is reset to 0.
-  for (auto i = 0; i < num_tablet_servers; ++i) {
-    if (cluster_->tablet_server(i) != follower) {
-      cluster_->tablet_server(i)->Start();
+  // Restart the rest of tablet servers.
+  for (auto i = 0; i < kNumTservers; ++i) {
+    auto* s = cluster_->tablet_server(i);
+    if (s == srv_ext) {
+      continue;
     }
+    ASSERT_OK(s->Restart());
   }
+  ASSERT_OK(WaitForServersToAgree(kTimeout, tablet_servers_, tablet_id_, 1));
+
+  // Verify failed_elections_since_stable_leader is reset to 0 eventually.
   ASSERT_EVENTUALLY([&]() {
-    ASSERT_EQ(0, GetFailedElectionsSinceStableLeader(follower, tablet_id_));
+    for (auto i = 0; i < kNumTservers; ++i) {
+      ASSERT_EQ(0, GetFailedElectionsSinceStableLeader(
+          cluster_->tablet_server(i), tablet_id_));
+    }
   });
 }
 


Mime
View raw message