kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ale...@apache.org
Subject [kudu] 02/02: [tests] use system_unsync time source for tests by default
Date Wed, 04 Dec 2019 05:58:37 GMT
This is an automated email from the ASF dual-hosted git repository.

alexey pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git

commit eb2b70d4b96be2fc2fdd6b3625acc284ac5774be
Author: Alexey Serbin <alexey@apache.org>
AuthorDate: Sat Oct 5 12:17:46 2019 -0700

    [tests] use system_unsync time source for tests by default
    
    This patch switches all KuduTest-derived tests to use 'system_unsync'
    time source by default (note that becomes so for test based on both
    internal and external mini-clusters).  The rationale is explained below.
    
    Essentially, there is no need to require NTP-synchronized clock for
    a generic test derived from KuduTest.  All the participating Kudu
    masters and tablet servers (and their sub-components in case of unit
    tests) are run at the same node using the same local wallclock.  Using
    the same wallclock makes the synchronisation of clocks unnecessary.
    
    The scenarios that verify the functioning of particular Kudu subsystems
    when using the 'system' and the 'builtin' clock sources should be put
    into a set of dedicated scenarios.  That's partially so even now: the
    scenarios which require verification of the built-in NTP client
    functionality are separated into a dedicated test set.  However, it's
    necessary to add dedicated test scenarios for the 'system' clock source
    and add more coverage for the 'built-in' clock source.
    
    That's exactly what follow-up changelists will do, i.e. add dedicated
    test scenarios to cover the specific functionality attributed to the use
    of the following clock sources:
      * system  : local machine's wallclock clock synchronized with NTP
      * builtin : independently tracked true time by the built-in NTP client
    
    Change-Id: If7edbf884afaa19121aa92a4ce93c8a7eeb2d937
    Reviewed-on: http://gerrit.cloudera.org:8080/14379
    Reviewed-by: Adar Dembo <adar@cloudera.com>
    Tested-by: Alexey Serbin <aserbin@cloudera.com>
---
 src/kudu/client/client_examples-test.sh        |  2 ++
 src/kudu/clock/hybrid_clock-test.cc            |  4 +++
 src/kudu/clock/hybrid_clock.cc                 | 17 +++++++------
 src/kudu/mini-cluster/external_mini_cluster.cc | 34 ++++++++++++++------------
 src/kudu/mini-cluster/external_mini_cluster.h  |  4 +--
 src/kudu/scripts/start_kudu.sh                 | 11 ++++++++-
 src/kudu/util/test_util.cc                     | 22 ++++++++++-------
 src/kudu/util/test_util.h                      |  7 ------
 8 files changed, 59 insertions(+), 42 deletions(-)

diff --git a/src/kudu/client/client_examples-test.sh b/src/kudu/client/client_examples-test.sh
index 75d5ac7..a495e08 100755
--- a/src/kudu/client/client_examples-test.sh
+++ b/src/kudu/client/client_examples-test.sh
@@ -162,6 +162,7 @@ mkdir -p "$BASE_DIR/master/logs"
   --log_dir="$BASE_DIR/master/logs" \
   --fs_wal_dir="$BASE_DIR/master/wals" \
   --fs_data_dirs="$BASE_DIR/master/data" \
+  --time_source=system_unsync \
   --webserver_interface=$LOCALHOST_IP \
   --webserver_port=0 \
   --rpc_bind_addresses=$LOCALHOST_IP:$MASTER_RPC_PORT &
@@ -178,6 +179,7 @@ mkdir -p "$BASE_DIR/ts/logs"
   --fs_data_dirs="$BASE_DIR/ts/data" \
   --rpc_bind_addresses=$LOCALHOST_IP:$TSERVER_RPC_PORT \
   --local_ip_for_outbound_sockets=$LOCALHOST_IP \
+  --time_source=system_unsync \
   --webserver_interface=$LOCALHOST_IP \
   --webserver_port=0 \
   --tserver_master_addrs=$LOCALHOST_IP:$MASTER_RPC_PORT &
diff --git a/src/kudu/clock/hybrid_clock-test.cc b/src/kudu/clock/hybrid_clock-test.cc
index b4aed17..d5e5641 100644
--- a/src/kudu/clock/hybrid_clock-test.cc
+++ b/src/kudu/clock/hybrid_clock-test.cc
@@ -353,6 +353,10 @@ TEST_F(HybridClockTest, TestRideOverNtpInterruption) {
 
 #ifndef __APPLE__
 TEST_F(HybridClockTest, TestNtpDiagnostics) {
+  FLAGS_time_source = "system";
+  clock_.reset(new HybridClock);
+  ASSERT_OK(clock_->Init());
+
   vector<string> log;
   clock_->time_service()->DumpDiagnostics(&log);
   string s = JoinStrings(log, "\n");
diff --git a/src/kudu/clock/hybrid_clock.cc b/src/kudu/clock/hybrid_clock.cc
index b15bc2a..0ea1408 100644
--- a/src/kudu/clock/hybrid_clock.cc
+++ b/src/kudu/clock/hybrid_clock.cc
@@ -29,6 +29,7 @@
 #include "kudu/clock/builtin_ntp.h"
 #include "kudu/clock/mock_ntp.h"
 #include "kudu/clock/system_ntp.h"
+#include "kudu/clock/system_unsync_time.h"
 #include "kudu/gutil/bind.h"
 #include "kudu/gutil/bind_helpers.h"
 #include "kudu/gutil/macros.h"
@@ -41,10 +42,6 @@
 #include "kudu/util/monotime.h"
 #include "kudu/util/status.h"
 
-#ifdef __APPLE__
-#include "kudu/clock/system_unsync_time.h"
-#endif
-
 using kudu::Status;
 using std::string;
 using strings::Substitute;
@@ -60,13 +57,17 @@ DEFINE_bool(use_hybrid_clock, true,
             "implementation. This should be disabled for testing purposes only.");
 TAG_FLAG(use_hybrid_clock, hidden);
 
+// Use the 'system' time source by default in standard (non-test) environment.
+// This requires local machine clock to be NTP-synchronized.
 DEFINE_string(time_source, "system",
               "The time source that HybridClock should use. Must be one of "
-              "'system', 'builtin', or 'mock' ('mock' is for tests only)");
+              "'builtin', 'system', 'system_unsync', or 'mock' "
+              "('system_unsync' and 'mock' are for tests only)");
 TAG_FLAG(time_source, evolving);
 DEFINE_validator(time_source, [](const char* flag_name, const string& value) {
-  if (boost::iequals(value, "system") ||
-      boost::iequals(value, "builtin") ||
+  if (boost::iequals(value, "builtin") ||
+      boost::iequals(value, "system") ||
+      boost::iequals(value, "system_unsync") ||
       boost::iequals(value, "mock")) {
     return true;
   }
@@ -137,6 +138,8 @@ Status HybridClock::Init() {
 #else
     time_service_.reset(new clock::SystemUnsyncTime);
 #endif
+  } else if (boost::iequals(FLAGS_time_source, "system_unsync")) {
+    time_service_.reset(new clock::SystemUnsyncTime);
   } else if (boost::iequals(FLAGS_time_source, "builtin")) {
     time_service_.reset(new clock::BuiltInNtp);
   } else {
diff --git a/src/kudu/mini-cluster/external_mini_cluster.cc b/src/kudu/mini-cluster/external_mini_cluster.cc
index 4cd450f..0120dc9 100644
--- a/src/kudu/mini-cluster/external_mini_cluster.cc
+++ b/src/kudu/mini-cluster/external_mini_cluster.cc
@@ -87,6 +87,7 @@ using kudu::server::ServerStatusPB;
 using kudu::tserver::ListTabletsRequestPB;
 using kudu::tserver::ListTabletsResponsePB;
 using kudu::tserver::TabletServerServiceProxy;
+using std::copy;
 using std::pair;
 using std::string;
 using std::unique_ptr;
@@ -121,10 +122,9 @@ ExternalMiniClusterOptions::ExternalMiniClusterOptions()
       start_process_timeout(MonoDelta::FromSeconds(70)),
       rpc_negotiation_timeout(MonoDelta::FromSeconds(3))
 #if !defined(NO_CHRONY)
-      ,
-      num_ntp_servers(UseSystemNtp() ? 0 : 1)
-#endif
-    {
+      , num_ntp_servers(1)
+#endif // #if !defined(NO_CHRONY) ...
+{
 }
 
 ExternalMiniCluster::ExternalMiniCluster()
@@ -167,9 +167,11 @@ Status ExternalMiniCluster::HandleOptions() {
   return Status::OK();
 }
 
-#if !defined(NO_CHRONY)
-Status ExternalMiniCluster::AddNtpFlags(std::vector<std::string>* flags) {
+Status ExternalMiniCluster::AddTimeSourceFlags(std::vector<std::string>* flags) {
   DCHECK(flags);
+#if defined(NO_CHRONY)
+  flags->emplace_back("--time_source=system_unsync");
+#else
   if (opts_.num_ntp_servers > 0) {
     vector<string> ntp_endpoints;
     CHECK_EQ(opts_.num_ntp_servers, ntp_servers_.size());
@@ -190,10 +192,12 @@ Status ExternalMiniCluster::AddNtpFlags(std::vector<std::string>*
flags) {
     // Switch the clock to use the built-in NTP client which clock is
     // synchronized with the test NTP server.
     flags->emplace_back("--time_source=builtin");
+  } else {
+    flags->emplace_back("--time_source=system_unsync");
   }
+#endif // #if defined(NO_CHRONY) ... else ...
   return Status::OK();
 }
-#endif // #if !defined(NO_CHRONY) ...
 
 Status ExternalMiniCluster::StartSentry() {
   sentry_->SetDataRoot(opts_.cluster_root);
@@ -515,7 +519,7 @@ Status ExternalMiniCluster::StartMasters() {
     }
   }
 
-  vector<string> flags = opts_.extra_master_flags;
+  vector<string> flags;
   flags.emplace_back("--rpc_reuseport=true");
   if (num_masters > 1) {
     flags.emplace_back(Substitute("--master_addresses=$0",
@@ -544,9 +548,11 @@ Status ExternalMiniCluster::StartMasters() {
     flags.emplace_back("--location_mapping_by_uuid");
 #   endif
   }
-#if !defined(NO_CHRONY)
-  RETURN_NOT_OK(AddNtpFlags(&flags));
-#endif
+  RETURN_NOT_OK(AddTimeSourceFlags(&flags));
+
+  // Add custom master flags.
+  copy(opts_.extra_master_flags.begin(), opts_.extra_master_flags.end(),
+       std::back_inserter(flags));
 
   // Start the masters.
   const string& exe = GetBinaryPath(kMasterBinaryName);
@@ -630,11 +636,9 @@ Status ExternalMiniCluster::AddTabletServer() {
         Substitute("$0/perf-$1.data", opts.log_dir, daemon_id);
   }
   vector<string> extra_flags;
-#if !defined(NO_CHRONY)
-  RETURN_NOT_OK(AddNtpFlags(&extra_flags));
-#endif
+  RETURN_NOT_OK(AddTimeSourceFlags(&extra_flags));
   auto flags = SubstituteInFlags(opts_.extra_tserver_flags, idx);
-  std::copy(flags.begin(), flags.end(), std::back_inserter(extra_flags));
+  copy(flags.begin(), flags.end(), std::back_inserter(extra_flags));
   opts.extra_flags = extra_flags;
   opts.start_process_timeout = opts_.start_process_timeout;
   opts.rpc_bind_address = HostPort(bind_host, 0);
diff --git a/src/kudu/mini-cluster/external_mini_cluster.h b/src/kudu/mini-cluster/external_mini_cluster.h
index f02c7ac..428ec3a 100644
--- a/src/kudu/mini-cluster/external_mini_cluster.h
+++ b/src/kudu/mini-cluster/external_mini_cluster.h
@@ -432,9 +432,7 @@ class ExternalMiniCluster : public MiniCluster {
 
   Status DeduceBinRoot(std::string* ret);
   Status HandleOptions();
-#if !defined(NO_CHRONY)
-  Status AddNtpFlags(std::vector<std::string>* flags);
-#endif
+  Status AddTimeSourceFlags(std::vector<std::string>* flags);
 
   ExternalMiniClusterOptions opts_;
 
diff --git a/src/kudu/scripts/start_kudu.sh b/src/kudu/scripts/start_kudu.sh
index 23e9a18..31fd2d2 100755
--- a/src/kudu/scripts/start_kudu.sh
+++ b/src/kudu/scripts/start_kudu.sh
@@ -29,12 +29,14 @@ cat << EOF
 Usage:
 start_kudu.sh [flags]
 -h, --help         Print help
--m, --num-masters  Number of Kudu Master servers to start (default: 1)
+-m, --num-masters  Number of Kudu Masters to start (default: 1)
 -t, --num-tservers Number of Kudu Tablet Servers to start (default: 3)
 --rpc-master       RPC port of first Kudu Master; HTTP port is the next number.
                    Subsequent Masters will have following numbers
 --rpc-tserver      RPC port of first Kudu Tablet Server; HTTP port is the next
                    number. Subsequent Tablet Servers will have following numbers
+--time_source      Time source for Kudu Masters and Tablet Servers
+                   (default: system_unsync)
 -b, --builddir     Path to the Kudu build directory
 EOF
 }
@@ -43,6 +45,7 @@ NUM_MASTERS=1
 NUM_TSERVERS=3
 MASTER_RPC_PORT_BASE=8764
 TSERVER_RPC_PORT_BASE=9870
+TIME_SOURCE=system_unsync
 BUILDDIR="$PWD"
 echo $(readlink -f $(dirname $0))
 while (( "$#" )); do
@@ -67,6 +70,10 @@ while (( "$#" )); do
       TSERVER_RPC_PORT_BASE=$2
       shift 2
       ;;
+    --time_source)
+      TIME_SOURCE=$2
+      shift 2
+      ;;
     -b|--builddir)
       BUILDDIR="$2"
       shift 2
@@ -138,6 +145,7 @@ function start_master() {
   ARGS="$ARGS --fs_wal_dir=$dir_wal"
   ARGS="$ARGS --log_dir=$dir_log"
   ARGS="$ARGS --rpc_bind_addresses=$IP:$RPC_PORT"
+  ARGS="$ARGS --time_source=$TIME_SOURCE"
   ARGS="$ARGS --webserver_port=$HTTP_PORT"
   ARGS="$ARGS --webserver_interface=$IP"
   ARGS="$ARGS --webserver_doc_root=$WEBSERVER_DOC_ROOT"
@@ -155,6 +163,7 @@ function start_tserver() {
   ARGS="$ARGS --fs_wal_dir=$dir_wal"
   ARGS="$ARGS --log_dir=$dir_log"
   ARGS="$ARGS --rpc_bind_addresses=$IP:$RPC_PORT"
+  ARGS="$ARGS --time_source=$TIME_SOURCE"
   ARGS="$ARGS --webserver_port=$HTTP_PORT"
   ARGS="$ARGS --webserver_interface=$IP"
   ARGS="$ARGS --webserver_doc_root=$WEBSERVER_DOC_ROOT"
diff --git a/src/kudu/util/test_util.cc b/src/kudu/util/test_util.cc
index c41d1d1..ec9e23b 100644
--- a/src/kudu/util/test_util.cc
+++ b/src/kudu/util/test_util.cc
@@ -38,6 +38,7 @@
 
 #include <boost/optional/optional.hpp>
 #include <gflags/gflags.h>
+#include <gflags/gflags_declare.h>
 #include <glog/logging.h>
 #include <gtest/gtest-spi.h>
 
@@ -65,6 +66,8 @@ DEFINE_string(test_leave_files, "on_failure",
 
 DEFINE_int32(test_random_seed, 0, "Random seed to use for randomized tests");
 
+DECLARE_string(time_source);
+
 using boost::optional;
 using std::string;
 using std::vector;
@@ -74,7 +77,6 @@ namespace kudu {
 
 const char* kInvalidPath = "/dev/invalid-path-for-kudu-tests";
 static const char* const kSlowTestsEnvVar = "KUDU_ALLOW_SLOW_TESTS";
-static const char* const kUseSystemNtpEnvVar = "KUDU_USE_SYSTEM_NTP";
 
 static const uint64_t kTestBeganAtMicros = Env::Default()->NowMicros();
 
@@ -90,9 +92,9 @@ bool g_is_gtest = true;
 ///////////////////////////////////////////////////
 
 KuduTest::KuduTest()
-  : env_(Env::Default()),
-    flag_saver_(new google::FlagSaver()),
-    test_dir_(GetTestDataDirectory()) {
+    : env_(Env::Default()),
+      flag_saver_(new google::FlagSaver()),
+      test_dir_(GetTestDataDirectory()) {
   std::map<const char*, const char*> flags_for_tests = {
     // Disabling fsync() speeds up tests dramatically, and it's safe to do as no
     // tests rely on cutting power to a machine or equivalent.
@@ -109,10 +111,16 @@ KuduTest::KuduTest()
     {"ipki_server_key_size", "1024"},
     {"ipki_ca_key_size", "1024"},
     {"tsk_num_rsa_bits", "512"},
+    // For a generic Kudu test, the local wall-clock time is good enough even
+    // if it's not synchronized by NTP. All test components are run at the same
+    // node, so there aren't multiple time sources to synchronize.
+    {"time_source", "system_unsync"},
   };
   for (const auto& e : flags_for_tests) {
     // We don't check for errors here, because we have some default flags that
-    // only apply to certain tests.
+    // only apply to certain tests. If a flag is defined in a library which
+    // the test binary isn't linked with, then SetCommandLineOptionWithMode()
+    // reports an error since the flag is unknown to the gflags runtime.
     google::SetCommandLineOptionWithMode(e.first, e.second, google::SET_FLAGS_DEFAULT);
   }
   // If the TEST_TMPDIR variable has been set, then glog will automatically use that
@@ -196,10 +204,6 @@ bool AllowSlowTests() {
   return GetBooleanEnvironmentVariable(kSlowTestsEnvVar);
 }
 
-bool UseSystemNtp() {
-  return GetBooleanEnvironmentVariable(kUseSystemNtpEnvVar);
-}
-
 void OverrideFlagForSlowTests(const std::string& flag_name,
                               const std::string& new_value) {
   // Ensure that the flag is valid.
diff --git a/src/kudu/util/test_util.h b/src/kudu/util/test_util.h
index 7e9764d..d320e3e 100644
--- a/src/kudu/util/test_util.h
+++ b/src/kudu/util/test_util.h
@@ -87,13 +87,6 @@ class KuduTest : public ::testing::Test {
 // Returns true if slow tests are runtime-enabled.
 bool AllowSlowTests();
 
-// Returns true if tests should rely on the system clock synchronized by the
-// kernel NTP discipline. By default, test clusters should run their own
-// test NTP server and configure Kudu masters and tablet servers to use the
-// built-in NTP client as the clock source for HybridTime, where the built-in
-// NTP clients are pointed to the test cluster's dedicated NTP server.
-bool UseSystemNtp();
-
 // Override the given gflag to the new value, only in the case that
 // slow tests are enabled and the user hasn't otherwise overridden
 // it on the command line.


Mime
View raw message