This is an automated email from the ASF dual-hosted git repository. awong 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 fae223e [tool] KUDU-3226 Validate List of Masters kudu ksck fae223e is described below commit fae223ecb139e0f54161279ee8970098067629c9 Author: Abhishek Chennaka <r...@ccycloud.kudutest.root.hwx.site> AuthorDate: Sun Feb 28 19:40:45 2021 -0800 [tool] KUDU-3226 Validate List of Masters kudu ksck This patch checks for duplicate kudu masters provided in all of the kudu command line tools which call ParseMasterAddresses(). Currently ksck and rebalance (as rebalance calls ksck internally) are the only commands which throw the reported stack trace as ksck constructs a map internally for the kudu masters and bombs once it sees duplicate UUIDs. Other commands do not report any issue currently with duplicate masters. The patch only does a string comparison check to detect and report all the duplicate master(/s). If a user provides the IP address and hostname of the same master, it won't be able to catch the duplicates. That seems more of a deliberate attempt rather than a common mistake a user could make and hence not considering that case. Also wrote a simple test case to test this functionality. Decided not to spin up a test cluster for this as this is more of client side check and a master is not actually contacted. This change got conflicted with another test i.e. AdminCliTest.TestAuthzResetCacheIncorrectMasterAddressList. Updated this test as well to by creating a new external mini cluster with three masters and running the command in the test with just one master Change-Id: I2f3b2b7dcf2ac78cb95cf43242651e3ce8fddf6f Reviewed-on: http://gerrit.cloudera.org:8080/17141 Reviewed-by: Alexey Serbin <aser...@cloudera.com> Tested-by: Kudu Jenkins Reviewed-by: Andrew Wong <aw...@cloudera.com> --- src/kudu/tools/kudu-admin-test.cc | 20 +++++++++++--------- src/kudu/tools/kudu-tool-test.cc | 14 ++++++++++++++ src/kudu/tools/tool_action_common.cc | 19 ++++++++++++++++++- 3 files changed, 43 insertions(+), 10 deletions(-) diff --git a/src/kudu/tools/kudu-admin-test.cc b/src/kudu/tools/kudu-admin-test.cc index bf30864..208c64b 100644 --- a/src/kudu/tools/kudu-admin-test.cc +++ b/src/kudu/tools/kudu-admin-test.cc @@ -53,7 +53,6 @@ #include "kudu/gutil/basictypes.h" #include "kudu/gutil/map-util.h" #include "kudu/gutil/stl_util.h" -#include "kudu/gutil/strings/join.h" #include "kudu/gutil/strings/split.h" #include "kudu/gutil/strings/strip.h" #include "kudu/gutil/strings/substitute.h" @@ -97,6 +96,8 @@ using kudu::client::KuduTableCreator; using kudu::client::KuduValue; using kudu::client::sp::shared_ptr; using kudu::cluster::ExternalTabletServer; +using kudu::cluster::ExternalMiniCluster; +using kudu::cluster::ExternalMiniClusterOptions; using kudu::consensus::COMMITTED_OPID; using kudu::consensus::ConsensusStatePB; using kudu::consensus::EXCLUDE_HEALTH_REPORT; @@ -2236,11 +2237,12 @@ TEST_F(AdminCliTest, TestDumpMemTrackers) { } TEST_F(AdminCliTest, TestAuthzResetCacheIncorrectMasterAddressList) { - NO_FATALS(BuildAndStart()); - - const auto& master_addr = cluster_->master()->bound_rpc_hostport().ToString(); - const vector<string> dup_master_addresses = { master_addr, master_addr, }; - const auto& dup_master_addresses_str = JoinStrings(dup_master_addresses, ","); + ExternalMiniClusterOptions opts; + opts.num_masters = 3; + cluster_.reset(new ExternalMiniCluster(std::move(opts))); + ASSERT_OK(cluster_->Start()); + auto master_addrs_str = HostPort::ToCommaSeparatedString(cluster_->master_rpc_addrs()); + auto incorrect_master_addrs_str = cluster_->master(0)->bound_rpc_hostport().ToString(); string out; string err; Status s; @@ -2249,14 +2251,14 @@ TEST_F(AdminCliTest, TestAuthzResetCacheIncorrectMasterAddressList) { "master", "authz_cache", "refresh", - dup_master_addresses_str, + incorrect_master_addrs_str, }, &out, &err); ASSERT_TRUE(s.IsRuntimeError()) << ToolRunInfo(s, out, err); const auto ref_err_msg = Substitute( "Invalid argument: list of master addresses provided ($0) " "does not match the actual cluster configuration ($1)", - dup_master_addresses_str, master_addr); + incorrect_master_addrs_str, master_addrs_str); ASSERT_STR_CONTAINS(err, ref_err_msg); // However, the '--force' option makes it possible to run the tool even @@ -2269,7 +2271,7 @@ TEST_F(AdminCliTest, TestAuthzResetCacheIncorrectMasterAddressList) { "authz_cache", "refresh", "--force", - dup_master_addresses_str, + incorrect_master_addrs_str, }, &out, &err)); } diff --git a/src/kudu/tools/kudu-tool-test.cc b/src/kudu/tools/kudu-tool-test.cc index 7a2bcc0..095e432 100644 --- a/src/kudu/tools/kudu-tool-test.cc +++ b/src/kudu/tools/kudu-tool-test.cc @@ -6100,5 +6100,19 @@ TEST_F(ToolTest, ConnectionNegotiationTimeoutOption) { } } +// Execute a ksck giving the hostname of the local machine twice +// and it should report the duplicates found. +TEST_F(ToolTest, TestDuplicateMastersInKsck) { + string master_addr; + if (!GetHostname(&master_addr).ok()) { + master_addr = "<unknown_host>"; + } + string out; + string cmd = Substitute("cluster ksck $0,$0", master_addr); + Status s = RunActionStderrString(cmd, &out); + ASSERT_TRUE(s.IsRuntimeError()) << s.ToString(); + ASSERT_STR_CONTAINS(out, "Duplicate master addresses specified"); +} + } // namespace tools } // namespace kudu diff --git a/src/kudu/tools/tool_action_common.cc b/src/kudu/tools/tool_action_common.cc index 661ded3..ebbe05a 100644 --- a/src/kudu/tools/tool_action_common.cc +++ b/src/kudu/tools/tool_action_common.cc @@ -648,7 +648,24 @@ Status ParseMasterAddresses(const RunnerContext& context, CHECK(master_addresses); string master_addresses_str; RETURN_NOT_OK(ParseMasterAddressesStr(context, master_addresses_arg, &master_addresses_str)); - *master_addresses = strings::Split(master_addresses_str, ","); + vector<string> master_addresses_local = strings::Split(master_addresses_str, ","); + std::unordered_set<string> unique_masters; + std::unordered_set<string> duplicate_masters; + // Loop through the master addresses to find the duplicate. If there is no master specified + // do not report as a duplicate + for (const auto& master : master_addresses_local) { + if (master.empty()) continue; + if (ContainsKey(unique_masters, master)) { + duplicate_masters.insert(master); + } else { + unique_masters.insert(master); + } + } + if (!duplicate_masters.empty()) { + return Status::InvalidArgument( + "Duplicate master addresses specified: " + JoinStrings(duplicate_masters, ",")); + } + *master_addresses = std::move(master_addresses_local); return Status::OK(); }