kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From t...@apache.org
Subject kudu git commit: KUDU-1231. Add "unlock" flag for experimental and unsafe flags
Date Tue, 30 Aug 2016 01:29:23 GMT
Repository: kudu
Updated Branches:
  refs/heads/master 11ad075a1 -> ed216bcdf


KUDU-1231. Add "unlock" flag for experimental and unsafe flags

This adds two new flags:
  --unlock_experimental_flags
  --unlock_unsafe_flags

If a flag is tagged as 'unsafe' or 'experimental', and the user tries
to set this flag on the command line without the corresponding 'unlock'
flag being set, then the process will exit at startup with an error.

Example error output without flags unlocked:
  E0824 14:04:57.263624 14821 flags.cc:296] Flag --never_fsync is unsafe and unsupported.
  E0824 14:04:57.263749 14821 flags.cc:302] 1 unsafe flag(s) in use.
  E0824 14:04:57.263761 14821 flags.cc:303] Use --unlock_unsafe_flags to proceed at your own
risk.
  E0824 14:04:57.264104 14821 flags.cc:296] Flag --local_ip_for_outbound_sockets is experimental
and unsupported.
  E0824 14:04:57.264128 14821 flags.cc:302] 1 experimental flag(s) in use.
  E0824 14:04:57.264137 14821 flags.cc:303] Use --unlock_experimental_flags to proceed at
your own risk.
  <exits>

Example error output with flags unlocked:
  W0824 14:04:42.922560 14773 flags.cc:294] Enabled unsafe flag: --never_fsync=true
  W0824 14:04:42.923032 14773 flags.cc:294] Enabled experimental flag: --local_ip_for_outbound_sockets=127.0.0.1
  <continues>

Change-Id: Iec49e77fca604a7c5ee7501121a6263b7ee590d6
Reviewed-on: http://gerrit.cloudera.org:8080/4100
Reviewed-by: Adar Dembo <adar@cloudera.com>
Tested-by: Todd Lipcon <todd@apache.org>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/ed216bcd
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/ed216bcd
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/ed216bcd

Branch: refs/heads/master
Commit: ed216bcdf4dc62926eef7776dcf6d97129214c97
Parents: 11ad075
Author: Todd Lipcon <todd@apache.org>
Authored: Tue Aug 23 15:09:44 2016 -0700
Committer: Todd Lipcon <todd@apache.org>
Committed: Tue Aug 30 01:15:14 2016 +0000

----------------------------------------------------------------------
 docs/release_notes.adoc                         |  6 ++
 java/kudu-client/src/test/resources/flags       |  2 +
 python/kudu/tests/common.py                     |  4 ++
 src/kudu/client/client_samples-test.sh          |  2 +
 .../integration-tests/external_mini_cluster.cc  |  5 ++
 src/kudu/util/flag_tags-test.cc                 | 53 +++++++++++++++++
 src/kudu/util/flag_tags.h                       |  9 ++-
 src/kudu/util/flags.cc                          | 60 ++++++++++++++++++++
 8 files changed, 136 insertions(+), 5 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/ed216bcd/docs/release_notes.adoc
----------------------------------------------------------------------
diff --git a/docs/release_notes.adoc b/docs/release_notes.adoc
index 4060bd6..00bf69b 100644
--- a/docs/release_notes.adoc
+++ b/docs/release_notes.adoc
@@ -52,6 +52,12 @@ detailed below.
 - The KuduScanToken::TabletServers method in the C++ library has been removed.
   The same information can now be found in the KuduScanToken::tablet method.
 
+- Configuration flags which have been marked as 'unsafe' and 'experimental' are now
+  disallowed by default. Users may access these flags by enabling the additional
+  flags '--unlock_unsafe_flags' and '--unlock_experimental_flags'. Such flags
+  are not recommended and may be removed or modified with no deprecation period
+  and without notice in future Kudu releases.
+
 [[rn_0.10.0]]
 == Release notes specific to 0.10.0
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/ed216bcd/java/kudu-client/src/test/resources/flags
----------------------------------------------------------------------
diff --git a/java/kudu-client/src/test/resources/flags b/java/kudu-client/src/test/resources/flags
index 6752e43..34b73cb 100644
--- a/java/kudu-client/src/test/resources/flags
+++ b/java/kudu-client/src/test/resources/flags
@@ -1,3 +1,5 @@
 --v=1
 --logtostderr
 --never_fsync
+--unlock_experimental_flags
+--unlock_unsafe_flags
\ No newline at end of file

http://git-wip-us.apache.org/repos/asf/kudu/blob/ed216bcd/python/kudu/tests/common.py
----------------------------------------------------------------------
diff --git a/python/kudu/tests/common.py b/python/kudu/tests/common.py
index 2ab48e2..fee183a 100644
--- a/python/kudu/tests/common.py
+++ b/python/kudu/tests/common.py
@@ -53,6 +53,8 @@ class KuduTestBase(object):
 
         path = [
             "{0}/kudu-master".format(bin_path),
+            "-unlock_unsafe_flags",
+            "-unlock_experimental_flags",
             "-rpc_server_allow_ephemeral_ports",
             "-rpc_bind_addresses=0.0.0.0:0",
             "-fs_wal_dir={0}/master/data".format(local_path),
@@ -93,6 +95,8 @@ class KuduTestBase(object):
 
             path = [
                 "{0}/kudu-tserver".format(bin_path),
+                "-unlock_unsafe_flags",
+                "-unlock_experimental_flags",
                 "-rpc_server_allow_ephemeral_ports",
                 "-rpc_bind_addresses=0.0.0.0:0",
                 "-tserver_master_addrs=127.0.0.1:{0}".format(master_port),

http://git-wip-us.apache.org/repos/asf/kudu/blob/ed216bcd/src/kudu/client/client_samples-test.sh
----------------------------------------------------------------------
diff --git a/src/kudu/client/client_samples-test.sh b/src/kudu/client/client_samples-test.sh
index 534750b..ee4dc43 100755
--- a/src/kudu/client/client_samples-test.sh
+++ b/src/kudu/client/client_samples-test.sh
@@ -122,6 +122,7 @@ export TEST_TMPDIR=${TEST_TMPDIR:-$TMPDIR/kudutest-$UID}
 mkdir -p $TEST_TMPDIR
 BASE_DIR=$(mktemp -d $TEST_TMPDIR/client_samples-test.XXXXXXXX)
 $OUTPUT_DIR/kudu-master \
+  --unlock_experimental_flags \
   --default_num_replicas=1 \
   --log_dir=$BASE_DIR \
   --fs_wal_dir=$BASE_DIR/master \
@@ -131,6 +132,7 @@ $OUTPUT_DIR/kudu-master \
   --rpc_bind_addresses=$LOCALHOST_IP &
 MASTER_PID=$!
 $OUTPUT_DIR/kudu-tserver \
+  --unlock_experimental_flags \
   --log_dir=$BASE_DIR \
   --fs_wal_dir=$BASE_DIR/ts \
   --fs_data_dirs=$BASE_DIR/ts \

http://git-wip-us.apache.org/repos/asf/kudu/blob/ed216bcd/src/kudu/integration-tests/external_mini_cluster.cc
----------------------------------------------------------------------
diff --git a/src/kudu/integration-tests/external_mini_cluster.cc b/src/kudu/integration-tests/external_mini_cluster.cc
index 8123dcb..d77e1d3 100644
--- a/src/kudu/integration-tests/external_mini_cluster.cc
+++ b/src/kudu/integration-tests/external_mini_cluster.cc
@@ -568,6 +568,11 @@ Status ExternalDaemon::StartProcess(const vector<string>& user_flags)
{
   argv.push_back("--logtostderr");
   argv.push_back("--logbuflevel=-1");
 
+  // Allow unsafe and experimental flags from tests, since we often use
+  // fault injection, etc.
+  argv.push_back("--unlock_experimental_flags");
+  argv.push_back("--unlock_unsafe_flags");
+
   gscoped_ptr<Subprocess> p(new Subprocess(exe_, argv));
   p->ShareParentStdout(false);
   LOG(INFO) << "Running " << exe_ << "\n" << JoinStrings(argv, "\n");

http://git-wip-us.apache.org/repos/asf/kudu/blob/ed216bcd/src/kudu/util/flag_tags-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/flag_tags-test.cc b/src/kudu/util/flag_tags-test.cc
index ea16b1c..59d7437 100644
--- a/src/kudu/util/flag_tags-test.cc
+++ b/src/kudu/util/flag_tags-test.cc
@@ -21,6 +21,8 @@
 
 #include "kudu/gutil/map-util.h"
 #include "kudu/util/flag_tags.h"
+#include "kudu/util/flags.h"
+#include "kudu/util/logging_test_util.h"
 #include "kudu/util/test_util.h"
 
 DEFINE_int32(flag_with_no_tags, 0, "test flag that has no tags");
@@ -32,6 +34,12 @@ DEFINE_int32(flag_with_two_tags, 0, "test flag that has 2 tags");
 TAG_FLAG(flag_with_two_tags, evolving);
 TAG_FLAG(flag_with_two_tags, unsafe);
 
+DEFINE_bool(test_unsafe_flag, false, "an unsafe flag");
+TAG_FLAG(test_unsafe_flag, unsafe);
+
+DEFINE_bool(test_experimental_flag, false, "an experimental flag");
+TAG_FLAG(test_experimental_flag, experimental);
+
 using std::string;
 using std::unordered_set;
 
@@ -58,4 +66,49 @@ TEST_F(FlagTagsTest, TestTags) {
   EXPECT_EQ(0, tags.size());
 }
 
+TEST_F(FlagTagsTest, TestUnlockFlags) {
+  // Setting an unsafe flag without unlocking should crash.
+  {
+    gflags::FlagSaver s;
+    gflags::SetCommandLineOption("test_unsafe_flag", "true");
+    ASSERT_DEATH({ HandleCommonFlags(); },
+                 "Flag --test_unsafe_flag is unsafe and unsupported.*"
+                 "Use --unlock_unsafe_flags to proceed");
+  }
+
+  // Setting an unsafe flag with unlocking should proceed with a warning.
+  {
+    StringVectorSink sink;
+    ScopedRegisterSink reg(&sink);
+    gflags::FlagSaver s;
+    gflags::SetCommandLineOption("test_unsafe_flag", "true");
+    gflags::SetCommandLineOption("unlock_unsafe_flags", "true");
+    HandleCommonFlags();
+    ASSERT_EQ(1, sink.logged_msgs().size());
+    ASSERT_STR_CONTAINS(sink.logged_msgs()[0], "Enabled unsafe flag: --test_unsafe_flag");
+  }
+
+  // Setting an experimental flag without unlocking should crash.
+  {
+    gflags::FlagSaver s;
+    gflags::SetCommandLineOption("test_experimental_flag", "true");
+    ASSERT_DEATH({ HandleCommonFlags(); },
+                 "Flag --test_experimental_flag is experimental and unsupported.*"
+                 "Use --unlock_experimental_flags to proceed");
+  }
+
+  // Setting an experimental flag with unlocking should proceed with a warning.
+  {
+    StringVectorSink sink;
+    ScopedRegisterSink reg(&sink);
+    gflags::FlagSaver s;
+    gflags::SetCommandLineOption("test_experimental_flag", "true");
+    gflags::SetCommandLineOption("unlock_experimental_flags", "true");
+    HandleCommonFlags();
+    ASSERT_EQ(1, sink.logged_msgs().size());
+    ASSERT_STR_CONTAINS(sink.logged_msgs()[0],
+                        "Enabled experimental flag: --test_experimental_flag");
+  }
+}
+
 } // namespace kudu

http://git-wip-us.apache.org/repos/asf/kudu/blob/ed216bcd/src/kudu/util/flag_tags.h
----------------------------------------------------------------------
diff --git a/src/kudu/util/flag_tags.h b/src/kudu/util/flag_tags.h
index ed2dbfd..425fec7 100644
--- a/src/kudu/util/flag_tags.h
+++ b/src/kudu/util/flag_tags.h
@@ -39,9 +39,8 @@
 //         removed at any point. Users should not expect any compatibility
 //         of these flags.
 //
-//         TODO: we should add a new flag like -unlock_experimental_flags
-//         which would be required if the user wants to use any of these,
-//         similar to the JVM's -XX:+UnlockExperimentalVMOptions.
+//         Users must pass --unlock_experimental_flags to use any of these
+//         flags.
 //
 // - "hidden":
 //         These flags are for internal use only (e.g. testing) and should
@@ -59,8 +58,8 @@
 //         happening. These flags are automatically excluded from user-facing
 //         documentation even if they are not also marked 'hidden'.
 //
-//         TODO: we should add a flag -unlock_unsafe_flags which would be required
-//         to use any of these flags.
+//         Users must pass --unlock_unsafe_flags to use any of these
+//         flags.
 //
 // - "runtime":
 //         These flags can be safely changed at runtime via an RPC to the

http://git-wip-us.apache.org/repos/asf/kudu/blob/ed216bcd/src/kudu/util/flags.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/flags.cc b/src/kudu/util/flags.cc
index 7a5e1ed..5677dc0 100644
--- a/src/kudu/util/flags.cc
+++ b/src/kudu/util/flags.cc
@@ -63,6 +63,20 @@ DEFINE_bool(disable_core_dumps, false, "Disable core dumps when this process
cra
 TAG_FLAG(disable_core_dumps, advanced);
 TAG_FLAG(disable_core_dumps, evolving);
 
+DEFINE_bool(unlock_experimental_flags, false,
+            "Unlock flags marked as 'experimental'. These flags are not guaranteed to "
+            "be maintained across releases of Kudu, and may enable features or behavior "
+            "known to be unstable. Use at your own risk.");
+TAG_FLAG(unlock_experimental_flags, advanced);
+TAG_FLAG(unlock_experimental_flags, stable);
+
+DEFINE_bool(unlock_unsafe_flags, false,
+            "Unlock flags marked as 'unsafe'. These flags are not guaranteed to "
+            "be maintained across releases of Kudu, and enable features or behavior "
+            "known to be unsafe. Use at your own risk.");
+TAG_FLAG(unlock_unsafe_flags, advanced);
+TAG_FLAG(unlock_unsafe_flags, stable);
+
 // Tag a bunch of the flags that we inherit from glog/gflags.
 
 //------------------------------------------------------------
@@ -260,6 +274,50 @@ void ShowVersionAndExit() {
   exit(0);
 }
 
+// Check that, if any flags tagged with 'tag' have been specified to
+// non-default values, that 'unlocked' is true. If so (i.e. if the
+// flags have been appropriately unlocked), emits a warning message
+// for each flag and returns false. Otherwise, emits an error message
+// and returns true.
+bool CheckFlagsAndWarn(const string& tag, bool unlocked) {
+  vector<CommandLineFlagInfo> flags;
+  GetAllFlags(&flags);
+
+  int use_count = 0;
+  for (const auto& f : flags) {
+    if (f.is_default) continue;
+    unordered_set<string> tags;
+    GetFlagTags(f.name, &tags);
+    if (!ContainsKey(tags, tag)) continue;
+
+    if (unlocked) {
+      LOG(WARNING) << "Enabled " << tag << " flag: --" << f.name
<< "=" << f.current_value;
+    } else {
+      LOG(ERROR) << "Flag --" << f.name << " is " << tag <<
" and unsupported.";
+      use_count++;
+    }
+  }
+
+  if (!unlocked && use_count > 0) {
+    LOG(ERROR) << use_count << " " << tag << " flag(s) in use.";
+    LOG(ERROR) << "Use --unlock_" << tag << "_flags to proceed at your
own risk.";
+    return true;
+  }
+  return false;
+}
+
+// Check that any flags specified on the command line are allowed
+// to be set. This ensures that, if the user is using any unsafe
+// or experimental flags, they have explicitly unlocked them.
+void CheckFlagsAllowed() {
+  bool should_exit = false;
+  should_exit |= CheckFlagsAndWarn("unsafe", FLAGS_unlock_unsafe_flags);
+  should_exit |= CheckFlagsAndWarn("experimental", FLAGS_unlock_experimental_flags);
+  if (should_exit) {
+    exit(1);
+  }
+}
+
 } // anonymous namespace
 
 int ParseCommandLineFlags(int* argc, char*** argv, bool remove_flags) {
@@ -269,6 +327,8 @@ int ParseCommandLineFlags(int* argc, char*** argv, bool remove_flags)
{
 }
 
 void HandleCommonFlags() {
+  CheckFlagsAllowed();
+
   if (FLAGS_helpxml) {
     DumpFlagsXML();
   } else if (FLAGS_dump_metrics_json) {


Mime
View raw message