[kudu] branch master updated: [tool] KUDU-3226 Validate List of Masters kudu ksck

[aw...@apache.org]

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 <ro...@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 <as...@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();
 }