[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 <[email protected]>
Reviewed-by: Alexey Serbin <[email protected]>

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

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");
   }

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;