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
The following commit(s) were added to refs/heads/master by this push:
new 943414d36 [Flag] Check flags consistency when setting a flag
943414d36 is described below
commit 943414d3658ede971eddd373bbb80d8a50f4e8c8
Author: xinghuayu007 <[email protected]>
AuthorDate: Sat Nov 12 21:38:11 2022 +0800
[Flag] Check flags consistency when setting a flag
There are existing some dependencies between some flags.
For example, if setting auto_rebalancing_enabled to true,
unlock_experimental_flags must be set to true. Currently
Kudu will not check flags consistency when setting a flag
using GenericService::SetFlag() RPC. That will cause unsafe
flags or experimental flags or flag group validators
unavailable.
Therefore this patch will check flags consistency when
setting a flag. First try to set this flag, then check
all flags consistency, if failed, roll back. This patch
supports to check unsafe flags, experimental flags,
flag group validators. Standard validator defined by
DEFINE_validator() will be checked when calling
google::SetCommandLineOption().
By the way, to support a group flag, for example,
rpc_certificate_file, rpc_private_key_file, and
rpc_ca_certificate_file, which all should be set at the same
time, this patch will add a parameter: --skip_check_consistency
to skip check consistency. There they can be set one by one.
Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
Reviewed-on: http://gerrit.cloudera.org:8080/19237
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <[email protected]>
---
src/kudu/server/generic_service.cc | 31 ++++++++---
src/kudu/server/server_base.proto | 2 +
src/kudu/tools/kudu-tool-test.cc | 98 +++++++++++++++++++++++++++++++++++
src/kudu/tools/tool_action_common.cc | 4 ++
src/kudu/tools/tool_action_master.cc | 2 +-
src/kudu/tools/tool_action_tserver.cc | 1 +
src/kudu/util/flags.cc | 23 ++++++++
src/kudu/util/flags.h | 5 ++
8 files changed, 157 insertions(+), 9 deletions(-)
diff --git a/src/kudu/server/generic_service.cc
b/src/kudu/server/generic_service.cc
index 475fe373a..eddc1d495 100644
--- a/src/kudu/server/generic_service.cc
+++ b/src/kudu/server/generic_service.cc
@@ -21,6 +21,7 @@
#include <string>
#include <unordered_set>
#include <utility>
+// IWYU pragma: no_include <unordered_map>
#include <gflags/gflags.h>
#include <glog/logging.h>
@@ -28,7 +29,6 @@
#include "kudu/clock/clock.h"
#include "kudu/clock/hybrid_clock.h"
#include "kudu/clock/mock_ntp.h"
-#include "kudu/clock/time_service.h"
#include "kudu/common/timestamp.h"
#include "kudu/common/wire_protocol.h"
#include "kudu/gutil/casts.h"
@@ -49,6 +49,7 @@ DECLARE_bool(use_hybrid_clock);
using std::string;
using std::unordered_set;
+using strings::Substitute;
namespace kudu {
namespace server {
@@ -150,14 +151,28 @@ void GenericServiceImpl::SetFlag(const SetFlagRequestPB*
req,
if (ret.empty()) {
resp->set_result(SetFlagResponsePB::BAD_VALUE);
resp->set_msg("Unable to set flag: bad value");
- } else {
- LOG(INFO) << rpc->requestor_string() << " changed flags via RPC: "
- << req->flag() << " from '" << old_val << "' to '"
- << req->value() << "'";
- resp->set_result(SetFlagResponsePB::SUCCESS);
- resp->set_msg(ret);
+ rpc->RespondSuccess();
+ return;
}
-
+ // Validate all flags.
+ if (req->run_consistency_check() && !AreFlagsConsistent()) {
+ string res = google::SetCommandLineOption(
+ req->flag().c_str(),
+ old_val.c_str());
+ if (ret.empty()) {
+ LOG(WARNING) << Substitute("Failed to roll back flag '$0' to previous
value '$1'",
+ req->flag(), old_val);
+ }
+ resp->set_result(SetFlagResponsePB::BAD_VALUE);
+ resp->set_msg("Detected inconsistency in command-line flags");
+ rpc->RespondSuccess();
+ return;
+ }
+ LOG(INFO) << rpc->requestor_string() << " changed flags via RPC: "
+ << req->flag() << " from '" << old_val << "' to '"
+ << req->value() << "'";
+ resp->set_result(SetFlagResponsePB::SUCCESS);
+ resp->set_msg(ret);
rpc->RespondSuccess();
}
diff --git a/src/kudu/server/server_base.proto
b/src/kudu/server/server_base.proto
index 8242a68cb..c9f42e0fb 100644
--- a/src/kudu/server/server_base.proto
+++ b/src/kudu/server/server_base.proto
@@ -77,6 +77,8 @@ message SetFlagRequestPB {
// at runtime. This can cause crashes or other bad behavior, so should
// only be used as a last resort.
optional bool force = 3 [default = false];
+ // Check all flags for consistency upon setting a new value for a flag.
+ optional bool run_consistency_check = 4 [default = false];
}
message SetFlagResponsePB {
diff --git a/src/kudu/tools/kudu-tool-test.cc b/src/kudu/tools/kudu-tool-test.cc
index 48c763cdb..63e0a4942 100644
--- a/src/kudu/tools/kudu-tool-test.cc
+++ b/src/kudu/tools/kudu-tool-test.cc
@@ -4374,6 +4374,104 @@ TEST_F(ToolTest, TestLocalReplicaCMetaOps) {
}
}
+TEST_F(ToolTest, TestServerSetFlag) {
+ NO_FATALS(StartExternalMiniCluster());
+ string master_addr = cluster_->master()->bound_rpc_addr().ToString();
+ { // Test set unsafe flag.
+ NO_FATALS(RunActionStdoutNone(
+ Substitute("master set_flag $0 unlock_unsafe_flags true --force",
+ master_addr)));
+ NO_FATALS(RunActionStdoutNone(
+ Substitute("master set_flag $0 enable_data_block_fsync true --force",
+ master_addr)));
+ // Test invalid unsafe flag.
+ string err;
+ RunActionStderrString(
+ Substitute("master set_flag $0 unlock_unsafe_flags false --force",
+ master_addr), &err);
+ ASSERT_STR_CONTAINS(err,
+ "Detected inconsistency in command-line flags");
+ // Flag value will not be changed.
+ string out;
+ RunActionStdoutString(
+ Substitute("master get_flags $0 --flags=unlock_unsafe_flags
--format=json",
+ master_addr), &out);
+ rapidjson::Document doc;
+ doc.Parse<0>(out.c_str());
+ const string flag_value = "true";
+ for (int i = 0; i < doc.Size(); i++) {
+ const rapidjson::Value& item = doc[i];
+ ASSERT_TRUE(item["value"].GetString() == flag_value);
+ }
+ }
+ { // Test set experimental flag.
+ NO_FATALS(RunActionStdoutNone(
+ Substitute("master set_flag $0 unlock_experimental_flags true --force",
+ master_addr)));
+ NO_FATALS(RunActionStdoutNone(
+ Substitute("master set_flag $0 auto_rebalancing_enabled true --force",
+ master_addr)));
+ // Test invalid experimental flag.
+ string err;
+ RunActionStderrString(
+ Substitute("master set_flag $0 unlock_experimental_flags false
--force",
+ master_addr), &err);
+ ASSERT_STR_CONTAINS(err,
+ "Detected inconsistency in command-line flags");
+ // Flag value will not be changed.
+ string out;
+ RunActionStdoutString(
+ Substitute("master get_flags $0 --flags=unlock_experimental_flags
--format=json",
+ master_addr), &out);
+ rapidjson::Document doc;
+ doc.Parse<0>(out.c_str());
+ const string flag_value = "true";
+ for (int i = 0; i < doc.Size(); i++) {
+ const rapidjson::Value& item = doc[i];
+ ASSERT_TRUE(item["value"].GetString() == flag_value);
+ }
+ }
+ { // Test set flag using group flag validator.
+ NO_FATALS(RunActionStdoutNone(
+ Substitute("master set_flag $0 unlock_experimental_flags true --force",
+ master_addr)));
+ NO_FATALS(RunActionStdoutNone(
+ Substitute("master set_flag $0
raft_prepare_replacement_before_eviction true --force",
+ master_addr)));
+ NO_FATALS(RunActionStdoutNone(
+ Substitute("master set_flag $0 auto_rebalancing_enabled true --force",
+ master_addr)));
+ // Test set flag violating group validator.
+ string err;
+ RunActionStderrString(
+ Substitute("master set_flag $0
raft_prepare_replacement_before_eviction false --force",
+ master_addr), &err);
+ NO_FATALS(RunActionStdoutNone(
+ Substitute("master set_flag $0 auto_rebalancing_enabled true --force",
+ master_addr)));
+ ASSERT_STR_CONTAINS(err, "Detected inconsistency in command-line flags");
+ // Flag value will not be changed.
+ string out;
+ RunActionStdoutString(
+ Substitute("master get_flags $0
--flags=raft_prepare_replacement_before_eviction"
+ " --format=json",
+ master_addr), &out);
+ rapidjson::Document doc;
+ doc.Parse<0>(out.c_str());
+ const string flag_value = "true";
+ for (int i = 0; i < doc.Size(); i++) {
+ const rapidjson::Value& item = doc[i];
+ ASSERT_TRUE(item["value"].GetString() == flag_value);
+ }
+ }
+ { // Test set flag using parameter: --run_consistency_check.
+ NO_FATALS(RunActionStdoutNone(
+ Substitute("master set_flag $0 rpc_certificate_file test "
+ "--force --run_consistency_check=false",
+ master_addr)));
+ }
+}
+
TEST_F(ToolTest, TestTserverList) {
NO_FATALS(StartExternalMiniCluster());
diff --git a/src/kudu/tools/tool_action_common.cc
b/src/kudu/tools/tool_action_common.cc
index d2969ac6a..b7dd58b76 100644
--- a/src/kudu/tools/tool_action_common.cc
+++ b/src/kudu/tools/tool_action_common.cc
@@ -101,6 +101,9 @@ DEFINE_bool(force, false, "If true, allows the set_flag
command to set a flag "
"which is not explicitly marked as runtime-settable. Such flag "
"changes may be simply ignored on the server, or may cause the "
"server to crash.");
+DEFINE_bool(run_consistency_check, true, "If true, Kudu server checks all
flags for consistency "
+ "upon setting a flag. In this mode, the server rolls the flag back
to its previous "
+ "value and sends corresponding error response if an inconsistency
is detected.");
DEFINE_bool(print_meta, true, "Include metadata in output");
DEFINE_string(print_entries, "decoded",
"How to print entries:\n"
@@ -628,6 +631,7 @@ Status SetServerFlag(const string& address, uint16_t
default_port,
req.set_flag(flag);
req.set_value(value);
req.set_force(FLAGS_force);
+ req.set_run_consistency_check(FLAGS_run_consistency_check);
RETURN_NOT_OK(proxy->SetFlag(req, &resp, &rpc));
switch (resp.result()) {
diff --git a/src/kudu/tools/tool_action_master.cc
b/src/kudu/tools/tool_action_master.cc
index da1d551bb..2a41f5c8f 100644
--- a/src/kudu/tools/tool_action_master.cc
+++ b/src/kudu/tools/tool_action_master.cc
@@ -49,7 +49,6 @@
#include "kudu/master/master.proxy.h"
#include "kudu/master/master_runner.h"
#include "kudu/master/sys_catalog.h"
-#include "kudu/rpc/response_callback.h"
#include "kudu/rpc/rpc_controller.h"
#include "kudu/tools/ksck.h"
#include "kudu/tools/ksck_remote.h"
@@ -824,6 +823,7 @@ unique_ptr<Mode> BuildMasterMode() {
.AddRequiredParameter({ kFlagArg, "Name of the gflag" })
.AddRequiredParameter({ kValueArg, "New value for the gflag" })
.AddOptionalParameter("force")
+ .AddOptionalParameter("run_consistency_check")
.Build();
builder.AddAction(std::move(set_flag));
}
diff --git a/src/kudu/tools/tool_action_tserver.cc
b/src/kudu/tools/tool_action_tserver.cc
index 4aeb96247..99603a4ce 100644
--- a/src/kudu/tools/tool_action_tserver.cc
+++ b/src/kudu/tools/tool_action_tserver.cc
@@ -422,6 +422,7 @@ unique_ptr<Mode> BuildTServerMode() {
.AddRequiredParameter({ kFlagArg, "Name of the gflag" })
.AddRequiredParameter({ kValueArg, "New value for the gflag" })
.AddOptionalParameter("force")
+ .AddOptionalParameter("run_consistency_check")
.Build();
unique_ptr<Action> set_flag_for_all =
diff --git a/src/kudu/util/flags.cc b/src/kudu/util/flags.cc
index 48a9eab72..f7925a4c2 100644
--- a/src/kudu/util/flags.cc
+++ b/src/kudu/util/flags.cc
@@ -437,6 +437,16 @@ void CheckFlagsAllowed() {
}
}
+bool CheckCustomValidators() {
+ const auto& validators(GetFlagValidators());
+ for (const auto& e : validators) {
+ if (!e.second()) {
+ return true;
+ }
+ }
+ return false;
+}
+
// Run 'late phase' custom validators: these can be run only when all flags are
// already parsed and individually validated.
void RunCustomValidators() {
@@ -543,6 +553,19 @@ void ValidateFlags() {
RunCustomValidators();
}
+bool AreFlagsConsistent() {
+ if (CheckFlagsAndWarn("unsafe", FLAGS_unlock_unsafe_flags)) {
+ return false;
+ }
+ if (CheckFlagsAndWarn("experimental", FLAGS_unlock_experimental_flags)) {
+ return false;
+ }
+ if (CheckCustomValidators()) {
+ return false;
+ }
+ return true;
+}
+
string CommandlineFlagsIntoString(EscapeMode mode) {
string ret_value;
vector<CommandLineFlagInfo> flags;
diff --git a/src/kudu/util/flags.h b/src/kudu/util/flags.h
index b88c25d26..91e8a24dd 100644
--- a/src/kudu/util/flags.h
+++ b/src/kudu/util/flags.h
@@ -57,6 +57,11 @@ void HandleCommonFlags();
// logging will write to stderr.
void ValidateFlags();
+// Check for unsafe and experimental flags, and run all group flag validators.
+// Returns 'true' if all checks pass and no inconsistencies are detected,
+// 'false' otherwise.
+bool AreFlagsConsistent();
+
enum class EscapeMode {
HTML,
NONE
