From 98f66fda463b998c8e53b8242d5febeb63b7e695 Mon Sep 17 00:00:00 2001 From: sfc-gh-tclinkenbeard Date: Sun, 9 Oct 2022 19:24:19 -0700 Subject: [PATCH 1/7] Add 'quota clear' fdbcli command --- fdbcli/QuotaCommand.actor.cpp | 33 +++++++++++++++++++++++++++++---- fdbclient/TagThrottle.actor.cpp | 2 +- 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/fdbcli/QuotaCommand.actor.cpp b/fdbcli/QuotaCommand.actor.cpp index e6a86e9b51..ba8125f75d 100644 --- a/fdbcli/QuotaCommand.actor.cpp +++ b/fdbcli/QuotaCommand.actor.cpp @@ -103,6 +103,21 @@ ACTOR Future setQuota(Reference db, TransactionTag tag, LimitTy } } +ACTOR Future clearQuota(Reference db, TransactionTag tag) { + state Reference tr = db->createTransaction(); + state Key key = tag.withPrefix(tagQuotaPrefix); + loop { + tr->setOption(FDBTransactionOptions::ACCESS_SYSTEM_KEYS); + try { + tr->clear(ThrottleApi::getTagQuotaKey(tag)); + wait(safeThreadFutureToFuture(tr->commit())); + return Void(); + } catch (Error& e) { + wait(safeThreadFutureToFuture(tr->onError(e))); + } + } +} + constexpr auto usage = "quota [get [reserved_throughput|total_throughput] | set " "[reserved_throughput|total_throughput] ]"; @@ -117,30 +132,40 @@ namespace fdb_cli { ACTOR Future quotaCommandActor(Reference db, std::vector tokens) { state bool result = true; - if (tokens.size() != 5 && tokens.size() != 6) { + if (tokens.size() < 3 || tokens.size() > 5) { return exitFailure(); } else { auto tag = parseTag(tokens[2]); - auto limitType = parseLimitType(tokens[3]); - if (!tag.present() || !limitType.present()) { + if (!tag.present()) { return exitFailure(); } if (tokens[1] == "get"_sr) { if (tokens.size() != 4) { return exitFailure(); } + auto const limitType = parseLimitType(tokens[3]); + if (!limitType.present()) { + return exitFailure(); + } wait(getQuota(db, tag.get(), limitType.get())); return true; } else if (tokens[1] == "set"_sr) { if (tokens.size() != 5) { return exitFailure(); } + auto const limitType = parseLimitType(tokens[3]); auto const limitValue = parseLimitValue(tokens[4]); - if (!limitValue.present()) { + if (!limitType.present() || !limitValue.present()) { return exitFailure(); } wait(setQuota(db, tag.get(), limitType.get(), limitValue.get())); return true; + } else if (tokens[1] == "clear"_sr) { + if (tokens.size() != 3) { + return exitFailure(); + } + wait(clearQuota(db, tag.get())); + return true; } else { return exitFailure(); } diff --git a/fdbclient/TagThrottle.actor.cpp b/fdbclient/TagThrottle.actor.cpp index 7a1712c4df..abf10c7e83 100644 --- a/fdbclient/TagThrottle.actor.cpp +++ b/fdbclient/TagThrottle.actor.cpp @@ -145,7 +145,7 @@ Value ThrottleApi::TagQuotaValue::toValue() const { ThrottleApi::TagQuotaValue ThrottleApi::TagQuotaValue::fromValue(ValueRef value) { auto tuple = Tuple::unpack(value); - if (tuple.size() != 4) { + if (tuple.size() != 2) { throw invalid_throttle_quota_value(); } TagQuotaValue result; From d67f3ca3b98a7025a0c0c293a46a77c901044d51 Mon Sep 17 00:00:00 2001 From: sfc-gh-tclinkenbeard Date: Sun, 9 Oct 2022 19:43:44 -0700 Subject: [PATCH 2/7] Update documentation for quota clear command --- design/global-tag-throttling.md | 6 ++++++ fdbcli/QuotaCommand.actor.cpp | 2 +- fdbcli/fdbcli.actor.cpp | 2 +- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/design/global-tag-throttling.md b/design/global-tag-throttling.md index ad7750a3cc..41ae6bdcda 100644 --- a/design/global-tag-throttling.md +++ b/design/global-tag-throttling.md @@ -47,6 +47,12 @@ Note that the quotas are specified in terms of bytes/second, and internally conv page_cost_quota = ceiling(byte_quota / CLIENT_KNOBS->READ_COST_BYTE_FACTOR) ``` +To clear a quota, run: + +``` +fdbcli> quota clear +``` + ### Limit Calculation The transaction budget that ratekeeper calculates and distributes to clients (via GRV proxies) for each tag is calculated based on several intermediate rate calculations, outlined in this section. diff --git a/fdbcli/QuotaCommand.actor.cpp b/fdbcli/QuotaCommand.actor.cpp index ba8125f75d..2626ddb2c2 100644 --- a/fdbcli/QuotaCommand.actor.cpp +++ b/fdbcli/QuotaCommand.actor.cpp @@ -119,7 +119,7 @@ ACTOR Future clearQuota(Reference db, TransactionTag tag) { } constexpr auto usage = "quota [get [reserved_throughput|total_throughput] | set " - "[reserved_throughput|total_throughput] ]"; + "[reserved_throughput|total_throughput] | clear ]"; bool exitFailure() { fmt::print(usage); diff --git a/fdbcli/fdbcli.actor.cpp b/fdbcli/fdbcli.actor.cpp index a5c2e2e75a..2ea89f8cd4 100644 --- a/fdbcli/fdbcli.actor.cpp +++ b/fdbcli/fdbcli.actor.cpp @@ -539,7 +539,7 @@ void initHelp() { "Displays the current read version of the database or currently running transaction."); helpMap["quota"] = CommandHelp("quota", "quota [get [reserved_throughput|total_throughput] | set " - "[reserved_throughput|total_throughput] ]", + "[reserved_throughput|total_throughput] | clear ]", "Get or modify the throughput quota for the specified tag."); helpMap["reset"] = CommandHelp("reset", From d0be1930f018e5dc48b240341dfe24919fd61671 Mon Sep 17 00:00:00 2001 From: sfc-gh-tclinkenbeard Date: Fri, 14 Oct 2022 11:54:38 -0700 Subject: [PATCH 3/7] Addressed review comments --- design/global-tag-throttling.md | 2 +- fdbcli/QuotaCommand.actor.cpp | 8 +++----- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/design/global-tag-throttling.md b/design/global-tag-throttling.md index 41ae6bdcda..c706013a3d 100644 --- a/design/global-tag-throttling.md +++ b/design/global-tag-throttling.md @@ -47,7 +47,7 @@ Note that the quotas are specified in terms of bytes/second, and internally conv page_cost_quota = ceiling(byte_quota / CLIENT_KNOBS->READ_COST_BYTE_FACTOR) ``` -To clear a quota, run: +To clear a both reserved and total throughput quotas for a tag, run: ``` fdbcli> quota clear diff --git a/fdbcli/QuotaCommand.actor.cpp b/fdbcli/QuotaCommand.actor.cpp index 2626ddb2c2..5f4f17418c 100644 --- a/fdbcli/QuotaCommand.actor.cpp +++ b/fdbcli/QuotaCommand.actor.cpp @@ -56,7 +56,7 @@ ACTOR Future getQuota(Reference db, TransactionTag tag, LimitTy loop { tr->setOption(FDBTransactionOptions::READ_SYSTEM_KEYS); try { - state ThreadFuture> resultFuture = tr->get(tag.withPrefix(tagQuotaPrefix)); + state ThreadFuture> resultFuture = tr->get(ThrottleApi::getTagQuotaKey(tag)); Optional v = wait(safeThreadFutureToFuture(resultFuture)); if (!v.present()) { fmt::print("\n"); @@ -77,11 +77,10 @@ ACTOR Future getQuota(Reference db, TransactionTag tag, LimitTy ACTOR Future setQuota(Reference db, TransactionTag tag, LimitType limitType, double value) { state Reference tr = db->createTransaction(); - state Key key = tag.withPrefix(tagQuotaPrefix); loop { tr->setOption(FDBTransactionOptions::ACCESS_SYSTEM_KEYS); try { - state ThreadFuture> resultFuture = tr->get(key); + state ThreadFuture> resultFuture = tr->get(ThrottleApi::getTagQuotaKey(tag)); Optional v = wait(safeThreadFutureToFuture(resultFuture)); ThrottleApi::TagQuotaValue quota; if (v.present()) { @@ -105,7 +104,6 @@ ACTOR Future setQuota(Reference db, TransactionTag tag, LimitTy ACTOR Future clearQuota(Reference db, TransactionTag tag) { state Reference tr = db->createTransaction(); - state Key key = tag.withPrefix(tagQuotaPrefix); loop { tr->setOption(FDBTransactionOptions::ACCESS_SYSTEM_KEYS); try { @@ -135,7 +133,7 @@ ACTOR Future quotaCommandActor(Reference db, std::vector 5) { return exitFailure(); } else { - auto tag = parseTag(tokens[2]); + auto const tag = parseTag(tokens[2]); if (!tag.present()) { return exitFailure(); } From 9a1876dd11ccf61fa0f3fac1c7c2c65725fe7af3 Mon Sep 17 00:00:00 2001 From: sfc-gh-tclinkenbeard Date: Fri, 14 Oct 2022 12:02:14 -0700 Subject: [PATCH 4/7] Update comment for fdbcli quota command --- fdbcli/fdbcli.actor.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fdbcli/fdbcli.actor.cpp b/fdbcli/fdbcli.actor.cpp index d100230061..c672ac52fb 100644 --- a/fdbcli/fdbcli.actor.cpp +++ b/fdbcli/fdbcli.actor.cpp @@ -543,7 +543,7 @@ void initHelp() { helpMap["quota"] = CommandHelp("quota", "quota [get [reserved_throughput|total_throughput] | set " "[reserved_throughput|total_throughput] | clear ]", - "Get or modify the throughput quota for the specified tag."); + "Get, modify, or clear the throughput quota for the specified tag."); helpMap["reset"] = CommandHelp("reset", "reset the current transaction", From 7e4f380423cce8227a0ad8328ee32bb107de514c Mon Sep 17 00:00:00 2001 From: sfc-gh-tclinkenbeard Date: Mon, 17 Oct 2022 16:27:45 -0700 Subject: [PATCH 5/7] Add tests for fdbcli quota commands --- fdbcli/tests/fdbcli_tests.py | 38 +++++++++++++++++++ fdbclient/TagThrottle.actor.cpp | 4 +- .../include/fdbclient/TagThrottle.actor.h | 4 +- 3 files changed, 42 insertions(+), 4 deletions(-) diff --git a/fdbcli/tests/fdbcli_tests.py b/fdbcli/tests/fdbcli_tests.py index b25386221d..42ac282a78 100755 --- a/fdbcli/tests/fdbcli_tests.py +++ b/fdbcli/tests/fdbcli_tests.py @@ -103,6 +103,42 @@ def maintenance(logger): output3 = run_fdbcli_command('maintenance') assert output3 == no_maintenance_output +@enable_logging() +def quota(logger): + command = 'quota get green total_throughput' + output = run_fdbcli_command(command) + logger.debug(command + ' : ' + output) + assert output == '' + + command = 'quota set green total_throughput 32768' + output = run_fdbcli_command(command) + logger.debug(command + ' : ' + output) + assert output == '' + + command = 'quota set green reserved_throughput 16384' + output = run_fdbcli_command(command) + logger.debug(command + ' : ' + output) + assert output == '' + + command = 'quota get green total_throughput' + output = run_fdbcli_command(command) + logger.debug(command + ' : ' + output) + assert output == '32768' + + command = 'quota get green reserved_throughput' + output = run_fdbcli_command(command) + logger.debug(command + ' : ' + output) + assert output == '16384' + + command = 'quota clear green' + output = run_fdbcli_command(command) + logger.debug(command + ' : ' + output) + assert output == '' + + command = 'quota get green total_throughput' + output = run_fdbcli_command(command) + logger.debug(command + ' : ' + output) + assert output == '' @enable_logging() def setclass(logger): @@ -1035,10 +1071,12 @@ if __name__ == '__main__': integer_options() tls_address_suffix() knobmanagement() + quota() else: assert args.process_number > 1, "Process number should be positive" coordinators() exclude() killall() + quota() # TODO: fix the failure where one process is not available after setclass call # setclass() diff --git a/fdbclient/TagThrottle.actor.cpp b/fdbclient/TagThrottle.actor.cpp index abf10c7e83..2bef9ae014 100644 --- a/fdbclient/TagThrottle.actor.cpp +++ b/fdbclient/TagThrottle.actor.cpp @@ -150,8 +150,8 @@ ThrottleApi::TagQuotaValue ThrottleApi::TagQuotaValue::fromValue(ValueRef value) } TagQuotaValue result; try { - result.reservedQuota = tuple.getDouble(0); - result.totalQuota = tuple.getDouble(1); + result.reservedQuota = tuple.getInt(0); + result.totalQuota = tuple.getInt(1); } catch (Error& e) { TraceEvent(SevWarnAlways, "TagQuotaValueFailedToDeserialize").error(e); throw invalid_throttle_quota_value(); diff --git a/fdbclient/include/fdbclient/TagThrottle.actor.h b/fdbclient/include/fdbclient/TagThrottle.actor.h index cf4ef8fd16..f5d480d8ec 100644 --- a/fdbclient/include/fdbclient/TagThrottle.actor.h +++ b/fdbclient/include/fdbclient/TagThrottle.actor.h @@ -597,8 +597,8 @@ Future enableAuto(Reference db, bool enabled) { class TagQuotaValue { public: - double reservedQuota{ 0.0 }; - double totalQuota{ 0.0 }; + int64_t reservedQuota{ 0 }; + int64_t totalQuota{ 0 }; bool isValid() const; Value toValue() const; static TagQuotaValue fromValue(ValueRef); From 8c082dac4edf62fee23a27ca2d20799560fa677f Mon Sep 17 00:00:00 2001 From: sfc-gh-tclinkenbeard Date: Mon, 17 Oct 2022 18:56:11 -0700 Subject: [PATCH 6/7] Expand quota tests in fdbcli_tests.py --- fdbcli/tests/fdbcli_tests.py | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/fdbcli/tests/fdbcli_tests.py b/fdbcli/tests/fdbcli_tests.py index 42ac282a78..9dc159cf19 100755 --- a/fdbcli/tests/fdbcli_tests.py +++ b/fdbcli/tests/fdbcli_tests.py @@ -105,11 +105,23 @@ def maintenance(logger): @enable_logging() def quota(logger): + # Should be a noop + command = 'quota clear green' + output = run_fdbcli_command(command) + logger.debug(command + ' : ' + output) + assert output == '' + command = 'quota get green total_throughput' output = run_fdbcli_command(command) logger.debug(command + ' : ' + output) assert output == '' + # Ignored update + command = 'quota set red total_throughput 49152' + output = run_fdbcli_command(command) + logger.debug(command + ' : ' + output) + assert output == '' + command = 'quota set green total_throughput 32768' output = run_fdbcli_command(command) logger.debug(command + ' : ' + output) @@ -140,6 +152,11 @@ def quota(logger): logger.debug(command + ' : ' + output) assert output == '' + # Too few arguments, should log help message + command = 'quota get green' + output = run_fdbcli_command(command) + logger.debug(command + ' : ' + output) + @enable_logging() def setclass(logger): # get all processes' network addresses @@ -1077,6 +1094,5 @@ if __name__ == '__main__': coordinators() exclude() killall() - quota() # TODO: fix the failure where one process is not available after setclass call # setclass() From 142d46400b3f91bbb88ba5a5dc5fb9bf45fbaf48 Mon Sep 17 00:00:00 2001 From: sfc-gh-tclinkenbeard Date: Tue, 18 Oct 2022 13:46:42 -0700 Subject: [PATCH 7/7] Reduce severity of trace events for local_config_changed --- fdbserver/worker.actor.cpp | 1 + flow/include/flow/genericactors.actor.h | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/fdbserver/worker.actor.cpp b/fdbserver/worker.actor.cpp index 4bebbc4d1d..be77fd8eaf 100644 --- a/fdbserver/worker.actor.cpp +++ b/fdbserver/worker.actor.cpp @@ -2802,6 +2802,7 @@ static std::set const& normalWorkerErrors() { if (s.empty()) { s.insert(error_code_please_reboot); s.insert(error_code_please_reboot_delete); + s.insert(error_code_local_config_changed); } return s; } diff --git a/flow/include/flow/genericactors.actor.h b/flow/include/flow/genericactors.actor.h index 5aecc04215..eb1ecabd40 100644 --- a/flow/include/flow/genericactors.actor.h +++ b/flow/include/flow/genericactors.actor.h @@ -83,7 +83,7 @@ Future> stopAfter(Future what) { ret = Optional(_); } catch (Error& e) { bool ok = e.code() == error_code_please_reboot || e.code() == error_code_please_reboot_delete || - e.code() == error_code_actor_cancelled; + e.code() == error_code_actor_cancelled || e.code() == error_code_local_config_changed; TraceEvent(ok ? SevInfo : SevError, "StopAfterError").error(e); if (!ok) { fprintf(stderr, "Fatal Error: %s\n", e.what());