kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From aw...@apache.org
Subject [kudu] 02/02: KUDU-2952: fix TOCTOU race in reporting stats
Date Tue, 24 Sep 2019 18:46:21 GMT
This is an automated email from the ASF dual-hosted git repository.

awong pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git

commit 4dcbcf1c95420fbc9f9dff72c49ed2c43c71eb6a
Author: Andrew Wong <awong@apache.org>
AuthorDate: Mon Sep 23 20:06:19 2019 -0700

    KUDU-2952: fix TOCTOU race in reporting stats
    
    It was possible for the following race to occur:
    
    During CreateReportedTabletPB() as a follower:
    1. T1 Get consensus state.
    2. T1 Serialize the ConsensusStatePB under lock -- we are not the
       leader.
    3. T2 Become the leader.
    4. T1 Check if we're leader so that we can generate stats. This check
          succeeds.
    5. T1 Populate the stats.
    6. Our tablet report now contains stats even though the consensus state
       says we aren't the leader.
    7. F0924 00:08:46.821594  9670 catalog_manager.cc:4239] Check failed: ts_desc->permanent_uuid()
== report.consensus_state().leader_uuid()
    
    This addresses this by checking leadership from the serialized consensus
    state and adds a test that would fail pretty consistently without this
    patch.
    
    Change-Id: I29fe63cb42c82a08a4811d42b244abeb2fa86bd9
    Reviewed-on: http://gerrit.cloudera.org:8080/14287
    Reviewed-by: Adar Dembo <adar@cloudera.com>
    Reviewed-by: Alexey Serbin <aserbin@cloudera.com>
    Tested-by: Kudu Jenkins
---
 .../integration-tests/ts_tablet_manager-itest.cc   | 41 +++++++++++++++++++++-
 src/kudu/tserver/ts_tablet_manager.cc              | 30 ++++++++--------
 src/kudu/tserver/ts_tablet_manager.h               |  1 +
 3 files changed, 55 insertions(+), 17 deletions(-)

diff --git a/src/kudu/integration-tests/ts_tablet_manager-itest.cc b/src/kudu/integration-tests/ts_tablet_manager-itest.cc
index d728d46..36300bc 100644
--- a/src/kudu/integration-tests/ts_tablet_manager-itest.cc
+++ b/src/kudu/integration-tests/ts_tablet_manager-itest.cc
@@ -379,7 +379,7 @@ TEST_F(LeadershipChangeReportingTest, TestConcurrentGetTableLocations)
{
   });
   SCOPED_CLEANUP({
     latch.CountDown();
-    NO_FATALS(t.join());
+    t.join();
   });
   for (int i = 0; i < FLAGS_num_election_test_loops; i++) {
     for (int t = 0; t < kNumTablets; t++) {
@@ -391,6 +391,45 @@ TEST_F(LeadershipChangeReportingTest, TestConcurrentGetTableLocations)
{
   }
 }
 
+// Regression test for KUDU-2952, where a leadership change racing with the
+// creation of a tablet report could lead to a non-leader report with stats.
+TEST_F(LeadershipChangeReportingTest, TestReportStatsDuringLeadershipChange) {
+  SKIP_IF_SLOW_NOT_ALLOWED();
+  ASSERT_OK(CreateTable(1));
+  // Stop heartbeating we don't race against the Master.
+  DisableHeartbeatingToMaster();
+
+  // Create a thread that just keeps triggering elections.
+  CountDownLatch latch(1);
+  thread t([&] {
+    while (!latch.WaitFor(MonoDelta::FromMilliseconds(1))) {
+      ignore_result(TriggerElection());
+    }
+  });
+  SCOPED_CLEANUP({
+    latch.CountDown(1);
+    t.join();
+  });
+
+  // Generate a bunch of reports and keep checking that only the leaders have
+  // stats.
+  const auto deadline = MonoTime::Now() + MonoDelta::FromSeconds(10);
+  while (MonoTime::Now() < deadline) {
+    for (int t = 0; t < cluster_->num_tablet_servers(); t++) {
+      ReportedTabletPB reported_tablet;
+      MiniTabletServer* mts = cluster_->mini_tablet_server(0);
+      mts->server()->tablet_manager()->CreateReportedTabletPB(tablet_replicas_[0],
+                                                              &reported_tablet);
+      // Only reports from leaders should have stats.
+      const string& reported_leader_uuid = reported_tablet.consensus_state().leader_uuid();
+      const string& mts_uuid = mts->uuid();
+      if (reported_tablet.has_stats()) {
+        ASSERT_EQ(reported_leader_uuid, mts_uuid);
+      }
+    }
+  }
+}
+
 // Test that when the leader changes, the tablet manager gets notified and
 // includes that information in the next tablet report.
 TEST_F(LeadershipChangeReportingTest, TestUpdatedConsensusState) {
diff --git a/src/kudu/tserver/ts_tablet_manager.cc b/src/kudu/tserver/ts_tablet_manager.cc
index 5f5acb8..97a970a 100644
--- a/src/kudu/tserver/ts_tablet_manager.cc
+++ b/src/kudu/tserver/ts_tablet_manager.cc
@@ -1311,24 +1311,22 @@ void TSTabletManager::CreateReportedTabletPB(const scoped_refptr<TabletReplica>&
 
   // We cannot get consensus state information unless the TabletReplica is running.
   shared_ptr<consensus::RaftConsensus> consensus = replica->shared_consensus();
-  if (consensus) {
-    auto include_health = FLAGS_raft_prepare_replacement_before_eviction ?
-                          INCLUDE_HEALTH_REPORT : EXCLUDE_HEALTH_REPORT;
-    ConsensusStatePB cstate;
-    Status s = consensus->ConsensusState(&cstate, include_health);
-    if (PREDICT_TRUE(s.ok())) {
-      *reported_tablet->mutable_consensus_state() = std::move(cstate);
-    }
+  if (!consensus) {
+    return;
   }
-
-  // First, the replica's state should be RUNNING already.
-  // Then fill in the replica's stats only when it's LEADER.
-  if (tablet::RUNNING == replica->state() &&
-      consensus::RaftPeerPB_Role_LEADER == consensus->role()) {
-    ReportedTabletStatsPB stats_pb = replica->GetTabletStats();
-    if (stats_pb.IsInitialized()) {
-      *reported_tablet->mutable_stats() = std::move(stats_pb);
+  auto include_health = FLAGS_raft_prepare_replacement_before_eviction ?
+                        INCLUDE_HEALTH_REPORT : EXCLUDE_HEALTH_REPORT;
+  ConsensusStatePB cstate;
+  Status s = consensus->ConsensusState(&cstate, include_health);
+  if (PREDICT_TRUE(s.ok())) {
+    // If we're the leader, report stats.
+    if (cstate.leader_uuid() == fs_manager_->uuid()) {
+      ReportedTabletStatsPB stats_pb = replica->GetTabletStats();
+      if (stats_pb.IsInitialized()) {
+        *reported_tablet->mutable_stats() = std::move(stats_pb);
+      }
     }
+    *reported_tablet->mutable_consensus_state() = std::move(cstate);
   }
 }
 
diff --git a/src/kudu/tserver/ts_tablet_manager.h b/src/kudu/tserver/ts_tablet_manager.h
index e303ad7..880fc86 100644
--- a/src/kudu/tserver/ts_tablet_manager.h
+++ b/src/kudu/tserver/ts_tablet_manager.h
@@ -242,6 +242,7 @@ class TSTabletManager : public tserver::TabletReplicaLookupIf {
   void UpdateTabletStatsIfNecessary();
 
  private:
+  FRIEND_TEST(LeadershipChangeReportingTest, TestReportStatsDuringLeadershipChange);
   FRIEND_TEST(TsTabletManagerTest, TestPersistBlocks);
   FRIEND_TEST(TsTabletManagerTest, TestTabletStatsReports);
   FRIEND_TEST(TsTabletManagerITest, TestTableStats);


Mime
View raw message