This is an automated email from the ASF dual-hosted git repository. zhangyifan pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 5ef0168cf0ae4471632d63cad223d7301f415982 Author: zhangyifan27 <[email protected]> AuthorDate: Thu Dec 30 00:32:39 2021 +0800 KUDU-3346: fix rebalancer tool fails to run with '--ignored_tservers' Prior to this patch the validity of 'ignored_tservers' was checked when 'BuildClusterinfo', which leads to a failure when the 'raw_info' only contains contains information of tservers on a specific location. This patch fix it by moving the parameter validity check into 'KsckResultsToClusterRawInfo', because ksck results contain original cluster information. I noticed 'ClusterInfo::tservers_to_empty' is not necessary to be built when 'BuildClusterInfo', because we use this info only for printing cluster's stats and running IgnoredTserverRunner. This should be refactored in follow-up patch. This patch adds a regression test for the issue and I also verified this fix on a real cluster. Change-Id: I1361f562f3e886077a79c3de8ea5fb2ebb8df6e9 Reviewed-on: http://gerrit.cloudera.org:8080/18114 Reviewed-by: Andrew Wong <[email protected]> Tested-by: Andrew Wong <[email protected]> --- src/kudu/rebalance/rebalancer.cc | 15 ++--- src/kudu/tools/rebalancer_tool-test.cc | 108 ++++++++++++++++++++++++++++++++- src/kudu/tools/rebalancer_tool.cc | 56 +++++++++-------- src/kudu/tools/rebalancer_tool.h | 18 +++--- 4 files changed, 151 insertions(+), 46 deletions(-) diff --git a/src/kudu/rebalance/rebalancer.cc b/src/kudu/rebalance/rebalancer.cc index 9699f6b..7b74b30 100644 --- a/src/kudu/rebalance/rebalancer.cc +++ b/src/kudu/rebalance/rebalancer.cc @@ -302,6 +302,10 @@ Status Rebalancer::BuildClusterInfo(const ClusterRawInfo& raw_info, for (const auto& summary : raw_info.tserver_summaries) { const auto& ts_id = summary.uuid; const auto& ts_location = summary.ts_location; + if (ContainsKey(config_.ignored_tservers, ts_id)) { + VLOG(1) << Substitute("ignoring tserver $0", ts_id); + continue; + } VLOG(1) << Substitute("found tserver $0 at location '$1'", ts_id, ts_location); EmplaceOrDie(&location_by_ts_uuid, ts_id, ts_location); auto& ts_ids = LookupOrEmplace(&ts_uuids_by_location, @@ -448,19 +452,16 @@ Status Rebalancer::BuildClusterInfo(const ClusterRawInfo& raw_info, table_info_by_skew.emplace(max_count - min_count, std::move(tbi)); } - // Populate ClusterInfo::tservers_to_empty + // Populate ClusterInfo::tservers_to_empty. + // TODO(zhangyifan): extract this to a seperate method. if (config_.move_replicas_from_ignored_tservers) { auto& tservers_to_empty = result_info.tservers_to_empty; for (const auto& ignored_tserver : config_.ignored_tservers) { if (ContainsKey(unhealthy_tablet_servers, ignored_tserver)) { continue; } - const int* replica_count = FindOrNull(tserver_replicas_count, ignored_tserver); - if (!replica_count) { - return Status::InvalidArgument(Substitute( - "ignored tserver $0 is not reported among known tservers", ignored_tserver)); - } - tservers_to_empty.emplace(ignored_tserver, *replica_count); + int replica_count = FindWithDefault(tserver_replicas_count, ignored_tserver, 0); + tservers_to_empty.emplace(ignored_tserver, replica_count); } } diff --git a/src/kudu/tools/rebalancer_tool-test.cc b/src/kudu/tools/rebalancer_tool-test.cc index 208854c..d2aed3d 100644 --- a/src/kudu/tools/rebalancer_tool-test.cc +++ b/src/kudu/tools/rebalancer_tool-test.cc @@ -173,15 +173,15 @@ Per-table replica distribution summary: << ToolRunInfo(s, out, err); } -// Make sure the rebalancer doesn't start if a tablet server is down. -// The rebalancer starts only when the dead tablet server is in the -// list of ignored_tservers. class RebalanceStartCriteriaTest : public AdminCliTest, public ::testing::WithParamInterface<Kudu1097> { }; INSTANTIATE_TEST_SUITE_P(, RebalanceStartCriteriaTest, ::testing::Values(Kudu1097::Disable, Kudu1097::Enable)); +// Make sure the rebalancer doesn't start if a tablet server is down. +// The rebalancer starts only when the dead tablet server is in the +// list of ignored_tservers. TEST_P(RebalanceStartCriteriaTest, TabletServerIsDown) { const bool is_343_scheme = (GetParam() == Kudu1097::Enable); const vector<string> kMasterFlags = { @@ -236,6 +236,49 @@ TEST_P(RebalanceStartCriteriaTest, TabletServerIsDown) { } } +// Make sure the rebalancer doesn't start if specified an unknown tablet server. +TEST_P(RebalanceStartCriteriaTest, UnknownIgnoredTServer) { + const bool is_343_scheme = (GetParam() == Kudu1097::Enable); + const vector<string> kMasterFlags = { + Substitute("--raft_prepare_replacement_before_eviction=$0", is_343_scheme), + }; + const vector<string> kTserverFlags = { + Substitute("--raft_prepare_replacement_before_eviction=$0", is_343_scheme), + }; + + FLAGS_num_tablet_servers = 5; + NO_FATALS(BuildAndStart(kTserverFlags, kMasterFlags)); + + { + string out; + string err; + Status s = RunKuduTool({"cluster", + "rebalance", + cluster_->master()->bound_rpc_addr().ToString(), + "--ignored_tservers=unknown_tablet_server"}, + &out, + &err); + ASSERT_TRUE(s.IsRuntimeError()) << ToolRunInfo(s, out, err); + ASSERT_STR_CONTAINS( + err, "ignored tserver unknown_tablet_server is not reported among known tservers") + << "stderr: " << err; + } + + { + string out; + string err; + Status s = RunKuduTool({"cluster", + "rebalance", + cluster_->master()->bound_rpc_addr().ToString(), + "--ignored_tservers=" + cluster_->tablet_server(0)->uuid()}, + &out, + &err); + ASSERT_TRUE(s.ok()) << ToolRunInfo(s, out, err); + ASSERT_STR_CONTAINS(out, "rebalancing is complete: cluster is balanced (moved 0 replicas)") + << "stderr: " << err; + } +} + // Make sure the rebalancer doesn't start if specified too many ignored tservers. class RebalanceStartSafetyTest : public AdminCliTest, @@ -1967,6 +2010,65 @@ TEST_P(LocationAwareRebalancingParamTest, Rebalance) { } } +// Regression test for KUDU-3346. +TEST_P(LocationAwareRebalancingParamTest, RebalanceWithIgnoredTServer) { + SKIP_IF_SLOW_NOT_ALLOWED(); + const auto& param = GetParam(); + const auto& location_info = param.location_info; + const auto& excluded_locations = param.excluded_locations; + const vector<string>& extra_master_flags = { + "--master_client_location_assignment_enabled=false", + "--allow_unsafe_replication_factor", + }; + NO_FATALS(Prepare({}, extra_master_flags, location_info, excluded_locations)); + + // Pick a random tserver as ignored tserver. + int rand_ts_idx = rand() % cluster_->num_tablet_servers(); + string rand_ts_uuid = cluster_->tablet_server(rand_ts_idx)->uuid(); + + { + string out; + string err; + const Status s = RunKuduTool({"cluster", + "rebalance", + cluster_->master()->bound_rpc_addr().ToString(), + "--ignored_tservers=" + rand_ts_uuid}, + &out, + &err); + ASSERT_TRUE(s.ok()) << ToolRunInfo(s, out, err); + ASSERT_STR_NOT_CONTAINS(out, rand_ts_uuid); + ASSERT_STR_CONTAINS(out, "rebalancing is complete: cluster is balanced") << "stderr: " << err; + } + + { + // Run rebalancer tool with 'move_replicas_from_ignored_tservers'. + string out; + string err; + Status s = RunKuduTool({"tserver", + "state", + "enter_maintenance", + cluster_->master()->bound_rpc_addr().ToString(), + rand_ts_uuid}, + &out, + &err); + ASSERT_TRUE(s.ok()) << ToolRunInfo(s, out, err); + s = RunKuduTool({"cluster", + "rebalance", + cluster_->master()->bound_rpc_addr().ToString(), + "--ignored_tservers=" + rand_ts_uuid, + "--move_replicas_from_ignored_tservers"}, + &out, + &err); + if (s.ok()) { + ASSERT_STR_CONTAINS(out, "rebalancing is complete: cluster is balanced") << "stderr: " << err; + ASSERT_STR_CONTAINS(out, Substitute("$0 | 0", rand_ts_uuid)) << "stderr: " << err; + } else { + // In some cases it's impossible to move replicas from a ignored tserver. + ASSERT_STR_CONTAINS(err, "Too many ignored tservers") << "stderr: " << err; + } + } +} + // Basic intra location rebalance tests. class IntraLocationRebalancingBasicTest : public RebalancingTest { public: diff --git a/src/kudu/tools/rebalancer_tool.cc b/src/kudu/tools/rebalancer_tool.cc index 6e638c9..3008ac8 100644 --- a/src/kudu/tools/rebalancer_tool.cc +++ b/src/kudu/tools/rebalancer_tool.cc @@ -189,7 +189,6 @@ Status RebalancerTool::Run(RunStatus* result_status, size_t* moves_count) { size_t moves_count_total = 0; if (config_.move_replicas_from_ignored_tservers) { // Move replicas from healthy ignored tservers to other healthy tservers. - RETURN_NOT_OK(CheckIgnoredServers(raw_info, ci)); LOG(INFO) << "replacing replicas on healthy ignored tservers"; IgnoredTserversRunner runner( this, config_.ignored_tservers, config_.max_moves_per_server, deadline); @@ -269,10 +268,24 @@ Status RebalancerTool::KsckResultsToClusterRawInfo( const KsckResults& ksck_info, ClusterRawInfo* raw_info) { DCHECK(raw_info); + const auto& cluster_status = ksck_info.cluster_status; + + // Check whether all ignored tservers in the config are valid. + if (!config_.ignored_tservers.empty()) { + unordered_set<string> known_tservers; + for (const auto& ts_summary : ksck_info.cluster_status.tserver_summaries) { + known_tservers.emplace(ts_summary.uuid); + } + for (const auto& ts : config_.ignored_tservers) { + if (!ContainsKey(known_tservers, ts)) { + return Status::InvalidArgument( + Substitute("ignored tserver $0 is not reported among known tservers", ts)); + } + } + } // Filter out entities that are not relevant to the specified location. vector<ServerHealthSummary> tserver_summaries; - const auto& cluster_status = ksck_info.cluster_status; tserver_summaries.reserve(cluster_status.tserver_summaries.size()); vector<TabletSummary> tablet_summaries; @@ -644,22 +657,6 @@ Status RebalancerTool::RefreshKsckResults() { return Status::OK(); } -Status RebalancerTool::CheckIgnoredServers(const rebalance::ClusterRawInfo& raw_info, - const rebalance::ClusterInfo& cluster_info) { - int remaining_tservers_count = cluster_info.balance.servers_by_total_replica_count.size(); - int max_replication_factor = 0; - for (const auto& s : raw_info.table_summaries) { - max_replication_factor = std::max(max_replication_factor, s.replication_factor); - } - if (remaining_tservers_count < max_replication_factor) { - return Status::InvalidArgument( - Substitute("Too many ignored tservers; " - "$0 healthy non-ignored servers exist but $1 are required.", - remaining_tservers_count, max_replication_factor)); - } - return Status::OK(); -} - RebalancerTool::BaseRunner::BaseRunner(RebalancerTool* rebalancer, std::unordered_set<std::string> ignored_tservers, size_t max_moves_per_server, @@ -1381,11 +1378,7 @@ Status RebalancerTool::IgnoredTserversRunner::GetReplaceMoves( const rebalance::ClusterInfo& ci, const rebalance::ClusterRawInfo& raw_info, vector<Rebalancer::ReplicaMove>* replica_moves) { - - // In order not to place any replica on the ignored tservers, - // allow to run only when all healthy ignored tservers are in - // maintenance (or decommision) mode. - RETURN_NOT_OK(CheckIgnoredTserversState(ci)); + RETURN_NOT_OK(CheckIgnoredTServers(raw_info, ci)); // Build IgnoredTserversInfo. IgnoredTserversInfo ignored_tservers_info; @@ -1425,12 +1418,25 @@ Status RebalancerTool::IgnoredTserversRunner::GetReplaceMoves( return Status::OK(); } -Status RebalancerTool::IgnoredTserversRunner::CheckIgnoredTserversState( - const rebalance::ClusterInfo& ci) { +Status RebalancerTool::IgnoredTserversRunner::CheckIgnoredTServers(const ClusterRawInfo& raw_info, + const ClusterInfo& ci) { if (ci.tservers_to_empty.empty()) { return Status::OK(); } + int remaining_tservers_count = ci.balance.servers_by_total_replica_count.size(); + int max_replication_factor = 0; + for (const auto& s : raw_info.table_summaries) { + max_replication_factor = std::max(max_replication_factor, s.replication_factor); + } + if (remaining_tservers_count < max_replication_factor) { + return Status::InvalidArgument( + Substitute("Too many ignored tservers; " + "$0 healthy non-ignored servers exist but $1 are required.", + remaining_tservers_count, + max_replication_factor)); + } + LeaderMasterProxy proxy(client_); ListTabletServersRequestPB req; ListTabletServersResponsePB resp; diff --git a/src/kudu/tools/rebalancer_tool.h b/src/kudu/tools/rebalancer_tool.h index 94e6aed..c44172f 100644 --- a/src/kudu/tools/rebalancer_tool.h +++ b/src/kudu/tools/rebalancer_tool.h @@ -354,9 +354,10 @@ class RebalancerTool : public rebalance::Rebalancer { const rebalance::ClusterRawInfo& raw_info, std::vector<Rebalancer::ReplicaMove>* replica_moves) override; - // Check the state of ignored tservers. - // Return Status::OK() only when all the ignored tservers are in maintenance mode. - Status CheckIgnoredTserversState(const rebalance::ClusterInfo& ci); + // Return Status::OK() if it's safe to move all replicas from the ignored to other servers + // and all tservers that need to empty are in maintenance mode. + Status CheckIgnoredTServers(const rebalance::ClusterRawInfo& raw_info, + const rebalance::ClusterInfo& ci); void GetMovesFromIgnoredTservers(const IgnoredTserversInfo& ignored_tservers_info, std::vector<Rebalancer::ReplicaMove>* replica_moves); @@ -371,10 +372,9 @@ class RebalancerTool : public rebalance::Rebalancer { // 'location' means that's about cross-location rebalancing). Basically, // 'raw' information is just a sub-set of relevant fields of the KsckResults // structure filtered to contain information only for the specified location. - static Status KsckResultsToClusterRawInfo( - const boost::optional<std::string>& location, - const KsckResults& ksck_info, - rebalance::ClusterRawInfo* raw_info); + Status KsckResultsToClusterRawInfo(const boost::optional<std::string>& location, + const KsckResults& ksck_info, + rebalance::ClusterRawInfo* raw_info); // Print replica count infomation on ClusterInfo::tservers_to_empty. Status PrintIgnoredTserversStats(const rebalance::ClusterInfo& ci, @@ -395,10 +395,6 @@ class RebalancerTool : public rebalance::Rebalancer { Status PrintPolicyViolationInfo(const rebalance::ClusterRawInfo& raw_info, std::ostream& out) const; - // Check whether it is safe to move all replicas from the ignored to other servers. - Status CheckIgnoredServers(const rebalance::ClusterRawInfo& raw_info, - const rebalance::ClusterInfo& cluster_info); - // Run rebalancing using the specified runner. Status RunWith(Runner* runner, RunStatus* result_status);
