kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ale...@apache.org
Subject [1/3] kudu git commit: KUDU-2274. RaftConsensus should not access cmeta when shutdown
Date Fri, 09 Mar 2018 22:38:19 GMT
Repository: kudu
Updated Branches:
  refs/heads/branch-1.6.x e8c68e6c0 -> 3aa933191


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.

Conflicts:
  src/kudu/tablet/tablet_replica.h
  src/kudu/tserver/tserver_path_handlers.cc

Change-Id: Ib3f3f75674d5739b281e7c689ddb2d433b9b7415
Reviewed-on: http://gerrit.cloudera.org:8080/9266
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <aserbin@cloudera.com>
(cherry picked from commit d977d1cf16d5b8e82d84396a0a82351582258fa1)
Reviewed-on: http://gerrit.cloudera.org:8080/9548
Reviewed-by: Todd Lipcon <todd@apache.org>


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

Branch: refs/heads/branch-1.6.x
Commit: e605c16235c04a7aaeb0432ab0487ab3d1203728
Parents: e8c68e6
Author: Mike Percy <mpercy@apache.org>
Authored: Thu Feb 8 21:34:08 2018 -0800
Committer: Alexey Serbin <aserbin@cloudera.com>
Committed: Fri Mar 9 20:05:57 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/tablet/tablet_replica.h                |  5 ++++
 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       | 15 +++++++++++-
 12 files changed, 100 insertions(+), 26 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/e605c162/src/kudu/consensus/raft_consensus.cc
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/raft_consensus.cc b/src/kudu/consensus/raft_consensus.cc
index cb3118e..3def94b 100644
--- a/src/kudu/consensus/raft_consensus.cc
+++ b/src/kudu/consensus/raft_consensus.cc
@@ -2326,10 +2326,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.
@@ -2342,7 +2346,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());
@@ -2350,7 +2354,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/e605c162/src/kudu/consensus/raft_consensus.h
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/raft_consensus.h b/src/kudu/consensus/raft_consensus.h
index 5ace791..bf44e83 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/e605c162/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 ee5a957..59c85a0 100644
--- a/src/kudu/integration-tests/ts_tablet_manager-itest.cc
+++ b/src/kudu/integration-tests/ts_tablet_manager-itest.cc
@@ -375,11 +375,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/e605c162/src/kudu/master/catalog_manager.cc
----------------------------------------------------------------------
diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc
index acea922..b2b7c4e 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -675,7 +675,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(
@@ -858,7 +859,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) {
@@ -883,7 +884,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.
@@ -921,7 +922,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.
@@ -4283,11 +4284,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)) {
@@ -4307,9 +4315,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/e605c162/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/e605c162/src/kudu/tablet/tablet_replica.h
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/tablet_replica.h b/src/kudu/tablet/tablet_replica.h
index bf3476d..c2f72ba 100644
--- a/src/kudu/tablet/tablet_replica.h
+++ b/src/kudu/tablet/tablet_replica.h
@@ -185,6 +185,11 @@ class TabletReplica : public RefCountedThreadSafe<TabletReplica>,
     return state_;
   }
 
+  const TabletDataState data_state() const {
+    std::lock_guard<simple_spinlock> lock(lock_);
+    return meta_->tablet_data_state();
+  }
+
   std::string StateName() const;
 
   // Returns the current Raft configuration.

http://git-wip-us.apache.org/repos/asf/kudu/blob/e605c162/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 7ac12cd..7f3b025 100644
--- a/src/kudu/tserver/tablet_copy_client-test.cc
+++ b/src/kudu/tserver/tablet_copy_client-test.cc
@@ -116,7 +116,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/e605c162/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/e605c162/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 de498dc..c936aea 100644
--- a/src/kudu/tserver/tablet_server-test.cc
+++ b/src/kudu/tserver/tablet_server-test.cc
@@ -51,6 +51,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"
@@ -107,6 +108,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;
@@ -359,6 +361,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/e605c162/src/kudu/tserver/tablet_service.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/tablet_service.cc b/src/kudu/tserver/tablet_service.cc
index 22753d8..dc89823 100644
--- a/src/kudu/tserver/tablet_service.cc
+++ b/src/kudu/tserver/tablet_service.cc
@@ -49,7 +49,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/time_manager.h"
@@ -1212,9 +1211,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);
   }
 
   context->RespondSuccess();

http://git-wip-us.apache.org/repos/asf/kudu/blob/e605c162/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 bef2d9d..4d2558b 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;
@@ -1159,7 +1160,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/e605c162/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 3dd602f..514ce14 100644
--- a/src/kudu/tserver/tserver_path_handlers.cc
+++ b/src/kudu/tserver/tserver_path_handlers.cc
@@ -211,6 +211,10 @@ string TabletLink(const string& id) {
                     EscapeForHtmlToString(id));
 }
 
+bool IsTombstoned(const scoped_refptr<TabletReplica>& replica) {
+  return replica->data_state() == tablet::TABLET_DATA_TOMBSTONED;
+}
+
 } // anonymous namespace
 
 void TabletServerPathHandlers::HandleTabletsPage(const Webserver::WebRequest& /*req*/,
@@ -286,7 +290,16 @@ void TabletServerPathHandlers::HandleTabletsPage(const Webserver::WebRequest&
/*
                                  .PartitionDebugString(replica->tablet_metadata()->partition(),
                                                        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();
+      if (!IsTombstoned(replica) && consensus) {
+        ConsensusStatePB cstate;
+        if (consensus->ConsensusState(&cstate).ok()) {
+          consensus_state_html = ConsensusStatePBToHtml(cstate);
+        }
+      }
+
       *output << Substitute(
           // Table name, tablet id, partition
           "<tr><td>$0</td><td>$1</td><td>$2</td>"
@@ -296,7 +309,7 @@ void TabletServerPathHandlers::HandleTabletsPage(const Webserver::WebRequest&
/*
           tablet_id_or_link, // $1
           EscapeForHtmlToString(partition), // $2
           EscapeForHtmlToString(replica->HumanReadableState()), mem_bytes, n_bytes, //
$3, $4, $5
-          consensus ? ConsensusStatePBToHtml(consensus->ConsensusState()) : "", // $6
+          consensus_state_html, // $6
           EscapeForHtmlToString(status.last_status())); // $7
     }
     *output << "<tbody></table>\n</div>\n";


Mime
View raw message