Repository: kudu
Updated Branches:
  refs/heads/master c61ed9b3f -> 84e0f3033


KUDU-2274. RaftConsensus should not access cmeta when shutdown

An additional case of KUDU-2274 found during stress testing was that
RaftConsensus will return a ConsensusStatePB from the ConsensusState()
RPC method even when shutdown. This patch prevents that.

Change-Id: Ib3f3f75674d5739b281e7c689ddb2d433b9b7415
Reviewed-on: http://gerrit.cloudera.org:8080/9266
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <aser...@cloudera.com>


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

Branch: refs/heads/master
Commit: d977d1cf16d5b8e82d84396a0a82351582258fa1
Parents: c61ed9b
Author: Mike Percy <mpe...@apache.org>
Authored: Thu Feb 8 21:34:08 2018 -0800
Committer: Mike Percy <mpe...@apache.org>
Committed: Mon Feb 12 23:29:07 2018 +0000

----------------------------------------------------------------------
 src/kudu/consensus/raft_consensus.cc            | 13 ++++++----
 src/kudu/consensus/raft_consensus.h             |  5 +++-
 .../ts_tablet_manager-itest.cc                  |  9 +++++--
 src/kudu/master/catalog_manager.cc              | 25 +++++++++++++-------
 src/kudu/master/sys_catalog.cc                  |  8 ++++++-
 src/kudu/tserver/tablet_copy_client-test.cc     |  3 ++-
 src/kudu/tserver/tablet_copy_source_session.cc  |  4 ++--
 src/kudu/tserver/tablet_server-test.cc          | 20 ++++++++++++++++
 src/kudu/tserver/tablet_service.cc              | 12 ++++++----
 src/kudu/tserver/ts_tablet_manager.cc           |  7 +++++-
 src/kudu/tserver/tserver_path_handlers.cc       | 10 +++++---
 11 files changed, 88 insertions(+), 28 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/d977d1cf/src/kudu/consensus/raft_consensus.cc
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/raft_consensus.cc 
b/src/kudu/consensus/raft_consensus.cc
index 98069d7..7a2017a 100644
--- a/src/kudu/consensus/raft_consensus.cc
+++ b/src/kudu/consensus/raft_consensus.cc
@@ -2333,10 +2333,14 @@ const string& RaftConsensus::tablet_id() const {
   return options_.tablet_id;
 }
 
-ConsensusStatePB RaftConsensus::ConsensusState(IncludeHealthReport 
report_health) const {
+Status RaftConsensus::ConsensusState(ConsensusStatePB* cstate,
+                                     IncludeHealthReport report_health) const {
   ThreadRestrictions::AssertWaitAllowed();
   UniqueLock l(lock_);
-  ConsensusStatePB cstate = cmeta_->ToConsensusStatePB();
+  if (state_ == kShutdown) {
+    return Status::IllegalState("Tablet replica is shutdown");
+  }
+  ConsensusStatePB cstate_tmp = cmeta_->ToConsensusStatePB();
 
   // If we need to include the health report, merge it into the committed
   // config iff we believe we are the current leader of the config.
@@ -2349,7 +2353,7 @@ ConsensusStatePB 
RaftConsensus::ConsensusState(IncludeHealthReport report_health
 
     // Iterate through each peer in the committed config and attach the health
     // report to it.
-    RaftConfigPB* committed_raft_config = cstate.mutable_committed_config();
+    RaftConfigPB* committed_raft_config = 
cstate_tmp.mutable_committed_config();
     for (int i = 0; i < committed_raft_config->peers_size(); i++) {
       RaftPeerPB* peer = committed_raft_config->mutable_peers(i);
       const HealthReportPB* report = FindOrNull(reports, 
peer->permanent_uuid());
@@ -2357,7 +2361,8 @@ ConsensusStatePB 
RaftConsensus::ConsensusState(IncludeHealthReport report_health
       *peer->mutable_health_report() = *report;
     }
   }
-  return cstate;
+  *cstate = std::move(cstate_tmp);
+  return Status::OK();
 }
 
 RaftConfigPB RaftConsensus::CommittedConfig() const {

http://git-wip-us.apache.org/repos/asf/kudu/blob/d977d1cf/src/kudu/consensus/raft_consensus.h
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/raft_consensus.h 
b/src/kudu/consensus/raft_consensus.h
index 81aa0a7..f77749f 100644
--- a/src/kudu/consensus/raft_consensus.h
+++ b/src/kudu/consensus/raft_consensus.h
@@ -296,7 +296,10 @@ class RaftConsensus : public 
std::enable_shared_from_this<RaftConsensus>,
   // If 'report_health' is set to 'INCLUDE_HEALTH_REPORT', and if the
   // local replica believes it is the leader of the config, it will include a
   // health report about each active peer in the committed config.
-  ConsensusStatePB ConsensusState(IncludeHealthReport report_health = 
EXCLUDE_HEALTH_REPORT) const;
+  // If RaftConsensus has been shut down, returns Status::IllegalState.
+  // Does not modify the out-param 'cstate' unless an OK status is returned.
+  Status ConsensusState(ConsensusStatePB* cstate,
+                        IncludeHealthReport report_health = 
EXCLUDE_HEALTH_REPORT) const;
 
   // Returns a copy of the current committed Raft configuration.
   RaftConfigPB CommittedConfig() const;

http://git-wip-us.apache.org/repos/asf/kudu/blob/d977d1cf/src/kudu/integration-tests/ts_tablet_manager-itest.cc
----------------------------------------------------------------------
diff --git a/src/kudu/integration-tests/ts_tablet_manager-itest.cc 
b/src/kudu/integration-tests/ts_tablet_manager-itest.cc
index c4ff736..e56d016 100644
--- a/src/kudu/integration-tests/ts_tablet_manager-itest.cc
+++ b/src/kudu/integration-tests/ts_tablet_manager-itest.cc
@@ -407,11 +407,16 @@ TEST_F(TsTabletManagerITest, ReportOnReplicaHealthStatus) 
{
     string leader_uuid;
     for (const auto& replica : tablet_replicas) {
       RaftConsensus* consensus = CHECK_NOTNULL(replica->consensus());
-      ConsensusStatePB 
cs(consensus->ConsensusState(RaftConsensus::INCLUDE_HEALTH_REPORT));
+      ConsensusStatePB cs;
+      Status s = consensus->ConsensusState(&cs, 
RaftConsensus::INCLUDE_HEALTH_REPORT);
+      if (!s.ok()) {
+        ASSERT_TRUE(s.IsIllegalState()) << s.ToString(); // Replica is shut 
down.
+        continue;
+      }
       if (consensus->peer_uuid() == cs.leader_uuid()) {
         // Only the leader replica has the up-to-date health report.
         leader_uuid = cs.leader_uuid();
-        cstate.Swap(&cs);
+        cstate = std::move(cs);
         break;
       }
     }

http://git-wip-us.apache.org/repos/asf/kudu/blob/d977d1cf/src/kudu/master/catalog_manager.cc
----------------------------------------------------------------------
diff --git a/src/kudu/master/catalog_manager.cc 
b/src/kudu/master/catalog_manager.cc
index ec01f18..ba1d894 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -693,7 +693,8 @@ Status CatalogManager::ElectedAsLeaderCb() {
 }
 
 Status CatalogManager::WaitUntilCaughtUpAsLeader(const MonoDelta& timeout) {
-  ConsensusStatePB cstate = 
sys_catalog_->tablet_replica()->consensus()->ConsensusState();
+  ConsensusStatePB cstate;
+  
RETURN_NOT_OK(sys_catalog_->tablet_replica()->consensus()->ConsensusState(&cstate));
   const string& uuid = master_->fs_manager()->uuid();
   if (cstate.leader_uuid() != uuid) {
     return Status::IllegalState(
@@ -878,7 +879,7 @@ void CatalogManager::PrepareForLeadershipTask() {
     shared_lock<LockType> l(lock_);
   }
   const RaftConsensus* consensus = sys_catalog_->tablet_replica()->consensus();
-  const int64_t term_before_wait = consensus->ConsensusState().current_term();
+  const int64_t term_before_wait = consensus->CurrentTerm();
   {
     std::lock_guard<simple_spinlock> l(state_lock_);
     if (leader_ready_term_ == term_before_wait) {
@@ -903,7 +904,7 @@ void CatalogManager::PrepareForLeadershipTask() {
     return;
   }
 
-  const int64_t term = consensus->ConsensusState().current_term();
+  const int64_t term = consensus->CurrentTerm();
   if (term_before_wait != term) {
     // If we got elected leader again while waiting to catch up then we will
     // get another callback to visit the tables and tablets, so bail.
@@ -941,7 +942,7 @@ void CatalogManager::PrepareForLeadershipTask() {
         }
       }
 
-      const int64_t term = consensus.ConsensusState().current_term();
+      const int64_t term = consensus.CurrentTerm();
       if (term != start_term) {
         // If the term has changed we assume the new leader catalog is about
         // to do the necessary work in its leadership preparation task.
@@ -4357,11 +4358,18 @@ 
CatalogManager::ScopedLeaderSharedLock::ScopedLeaderSharedLock(
                    catalog_->state_));
     return;
   }
+
+  ConsensusStatePB cstate;
+  Status s = 
catalog_->sys_catalog_->tablet_replica()->consensus()->ConsensusState(&cstate);
+  if (PREDICT_FALSE(!s.ok())) {
+    DCHECK(s.IsIllegalState()) << s.ToString();
+    catalog_status_ = s.CloneAndPrepend("ConsensusState is not available");
+    return;
+  }
+
   catalog_status_ = Status::OK();
 
   // Check if the catalog manager is the leader.
-  const ConsensusStatePB cstate = catalog_->sys_catalog_->tablet_replica()->
-      consensus()->ConsensusState();
   initial_term_ = cstate.current_term();
   const string& uuid = catalog_->master_->fs_manager()->uuid();
   if (PREDICT_FALSE(cstate.leader_uuid() != uuid)) {
@@ -4381,9 +4389,8 @@ 
CatalogManager::ScopedLeaderSharedLock::ScopedLeaderSharedLock(
 
 bool CatalogManager::ScopedLeaderSharedLock::has_term_changed() const {
   DCHECK(leader_status().ok());
-  const ConsensusStatePB cstate = catalog_->sys_catalog_->tablet_replica()->
-      consensus()->ConsensusState();
-  return cstate.current_term() != initial_term_;
+  const auto current_term = 
catalog_->sys_catalog_->tablet_replica()->consensus()->CurrentTerm();
+  return current_term != initial_term_;
 }
 
 template<typename RespClass>

http://git-wip-us.apache.org/repos/asf/kudu/blob/d977d1cf/src/kudu/master/sys_catalog.cc
----------------------------------------------------------------------
diff --git a/src/kudu/master/sys_catalog.cc b/src/kudu/master/sys_catalog.cc
index 7836380..355b8f6 100644
--- a/src/kudu/master/sys_catalog.cc
+++ b/src/kudu/master/sys_catalog.cc
@@ -60,6 +60,7 @@
 #include "kudu/gutil/bind_helpers.h"
 #include "kudu/gutil/gscoped_ptr.h"
 #include "kudu/gutil/move.h"
+#include "kudu/gutil/port.h"
 #include "kudu/gutil/strings/join.h"
 #include "kudu/gutil/strings/substitute.h"
 #include "kudu/master/catalog_manager.h"
@@ -325,7 +326,12 @@ void SysCatalogTable::SysCatalogStateChanged(const string& 
tablet_id, const stri
                              << tablet_id << ". Reason: " << reason;
     return;
   }
-  consensus::ConsensusStatePB cstate = consensus->ConsensusState();
+  consensus::ConsensusStatePB cstate;
+  Status s = consensus->ConsensusState(&cstate);
+  if (PREDICT_FALSE(!s.ok())) {
+    LOG_WITH_PREFIX(WARNING) << s.ToString();
+    return;
+  }
   LOG_WITH_PREFIX(INFO) << "SysCatalogTable state changed. Reason: " << reason 
<< ". "
                         << "Latest consensus state: " << 
SecureShortDebugString(cstate);
   RaftPeerPB::Role new_role = 
GetConsensusRole(tablet_replica_->permanent_uuid(), cstate);

http://git-wip-us.apache.org/repos/asf/kudu/blob/d977d1cf/src/kudu/tserver/tablet_copy_client-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/tablet_copy_client-test.cc 
b/src/kudu/tserver/tablet_copy_client-test.cc
index cc5b695..de5d623 100644
--- a/src/kudu/tserver/tablet_copy_client-test.cc
+++ b/src/kudu/tserver/tablet_copy_client-test.cc
@@ -118,7 +118,8 @@ class TabletCopyClientTest : public TabletCopyTest {
                                        messenger_,
                                        nullptr /* no metrics */));
     RaftPeerPB* cstate_leader;
-    ConsensusStatePB cstate = tablet_replica_->consensus()->ConsensusState();
+    ConsensusStatePB cstate;
+    ASSERT_OK(tablet_replica_->consensus()->ConsensusState(&cstate));
     ASSERT_OK(GetRaftConfigLeader(&cstate, &cstate_leader));
     leader_ = *cstate_leader;
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/d977d1cf/src/kudu/tserver/tablet_copy_source_session.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/tablet_copy_source_session.cc 
b/src/kudu/tserver/tablet_copy_source_session.cc
index e2a5227..18bb5fb 100644
--- a/src/kudu/tserver/tablet_copy_source_session.cc
+++ b/src/kudu/tserver/tablet_copy_source_session.cc
@@ -29,7 +29,6 @@
 #include "kudu/consensus/log.h"
 #include "kudu/consensus/log.pb.h"
 #include "kudu/consensus/log_reader.h"
-#include "kudu/consensus/metadata.pb.h"
 #include "kudu/consensus/opid.pb.h"
 #include "kudu/consensus/opid_util.h"
 #include "kudu/consensus/raft_consensus.h"
@@ -189,7 +188,8 @@ Status TabletCopySourceSession::InitOnce() {
         "Raft Consensus is not available. Tablet state: $1 ($2)",
         tablet_id, tablet::TabletStatePB_Name(tablet_state), tablet_state));
   }
-  initial_cstate_ = consensus->ConsensusState();
+  RETURN_NOT_OK_PREPEND(consensus->ConsensusState(&initial_cstate_),
+                        "Consensus state not available");
 
   // Re-anchor on the highest OpId that was in the log right before we
   // snapshotted the log segments. This helps ensure that we don't end up in a

http://git-wip-us.apache.org/repos/asf/kudu/blob/d977d1cf/src/kudu/tserver/tablet_server-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/tablet_server-test.cc 
b/src/kudu/tserver/tablet_server-test.cc
index 73b0b1d..a87b03f 100644
--- a/src/kudu/tserver/tablet_server-test.cc
+++ b/src/kudu/tserver/tablet_server-test.cc
@@ -52,6 +52,7 @@
 #include "kudu/consensus/log-test-base.h"
 #include "kudu/consensus/log.h"
 #include "kudu/consensus/metadata.pb.h"
+#include "kudu/consensus/raft_consensus.h"
 #include "kudu/fs/block_id.h"
 #include "kudu/fs/fs-test-util.h"
 #include "kudu/fs/fs.pb.h"
@@ -110,6 +111,7 @@
 using google::protobuf::util::MessageDifferencer;
 using kudu::clock::Clock;
 using kudu::clock::HybridClock;
+using kudu::consensus::ConsensusStatePB;
 using kudu::fs::CreateCorruptBlock;
 using kudu::pb_util::SecureDebugString;
 using kudu::pb_util::SecureShortDebugString;
@@ -412,6 +414,24 @@ TEST_F(TabletServerTest, TestWebPages) {
 #endif
 }
 
+// Ensure that when a replica is in a failed / shutdown state, it returns an
+// error for ConsensusState() requests.
+TEST_F(TabletServerTest, TestFailedTabletsRejectConsensusState) {
+  scoped_refptr<TabletReplica> replica;
+  TSTabletManager* tablet_manager = mini_server_->server()->tablet_manager();
+  ASSERT_TRUE(tablet_manager->LookupTablet(kTabletId, &replica));
+  replica->SetError(Status::IOError("This error will leave the replica FAILED 
state at shutdown"));
+  replica->Shutdown();
+  ASSERT_EQ(tablet::FAILED, replica->state());
+
+  auto consensus = replica->shared_consensus();
+  ASSERT_TRUE(consensus);
+  ConsensusStatePB cstate;
+  Status s = consensus->ConsensusState(&cstate);
+  ASSERT_TRUE(s.IsIllegalState()) << s.ToString();
+  ASSERT_STR_CONTAINS(s.ToString(), "Tablet replica is shutdown");
+}
+
 // Test that tablets that get failed and deleted will eventually show up as
 // failed tombstones on the web UI.
 TEST_F(TabletServerTest, TestFailedTabletsOnWebUI) {

http://git-wip-us.apache.org/repos/asf/kudu/blob/d977d1cf/src/kudu/tserver/tablet_service.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/tablet_service.cc 
b/src/kudu/tserver/tablet_service.cc
index 1294491..f5fc72f 100644
--- a/src/kudu/tserver/tablet_service.cc
+++ b/src/kudu/tserver/tablet_service.cc
@@ -50,7 +50,6 @@
 #include "kudu/common/wire_protocol.h"
 #include "kudu/common/wire_protocol.pb.h"
 #include "kudu/consensus/consensus.pb.h"
-#include "kudu/consensus/metadata.pb.h"
 #include "kudu/consensus/opid.pb.h"
 #include "kudu/consensus/raft_consensus.h"
 #include "kudu/consensus/replica_management.pb.h"
@@ -1215,9 +1214,14 @@ void ConsensusServiceImpl::GetConsensusState(const 
consensus::GetConsensusStateR
       continue;
     }
 
-    auto* tablet_info = resp->add_tablets();
-    tablet_info->set_tablet_id(replica->tablet_id());
-    *tablet_info->mutable_cstate() = consensus->ConsensusState();
+    consensus::GetConsensusStateResponsePB_TabletConsensusInfoPB tablet_info;
+    Status s = consensus->ConsensusState(tablet_info.mutable_cstate());
+    if (!s.ok()) {
+      DCHECK(s.IsIllegalState()) << s.ToString();
+      continue;
+    }
+    tablet_info.set_tablet_id(replica->tablet_id());
+    *resp->add_tablets() = std::move(tablet_info);
   }
   const auto scheme = FLAGS_raft_prepare_replacement_before_eviction
       ? consensus::ReplicaManagementInfoPB::PREPARE_REPLACEMENT_BEFORE_EVICTION

http://git-wip-us.apache.org/repos/asf/kudu/blob/d977d1cf/src/kudu/tserver/ts_tablet_manager.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/ts_tablet_manager.cc 
b/src/kudu/tserver/ts_tablet_manager.cc
index e96c7cf..8325d67 100644
--- a/src/kudu/tserver/ts_tablet_manager.cc
+++ b/src/kudu/tserver/ts_tablet_manager.cc
@@ -172,6 +172,7 @@ namespace kudu {
 using consensus::ConsensusMetadata;
 using consensus::ConsensusMetadataCreateMode;
 using consensus::ConsensusMetadataManager;
+using consensus::ConsensusStatePB;
 using consensus::OpId;
 using consensus::OpIdToString;
 using consensus::RECEIVED_OPID;
@@ -1157,7 +1158,11 @@ void TSTabletManager::CreateReportedTabletPB(const 
scoped_refptr<TabletReplica>&
     auto include_health = FLAGS_raft_prepare_replacement_before_eviction ?
                           RaftConsensus::INCLUDE_HEALTH_REPORT :
                           RaftConsensus::EXCLUDE_HEALTH_REPORT;
-    *reported_tablet->mutable_consensus_state() = 
consensus->ConsensusState(include_health);
+    ConsensusStatePB cstate;
+    Status s = consensus->ConsensusState(&cstate, include_health);
+    if (PREDICT_TRUE(s.ok())) {
+      *reported_tablet->mutable_consensus_state() = std::move(cstate);
+    }
   }
 }
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/d977d1cf/src/kudu/tserver/tserver_path_handlers.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/tserver_path_handlers.cc 
b/src/kudu/tserver/tserver_path_handlers.cc
index ae4aa51..9723b4a 100644
--- a/src/kudu/tserver/tserver_path_handlers.cc
+++ b/src/kudu/tserver/tserver_path_handlers.cc
@@ -287,10 +287,14 @@ void TabletServerPathHandlers::HandleTabletsPage(const 
Webserver::WebRequest& /*
                                                        
replica->tablet_metadata()->schema());
 
       // We don't show the config if it's a tombstone because it's misleading.
+      string consensus_state_html;
       shared_ptr<consensus::RaftConsensus> consensus = 
replica->shared_consensus();
-      string consensus_state_html =
-          consensus && !IsTombstoned(replica) ? 
ConsensusStatePBToHtml(consensus->ConsensusState())
-                                              : "";
+      if (!IsTombstoned(replica) && consensus) {
+        ConsensusStatePB cstate;
+        if (consensus->ConsensusState(&cstate).ok()) {
+          consensus_state_html = ConsensusStatePBToHtml(cstate);
+        }
+      }
 
       *output << Substitute(
           // Table name, tablet id, partition

Reply via email to