[kudu] branch master updated: [master] Fix validation for NON_VOTER on adding a master

[ba...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

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


The following commit(s) were added to refs/heads/master by this push:
     new f169738  [master] Fix validation for NON_VOTER on adding a master
f169738 is described below

commit f16973890c31042ec5770e2735708577b8192959
Author: Bankim Bhavsar <ba...@cloudera.com>
AuthorDate: Mon Feb 22 22:43:26 2021 -0800

    [master] Fix validation for NON_VOTER on adding a master
    
    When a master is added, validation check is made whether
    the supplied master is already part of the Raft configuration.
    However this check doesn't include NON_VOTER masters and hence
    an InvalidArgument error is thrown later by the ChangeConfig
    implementation in Raft. "master add" CLI is designed to specifically
    catch Status::AlreadyPresent error to allow retries.
    
    This change basically checks for all masters in the committed
    Raft config irrespective of the member type on adding a master.
    
    Change-Id: I10e6b3617b032c74ebed4359b10c36b7b365d9b7
    Reviewed-on: http://gerrit.cloudera.org:8080/17108
    Tested-by: Bankim Bhavsar <ba...@cloudera.com>
    Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
 src/kudu/master/dynamic_multi_master-test.cc | 14 ++++++++++++++
 src/kudu/master/master.cc                    | 17 +++++++++++++----
 src/kudu/master/master.h                     |  9 +++++++--
 3 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/src/kudu/master/dynamic_multi_master-test.cc b/src/kudu/master/dynamic_multi_master-test.cc
index 2bb6e46..cdf3a07 100644
--- a/src/kudu/master/dynamic_multi_master-test.cc
+++ b/src/kudu/master/dynamic_multi_master-test.cc
@@ -756,6 +756,20 @@ TEST_P(ParameterizedAddMasterTest, TestAddMasterSysCatalogCopy) {
     NO_FATALS(VerifyNewMasterInFailedUnrecoverableState());
   });
 
+  // Adding the same master again should print a message but not throw an error.
+  {
+    string err;
+    ASSERT_OK(AddMasterToClusterUsingCLITool(reserved_hp_, &err));
+    ASSERT_STR_CONTAINS(err, "Master already present");
+  }
+
+  // Adding one of the former masters should print a message but not throw an error.
+  {
+    string err;
+    ASSERT_OK(AddMasterToClusterUsingCLITool(master_hps[0], &err));
+    ASSERT_STR_CONTAINS(err, "Master already present");
+  }
+
   // Without system catalog copy, the new master will remain in the FAILED_UNRECOVERABLE state.
   // So lets proceed with the tablet copy process for system catalog.
   // Shutdown the new master
diff --git a/src/kudu/master/master.cc b/src/kudu/master/master.cc
index 3bb47b7..68552da 100644
--- a/src/kudu/master/master.cc
+++ b/src/kudu/master/master.cc
@@ -468,16 +468,25 @@ Status Master::ListMasters(vector<ServerEntryPB>* masters) const {
   return Status::OK();
 }
 
-Status Master::GetMasterHostPorts(vector<HostPort>* hostports) const {
+Status Master::GetMasterHostPorts(vector<HostPort>* hostports, MasterType type) const {
   auto consensus = catalog_manager_->master_consensus();
   if (!consensus) {
     return Status::IllegalState("consensus not running");
   }
 
+  auto get_raft_member_type = [] (MasterType type) constexpr {
+    switch (type) {
+      case MasterType::VOTER_ONLY:
+        return RaftPeerPB::VOTER;
+      default:
+        LOG(FATAL) << "No matching Raft member type for master type: " << type;
+    }
+  };
+
   hostports->clear();
   consensus::RaftConfigPB config = consensus->CommittedConfig();
   for (const auto& peer : *config.mutable_peers()) {
-    if (peer.member_type() == consensus::RaftPeerPB::VOTER) {
+    if (type == MasterType::ALL || get_raft_member_type(type) == peer.member_type()) {
       // In non-distributed master configurations, we may not store our own
       // last known address in the Raft config. So, we'll fill it in from
       // the server Registration instead.
@@ -497,8 +506,8 @@ Status Master::GetMasterHostPorts(vector<HostPort>* hostports) const {
 Status Master::AddMaster(const HostPort& hp, rpc::RpcContext* rpc) {
   // Ensure requested master to be added is not already part of list of masters.
   vector<HostPort> masters;
-  // Here the check is made against committed config with voters only.
-  RETURN_NOT_OK(GetMasterHostPorts(&masters));
+  // Here the check is made against committed config with all member types.
+  RETURN_NOT_OK(GetMasterHostPorts(&masters, MasterType::ALL));
   if (std::find(masters.begin(), masters.end(), hp) != masters.end()) {
     return Status::AlreadyPresent("Master already present");
   }
diff --git a/src/kudu/master/master.h b/src/kudu/master/master.h
index a9b919d..6ff30e8 100644
--- a/src/kudu/master/master.h
+++ b/src/kudu/master/master.h
@@ -113,10 +113,15 @@ class Master : public kserver::KuduServer {
   // request.
   Status ListMasters(std::vector<ServerEntryPB>* masters) const;
 
-  // Gets the HostPorts for all of the VOTER masters in the cluster.
+  enum MasterType {
+    ALL,
+    VOTER_ONLY
+  };
+
+  // Gets the HostPorts of masters in the cluster of the specified 'type'.
   // This is not as complete as ListMasters() above, but operates just
   // based on local state.
-  Status GetMasterHostPorts(std::vector<HostPort>* hostports) const;
+  Status GetMasterHostPorts(std::vector<HostPort>* hostports, MasterType type = VOTER_ONLY) const;
 
   bool IsShutdown() const {
     return state_ == kStopped;