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 <[email protected]> Tested-by: Todd Lipcon <[email protected]> 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 <[email protected]> Authored: Tue Aug 23 15:09:44 2016 -0700 Committer: Todd Lipcon <[email protected]> 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) {
