Skip to content

Commit

Permalink
[master] KUDU-3182 Allow single master to specify --master_addresses
Browse files Browse the repository at this point in the history
'last_known_addr' field is not persisted for a single master Raft
configuration. This is okay as long as we have a single master
configuration. On dynamically transitioning from single master
to two master configuration, ChangeConfig() request to ADD_PEER
fails the validation in VerifyRaftConfig().

In order to transition to multi-master configuration, 'last_known_addr'
must be specified.

Some options considered were using the --rpc_bind_addresses flag, if
specified. If not, use GetHostName(). None of these are reliable.
So it's best to let user to specify the address of the single master
as is the case for specifying master address in tablet servers.

Currently --master_addresses flag must be empty for single master
configuration and --master_addresses with single address is not
permitted.

So the idea is to use existing --master_addresses flag for single
master configuration too and not just distributed configuration.

This change allows specifying --master_addresses flag for
single master configuration. To be backward compatible with existing
deployments, specifying this flag for single master configurations
is still optional and not required. If specified, it's used to set the
'last_known_addr' field in the master Raft configuration.

This change also provides mechanism for existing single master
Kudu deployments to specify the --master_addresses flag and populate
the 'last_known_addr' field in the master Raft configuration.

This change is a pre-requisite for upcoming dynamic multi-master
changes.

Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Reviewed-on: http://gerrit.cloudera.org:8080/16340
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <[email protected]>
  • Loading branch information
bbhavsar committed Sep 3, 2020
1 parent dcb9b71 commit a59d382
Show file tree
Hide file tree
Showing 13 changed files with 305 additions and 18 deletions.
1 change: 1 addition & 0 deletions src/kudu/master/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ ADD_KUDU_TEST(auto_rebalancer-test)
ADD_KUDU_TEST(catalog_manager-test)
ADD_KUDU_TEST(hms_notification_log_listener-test)
ADD_KUDU_TEST(location_cache-test DATA_FILES ../scripts/first_argument.sh)
ADD_KUDU_TEST(master_options-test)
ADD_KUDU_TEST(master-test RESOURCE_LOCK "master-web-port"
DATA_FILES ../scripts/first_argument.sh)
ADD_KUDU_TEST(mini_master-test RESOURCE_LOCK "master-web-port")
Expand Down
2 changes: 1 addition & 1 deletion src/kudu/master/master.cc
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ Status Master::GetMasterHostPorts(vector<HostPort>* hostports) const {
consensus::RaftConfigPB config = consensus->CommittedConfig();
for (const auto& peer : *config.mutable_peers()) {
if (peer.member_type() == consensus::RaftPeerPB::VOTER) {
// In non-distributed master configurations, we don't store our own
// 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.
if (!peer.has_last_known_addr()) {
Expand Down
205 changes: 205 additions & 0 deletions src/kudu/master/master_options-test.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,205 @@
// Licensed to the Apache Software Foundation (ASF) under one
// or more contributor license agreements. See the NOTICE file
// distributed with this work for additional information
// regarding copyright ownership. The ASF licenses this file
// to you under the Apache License, Version 2.0 (the
// "License"); you may not use this file except in compliance
// with the License. You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing,
// software distributed under the License is distributed on an
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.

#include <memory>
#include <string>
#include <vector>

#include <gtest/gtest.h>

#include "kudu/client/client.h"
#include "kudu/common/wire_protocol.h"
#include "kudu/common/wire_protocol.pb.h"
#include "kudu/consensus/metadata.pb.h"
#include "kudu/consensus/raft_consensus.h"
#include "kudu/gutil/strings/substitute.h"
#include "kudu/master/catalog_manager.h"
#include "kudu/master/master.h"
#include "kudu/master/master.pb.h"
#include "kudu/master/master.proxy.h"
#include "kudu/master/mini_master.h"
#include "kudu/mini-cluster/external_mini_cluster.h"
#include "kudu/mini-cluster/internal_mini_cluster.h"
#include "kudu/mini-cluster/mini_cluster.h"
#include "kudu/rpc/rpc_controller.h"
#include "kudu/util/monotime.h"
#include "kudu/util/net/net_util.h"
#include "kudu/util/net/sockaddr.h"
#include "kudu/util/net/socket.h"
#include "kudu/util/status.h"
#include "kudu/util/test_macros.h"
#include "kudu/util/test_util.h"

using kudu::client::KuduClient;
using kudu::client::sp::shared_ptr;
using kudu::cluster::ExternalMiniCluster;
using kudu::cluster::ExternalMiniClusterOptions;
using kudu::cluster::InternalMiniCluster;
using kudu::cluster::InternalMiniClusterOptions;
using kudu::cluster::MiniCluster;
using std::string;
using std::unique_ptr;
using std::vector;
using strings::Substitute;

namespace kudu {
namespace master {
class MasterOptionsTest : public KuduTest {
};

// Verifies the InternalMiniCluster has a single master with 'last_known_addr' in the
// Raft config set to the 'expected_addr'. If 'expected_addr' is empty then verifies that
// the 'last_known_addr' field is not set.
void VerifySingleMasterRaftConfig(const InternalMiniCluster& cluster,
const string& expected_addr = "") {
auto consensus = cluster.mini_master()->master()->catalog_manager()->master_consensus();
ASSERT_NE(nullptr, consensus.get());
auto config = consensus->CommittedConfig();
ASSERT_EQ(1, config.peers_size());
const auto& peer = config.peers(0);
if (!expected_addr.empty()) {
ASSERT_TRUE(peer.has_last_known_addr());
ASSERT_EQ(expected_addr, HostPortFromPB(peer.last_known_addr()).ToString());
} else {
ASSERT_FALSE(peer.has_last_known_addr());
}
}

// Test bringing up a cluster with a single master config by populating --master_addresses flag
// with a single address using ExternalMiniCluster that closely simulates a real Kudu cluster.
TEST_F(MasterOptionsTest, TestSingleMasterWithMasterAddresses) {
// Reserving a port upfront for the master so that its address can be specified in
// --master_addresses.
unique_ptr<Socket> reserved_socket;
ASSERT_OK(MiniCluster::ReserveDaemonSocket(MiniCluster::MASTER, 1 /* index */,
kDefaultBindMode, &reserved_socket));
Sockaddr reserved_addr;
ASSERT_OK(reserved_socket->GetSocketAddress(&reserved_addr));
const string reserved_hp_str = HostPort(reserved_addr).ToString();

// ExternalMiniCluster closely simulates a real cluster where MasterOptions
// is constructed from the supplied flags.
ExternalMiniClusterOptions opts;
opts.num_masters = 1;
opts.extra_master_flags = { "--master_addresses=" + reserved_hp_str };

ExternalMiniCluster cluster(opts);
ASSERT_OK(cluster.Start());
ASSERT_OK(cluster.WaitForTabletServerCount(cluster.num_tablet_servers(),
MonoDelta::FromSeconds(5)));
shared_ptr<KuduClient> client;
ASSERT_OK(cluster.CreateClient(nullptr, &client));

auto verification_steps = [&] {
ASSERT_EQ(reserved_hp_str, client->GetMasterAddresses());
ASSERT_FALSE(client->IsMultiMaster());

ListMastersRequestPB req;
ListMastersResponsePB resp;
rpc::RpcController rpc;
ASSERT_OK(cluster.master_proxy()->ListMasters(req, &resp, &rpc));
ASSERT_EQ(1, resp.masters_size());
ASSERT_EQ(consensus::RaftPeerPB::LEADER, resp.masters(0).role());
};
verification_steps();

// Restarting the cluster exercises loading the existing system catalog code-path.
ASSERT_OK(cluster.Restart());
ASSERT_OK(cluster.WaitForTabletServerCount(cluster.num_tablet_servers(),
MonoDelta::FromSeconds(5)));
verification_steps();
}

// Test that verifies the 'last_known_addr' field in Raft config is set with a single master
// configuration when '--master_addresses' field is supplied on a fresh kudu deployment.
TEST_F(MasterOptionsTest, TestSingleMasterForRaftConfigFresh) {
InternalMiniClusterOptions opts;
opts.num_masters = 1;
opts.supply_single_master_addr = true;
InternalMiniCluster cluster(env_, opts);
ASSERT_OK(cluster.Start());
const string master_addr = HostPort(cluster.mini_master()->bound_rpc_addr()).ToString();
VerifySingleMasterRaftConfig(cluster, master_addr);
}

// Test that verifies existing single master kudu deployments where '--master_addresses' will not
// be set after upgrade. On specifying '--master_addresses' followed by a master restart the test
// verifies that 'last_known_addr' field is set in the master Raft config.
// Also verifies other cases like mismatch in '--master_addresses' field and persisted master
// Raft config and missing '--master_addresses' field but presence of 'last_known_addr' field
// in master Raft config. See the corresponding implementation in SysCatalogTable::Load().
TEST_F(MasterOptionsTest, TestSingleMasterForRaftConfigUpgrade) {
// Bring up a single master new cluster without supplying '--master_addresses' field.
InternalMiniClusterOptions opts;
opts.num_masters = 1;
opts.supply_single_master_addr = false;
InternalMiniCluster cluster(env_, opts);
ASSERT_OK(cluster.Start());

// Verify 'last_known_addr' field is not set in Raft config.
VerifySingleMasterRaftConfig(cluster);

// Verify 'last_known_addr' field is set in Raft config on supplying '--master_addresses'
// field after restart.
const vector<HostPort> master_addresses = { HostPort(cluster.mini_master()->bound_rpc_addr()) };
const string& master_addr = master_addresses[0].ToString();
cluster.mini_master()->Shutdown();
cluster.mini_master()->SetMasterAddresses(master_addresses);
ASSERT_OK(cluster.mini_master()->Restart());
VerifySingleMasterRaftConfig(cluster, master_addr);

// Verify 'last_known_addr' field is set in Raft even after restart.
cluster.mini_master()->Shutdown();
ASSERT_OK(cluster.mini_master()->Restart());
VerifySingleMasterRaftConfig(cluster, master_addr);

// If --master_addresses flag is omitted when it was previously supplied then it'll
// result in a warning and still bring up master.
cluster.mini_master()->Shutdown();
cluster.mini_master()->SetMasterAddresses({});
ASSERT_OK(cluster.mini_master()->Restart());
VerifySingleMasterRaftConfig(cluster, master_addr);

// Supplying --master_addresses flag that's different from what's persisted in Raft config
// will result in master bring up error.
cluster.mini_master()->Shutdown();
HostPort incorrect_hp("foorbarbaz", Master::kDefaultPort);
cluster.mini_master()->SetMasterAddresses({incorrect_hp});
Status s = cluster.mini_master()->Restart();
ASSERT_TRUE(s.IsInvalidArgument());
ASSERT_STR_CONTAINS(s.ToString(),
Substitute("Single master Raft config error. On-disk master: $0 and supplied "
"master: $1 are different", master_addr, incorrect_hp.ToString()));

// Supplying multiple masters in --master_addresses flag to a single master configuration will
// result in master bring up error.
cluster.mini_master()->Shutdown();
cluster.mini_master()->SetMasterAddresses(
{master_addresses[0], HostPort("master-2", Master::kDefaultPort)});
// For multi-master configuration, as derived from --master_addresses flag, Restart()
// doesn't wait for catalog manager init.
s = cluster.mini_master(0)->Restart();
ASSERT_TRUE(s.ok() || s.IsInvalidArgument());
s = cluster.mini_master(0)->master()->WaitForCatalogManagerInit();
ASSERT_TRUE(s.IsInvalidArgument());
ASSERT_STR_MATCHES(s.ToString(),
"Unable to initialize catalog manager: Failed to initialize sys tables async.*"
"Their symmetric difference is: master-2.*");
}

} // namespace master
} // namespace kudu
21 changes: 14 additions & 7 deletions src/kudu/master/master_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@
DEFINE_string(master_addresses, "",
"Comma-separated list of the RPC addresses belonging to all "
"Masters in this cluster. "
"NOTE: if not specified, configures a non-replicated Master.");
"NOTE: if not specified or a single address is specified, "
"configures a non-replicated Master.");
TAG_FLAG(master_addresses, stable);

namespace kudu {
Expand All @@ -48,11 +49,6 @@ MasterOptions::MasterOptions() {
LOG(FATAL) << "Couldn't parse the master_addresses flag('" << FLAGS_master_addresses << "'): "
<< s.ToString();
}
if (master_addresses.size() < 2) {
LOG(FATAL) << "At least 2 masters are required for a distributed config, but "
"master_addresses flag ('" << FLAGS_master_addresses << "') only specifies "
<< master_addresses.size() << " masters.";
}
// TODO(wdberkeley): Un-actionable warning. Link to docs, once they exist.
if (master_addresses.size() == 2) {
LOG(WARNING) << "Only 2 masters are specified by master_addresses_flag ('" <<
Expand All @@ -63,7 +59,18 @@ MasterOptions::MasterOptions() {
}

bool MasterOptions::IsDistributed() const {
return !master_addresses.empty();
return master_addresses.size() > 1;
}

Status MasterOptions::GetTheOnlyMasterAddress(HostPort* hp) const {
if (IsDistributed()) {
return Status::IllegalState("Not a single master configuration");
}
if (master_addresses.empty()) {
return Status::NotFound("Master address not specified");
}
*hp = master_addresses.front();
return Status::OK();
}

} // namespace master
Expand Down
5 changes: 5 additions & 0 deletions src/kudu/master/master_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

#include "kudu/kserver/kserver_options.h"
#include "kudu/util/net/net_util.h"
#include "kudu/util/status.h"

namespace kudu {
namespace master {
Expand All @@ -33,6 +34,10 @@ struct MasterOptions : public kserver::KuduServerOptions {
std::vector<HostPort> master_addresses;

bool IsDistributed() const;

// For a single master configuration output the only master address in 'hp', if available.
// Otherwise NotFound error or IllegalState for distributed master config.
Status GetTheOnlyMasterAddress(HostPort* hp) const;
};

} // namespace master
Expand Down
2 changes: 1 addition & 1 deletion src/kudu/master/master_path_handlers.cc
Original file line number Diff line number Diff line change
Expand Up @@ -830,7 +830,7 @@ pair<string, string> MasterPathHandlers::TSDescToLinkPair(const TSDescriptor& de
}

string MasterPathHandlers::MasterAddrsToCsv() const {
if (master_->opts().IsDistributed()) {
if (!master_->opts().master_addresses.empty()) {
vector<string> all_addresses;
all_addresses.reserve(master_->opts().master_addresses.size());
for (const HostPort& hp : master_->opts().master_addresses) {
Expand Down
4 changes: 2 additions & 2 deletions src/kudu/master/mini_master.cc
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ void MiniMaster::SetMasterAddresses(vector<HostPort> master_addrs) {

Status MiniMaster::Start() {
CHECK(!master_);
if (opts_.master_addresses.empty()) {
if (!opts_.IsDistributed()) {
FLAGS_rpc_server_allow_ephemeral_ports = true;
}
// In case the wal dir and data dirs are subdirectories of the root directory,
Expand All @@ -95,7 +95,7 @@ Status MiniMaster::Start() {
master_.swap(master);

// Wait for the catalog manager to be ready if we only have a single master.
if (opts_.master_addresses.empty()) {
if (!opts_.IsDistributed()) {
return master_->WaitForCatalogManagerInit();
}
return Status::OK();
Expand Down
64 changes: 58 additions & 6 deletions src/kudu/master/sys_catalog.cc
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,50 @@ void SysCatalogTable::Shutdown() {
}
}

Status SysCatalogTable::VerifyAndPopulateSingleMasterConfig(ConsensusMetadata* cmeta) {
RaftConfigPB config = cmeta->CommittedConfig();
// Manual single to multi-master migration relies on starting up a single master with multiple
// masters specified in the on-disk Raft config, so log a warning instead of a strict check.
// No further validations or populating 'last_known_addr' in such a case.
if (config.peers_size() > 1) {
LOG(WARNING) << "For a single master config, multiple peers found in on-disk Raft config! "
"Supply correct list of masters in --master_addresses flag.";
return Status::OK();
}

CHECK_EQ(1, config.peers_size()) << "No Raft peer found for single master configuration!";
const auto& peer = config.peers(0);
HostPort master_addr;
bool master_addr_specified = master_->opts().GetTheOnlyMasterAddress(&master_addr).ok();
if (master_addr_specified) {
if (peer.has_last_known_addr()) {
// Verify the supplied master address matches with the on-disk Raft config.
auto raft_master_addr = HostPortFromPB(peer.last_known_addr());
if (raft_master_addr != master_addr) {
return Status::InvalidArgument(
Substitute("Single master Raft config error. On-disk master: $0 and "
"supplied master: $1 are different", raft_master_addr.ToString(),
master_addr.ToString()));
}
} else {
// Set the 'last_known_addr' in Raft config for single master configuration.
*config.mutable_peers(0)->mutable_last_known_addr() = HostPortToPB(master_addr);
cmeta->set_committed_config(config);
ConsensusStatePB cstate = cmeta->ToConsensusStatePB();
RETURN_NOT_OK(consensus::VerifyRaftConfig(cstate.committed_config()));
RETURN_NOT_OK_PREPEND(cmeta->Flush(),
"Failed to flush consensus metadata on populating "
"'last_known_addr' field in consensus metadata");
}
} else if (peer.has_last_known_addr()) {
LOG(WARNING) << Substitute("For a single master config, on-disk Raft master: $0 exists but "
"no master address supplied!",
HostPortFromPB(peer.last_known_addr()).ToString());
}

return Status::OK();
}

Status SysCatalogTable::Load(FsManager *fs_manager) {
// Load Metadata Information from disk
scoped_refptr<tablet::TabletMetadata> metadata;
Expand All @@ -205,12 +249,14 @@ Status SysCatalogTable::Load(FsManager *fs_manager) {
return(Status::Corruption("Unexpected schema", metadata->schema().ToString()));
}

if (master_->opts().IsDistributed()) {
LOG(INFO) << "Verifying existing consensus state";
string tablet_id = metadata->tablet_id();
scoped_refptr<ConsensusMetadata> cmeta;
RETURN_NOT_OK_PREPEND(cmeta_manager_->Load(tablet_id, &cmeta),
"Unable to load consensus metadata for tablet " + tablet_id);
LOG(INFO) << "Verifying existing consensus state";
const string tablet_id = metadata->tablet_id();
scoped_refptr<ConsensusMetadata> cmeta;
RETURN_NOT_OK_PREPEND(cmeta_manager_->Load(tablet_id, &cmeta),
"Unable to load consensus metadata for tablet " + tablet_id);
if (!master_->opts().IsDistributed()) {
RETURN_NOT_OK(VerifyAndPopulateSingleMasterConfig(cmeta.get()));
} else {
ConsensusStatePB cstate = cmeta->ToConsensusStatePB();
RETURN_NOT_OK(consensus::VerifyRaftConfig(cstate.committed_config()));
CHECK(!cstate.has_pending_config());
Expand Down Expand Up @@ -286,6 +332,12 @@ Status SysCatalogTable::CreateNew(FsManager *fs_manager) {
RaftPeerPB* peer = config.add_peers();
peer->set_permanent_uuid(fs_manager->uuid());
peer->set_member_type(RaftPeerPB::VOTER);
// Set 'last_known_addr' if master address is specified for a single master configuration.
// This helps in migrating to multiple masters.
HostPort hp;
if (master_->opts().GetTheOnlyMasterAddress(&hp).ok()) {
*peer->mutable_last_known_addr() = HostPortToPB(hp);
}
}

string tablet_id = metadata->tablet_id();
Expand Down
Loading

0 comments on commit a59d382

Please sign in to comment.