[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 <bankim@cloudera.com>
Reviewed-by: Alexey Serbin <aserbin@cloudera.com>
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 @@
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 @@
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::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 @@
// 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;
