Node-wide throughput exemptions for clients #10755

dlex · 2023-05-15T06:39:29Z

This PR introduces a feature to make client_ids specified by a value, regex, or by a special selector for missing client_ids exempt from throughput control. Connections presenting a matching client_id value will not have the size of their requests accounted for throughput control, not they will be throttled.

The new configuration is forward looking to support the fully functional throughput control groups, this is why it may look too bulky for this specific PR.

Fixes #10575

Backports Required

Release Notes

Improvements

A client_id, or a group of client_ids specified with a regex, can now be excluded from node wide throughput control. The new cluster property kafka_throughput_control can be used to define throughput control groups for which Kafka traffic will not be limited by the values specified by kafka_throughput_limit_node_*_bps.

dlex · 2023-05-16T00:58:16Z

Force push: fixed a followup from #10285, avoid possible segfault when "kafka_throughput_controlled_api_keys" changes on the fly.

dlex · 2023-05-18T22:54:41Z

Force push:

example added to the configuration so that test_valid_settings does not fail on it
use the existing property_type_name instead of the new memeber name to define configuration type
validation function now returns std::nullopt for success as expected by property
error message extended in test_valid_settings

dlex · 2023-05-19T00:52:53Z

Force push: unit test fixed

dlex · 2023-05-20T02:28:28Z

CI failure in https://buildkite.com/redpanda/redpanda/builds/29432#01883193-77a7-4f46-bac1-be8fac94ff52

Exisinng: CI Failure (TimeoutError: Node docker-rp-xx draining leaderships) in BasicAuthUpgradeTest.test_upgrade_and_enable_basic_auth #10136

dlex · 2023-05-20T02:31:55Z

CI Failures in https://buildkite.com/redpanda/redpanda/builds/29432#01883193-77ab-4371-bfc8-daeb63c0840b:

dlex · 2023-05-20T04:13:11Z

/ci-repeat 3

dotnwat

looks great. just a few suggestions about simplifying some stuff like the std::variant usage but those aren't blockers or anything.

dotnwat · 2023-05-20T20:17:10Z

src/v/config/throughput_control_group.cc

+    struct unspecified_tag {};
+    std::variant<unspecified_tag, copyable_RE2> v;


is this what std::monostate is for, or would this be better represented as optional<RE2>?

is this what std::monostate is for

not exactly, monostate is more for "uninitalized" semantics while this one is specifically for "unspecified", that's why I used a separate tag. In theory, there may be more that just these two.

or would this be better represented as optional

it may, but the value of this type is wrapped into optional itself, so this variant is in part to avoid optional<optional<RE2>>

heh yeh, optional<optional<re2>> is sus, but generally i think the outcome is better when stuff is represented as directly as possible, even if it means weird stuff like nested optionals. in actual usages you can unwrap each layer of the optional and then add a comment or something about what is going on.

dotnwat · 2023-05-20T20:19:26Z

src/v/config/throughput_control_group.cc

-    return re2::RE2::FullMatch(
-      re2::StringPiece(client_id.c_str(), client_id.length()), *client_id_re);
+    return std::visit(
+      [client_id](auto&& v) -> bool {
+          using T = std::decay_t<decltype(v)>;
+          if constexpr (std::is_same_v<
+                          T,
+                          client_id_matcher_type::unspecified_tag>) {
+              // only missing client_id matches the unspecified tag
+              return !client_id;
+          } else if constexpr (std::is_same_v<T, copyable_RE2>) {
+              // missing client_id never matches a re
+              return client_id
+                     && re2::RE2::FullMatch(re2::StringPiece(*client_id), v);
+          } else {
+              static_assert(
+                always_false_v<T>, "non-exhaustive client_id_re_type visitor");
+          }
+      },
+      client_id_matcher->v);


since this PR introduces throughput_control_group.cc why have this other commit that rewrites a bunch of it? seems like this should be squashed with the second commit to avoid extra reviewing?

I was trying to split the large file into smaller parts to make reviewing easier :) If that's the other way around actually, I will squash

yeh i mean i think it's fine the way it is. i only mentioned it because it seemed like enough code was changing that it wasn't necessarily making review easier. but it isn't a problem either way.

dotnwat · 2023-05-20T20:21:43Z

src/v/config/throughput_control_group.cc

+    return std::visit(
+      [client_id](auto&& v) -> bool {
+          using T = std::decay_t<decltype(v)>;
+          if constexpr (std::is_same_v<
+                          T,
+                          client_id_matcher_type::unspecified_tag>) {
+              // only missing client_id matches the unspecified tag
+              return !client_id;
+          } else if constexpr (std::is_same_v<T, copyable_RE2>) {
+              // missing client_id never matches a re
+              return client_id
+                     && re2::RE2::FullMatch(re2::StringPiece(*client_id), v);
+          } else {
+              static_assert(
+                always_false_v<T>, "non-exhaustive client_id_re_type visitor");
+          }
+      },


this could be if std::holds_alternative instead of using visit with constexpr etc... but maybe using optional woudl be even simpler?

I've compared the above code with holds_alternative approach:

if (std::holds_alternative<client_id_matcher_type::unspecified_tag>( client_id_matcher->v)) { // only missing client_id matches the unspecified tag return !client_id; } if (auto* const v = std::get_if<copyable_RE2>(&client_id_matcher->v); v) { // missing client_id never matches a re return client_id && re2::RE2::FullMatch(re2::StringPiece(*client_id), *v); } assert(false, "non-exhaustive client_id_re_type visitor");

So it's a comparable amount of code, a bit simpler, but it has an important drawback: a runtime fallback case. With visit, a missed case will cause compilation to fail, but with holds_alternative it will be a runtime assert/soft assert.

maybe using optional woudl be even simpler?

+1

src/v/config/throughput_control_group.cc

src/v/kafka/server/snc_quota_manager.cc

dotnwat · 2023-05-20T20:32:46Z

src/v/kafka/server/snc_quota_manager.cc

+      client_id);
+    if (tcgroup_it == _kafka_throughput_control().cend()) {
+        ctx->_exempt = false;
+        vlog(klog.info, "qm - No throghput control group assigned");


should these be at info level?

In the normal course that would happen once per connection lifetime, so I think info is fine. Unless we have supported clients that open and drop kafka connections all the time.

do we log anything else at connection establishment at info level? i don't think we do...

we log at INFO at connection loss here, but that's an exceptional future handling, so that does not happen on graceful connection close. Ok I'll demote it to DEBUG to align with the rest of connection event

src/v/kafka/server/snc_quota_manager.h

dlex · 2023-05-23T06:28:18Z

Force push:

dropped variant in favour of optional
hid snc_quota_context
spelling

dlex · 2023-05-23T06:30:21Z

Force push: rebased upstream

dlex · 2023-05-23T18:16:11Z

Force push:

snc_quota_manager_context returned to the header

dlex · 2023-05-23T19:44:08Z

CI failure in https://buildkite.com/redpanda/redpanda/builds/29707#018849e9-ac22-48fd-aab9-0e25ecebf15b:

existing: CI Failure (Controller logs are not the same) in ConfigurationUpdateTest.test_two_nodes_update #10867

dlex · 2023-05-28T21:25:56Z

Ci failures in https://buildkite.com/redpanda/redpanda/builds/30045#01885b1e-db12-450d-bee9-e7fc3f71eac7:

existing: CI Failure (TimeoutError in wait_for_partitions_rebalanced) in ScalingUpTest.test_adding_nodes_to_cluster #11042

dlex · 2023-05-28T21:27:21Z

CI failures in https://buildkite.com/redpanda/redpanda/builds/30045#01885b1e-db19-4c8b-81d5-cf6ba965e5a0:

existing: CI Failure (consumer timed out waiting for offsets) in ControllerLogLimitMirrorMakerTests.test_mirror_maker_with_limits #10502

dlex · 2023-05-29T02:34:28Z

CI failures in https://buildkite.com/redpanda/redpanda/builds/30045#01886442-4e6d-4328-b0bf-5656d157f15b:

dlex · 2023-05-30T21:44:15Z

Force push: a typo fixed in property description

Followup to redpanda-data#10285. There the intake traffic point was left unconditional of the api key, which caused anomalities in tput related metrics.

throughput_control_group is a structural element for tput control groups configuration. This commit adds its implementation sufficient to cover tput exemptions by client_id matched with a regex.

always_false_v moved to utils/functional.h so that it can be reused

Client can omit client_id specification on its side. This is different from empty client_id value, and for throughput_control_group it is also different from omitting client_id (which means match anything regardless of client_id). This commit introduces special selector tags for client_id value in throughput_control_group. The syntax for selector tags is "+name", this makes them distinct from any regex because regex cannot start with a "+". One selector name is introduced: "empty", it matches the omitted client_id values and only them. To support that, throughput_control_group does not store a regex directly anymore. Now it's a variant type with one option for "empty" and another for the regex.

also a typo fixed in another description

connection_context now has all quota related stuff for the connection stored in `_snc_quota_context`. This object is supposed to be created once per connection lifetime by `snc_quota_manager`, but it will be recreated each time a client_id changes on the connection. When the quota context is created (lazily on the connection context), the `kafka_throughput_control` rules are used to select the matching throughput control group. If any group is matched, the context saves it as a flag to exempt the connection from any snc_quota_manager control. This will change into a full association with the control group. Currently the exempt flag simply tells the quota manager to skip any messages in that context.

Set some possible values for `kafka_throughput_control` in the configurations test

dlex · 2023-06-02T16:26:34Z

Force push: rebased upstream

dotnwat · 2023-06-02T20:56:48Z

/ci-repeat 5

dotnwat · 2023-06-03T18:40:34Z

Failures

vbotbuildovich · 2023-06-03T21:00:21Z

/backport v23.1.x

vbotbuildovich · 2023-06-03T21:01:17Z

Failed to run cherry-pick command. I executed the commands below:

git checkout -b backport-pr-10755-v23.1.x-706 remotes/upstream/v23.1.x
git cherry-pick -x 16a6cd0bd56bb16b3aa31a8b1577d97c10655a07 a1cc8f7b9d6d68e5a3efe1dc09b615cd61a82560 bd43f4727a9887bad52d8b56c77934e755c69a67 b1fa02e1b41bc76ba57409c6106b295a67fcbdcc 7af5c60a2836e65233c433c9e068faf7c1ea302c 2ded90fd6d1ed1b7f05a07c7353a190684386844 cd9b9518bdf395e695fbedba52bca74e6b523b0e edd1f11ebd671b97b4d36f3c35e159577433a3b5 529cbf39c14e32fca87352063e951f8f39cc30b5

Workflow run logs.

dlex requested review from dotnwat and graphcareful May 15, 2023 06:39

github-actions bot added the area/redpanda label May 15, 2023

dlex removed the request for review from graphcareful May 15, 2023 06:40

dlex marked this pull request as ready for review May 15, 2023 06:41

dlex added this to the v23.2.1 milestone May 15, 2023

dlex marked this pull request as draft May 15, 2023 17:39

dlex force-pushed the 10575_tp-exemptions-for-clients branch from 6d19d35 to 99cfeb4 Compare May 15, 2023 18:04

dlex marked this pull request as ready for review May 15, 2023 18:18

dlex requested a review from BenPope May 15, 2023 18:20

dlex force-pushed the 10575_tp-exemptions-for-clients branch from 99cfeb4 to c2174e0 Compare May 16, 2023 00:55

dotnwat requested review from ZeDRoman and graphcareful May 16, 2023 04:26

dlex force-pushed the 10575_tp-exemptions-for-clients branch from c2174e0 to bfe47f0 Compare May 18, 2023 22:51

dlex force-pushed the 10575_tp-exemptions-for-clients branch from bfe47f0 to 64fb201 Compare May 19, 2023 00:51

dotnwat reviewed May 20, 2023

View reviewed changes

dlex force-pushed the 10575_tp-exemptions-for-clients branch from 64fb201 to c975ac2 Compare May 23, 2023 06:26

dlex force-pushed the 10575_tp-exemptions-for-clients branch from c975ac2 to 1cd7048 Compare May 23, 2023 06:30

dlex force-pushed the 10575_tp-exemptions-for-clients branch from 1cd7048 to 9e375ba Compare May 23, 2023 18:15

dlex requested a review from dotnwat May 23, 2023 19:45

dlex requested a review from dotnwat May 30, 2023 04:09

dlex force-pushed the 10575_tp-exemptions-for-clients branch from 07786f1 to cdaf180 Compare May 30, 2023 21:43

dlex added 9 commits June 2, 2023 12:02

f/quotas: do not record intake traffic for noncontrolled api keys

16a6cd0

Followup to redpanda-data#10285. There the intake traffic point was left unconditional of the api key, which caused anomalities in tput related metrics.

config: throughput_control_group +UT

a1cc8f7

throughput_control_group is a structural element for tput control groups configuration. This commit adds its implementation sufficient to cover tput exemptions by client_id matched with a regex.

cloud_storage_clients: move always_false_v -> utils

bd43f47

always_false_v moved to utils/functional.h so that it can be reused

config: "kafka_throughput_control" cluster property

7af5c60

also a typo fixed in another description

k/quotas: binding for "kafka_throughput_control"

2ded90f

tests: enhance error message in test_valid_settings

edd1f11

k/quotas: add kafka_throughput_control to test_configurations

529cbf3

Set some possible values for `kafka_throughput_control` in the configurations test

dlex added the doc-needed label Jun 2, 2023

dlex force-pushed the 10575_tp-exemptions-for-clients branch from cdaf180 to 529cbf3 Compare June 2, 2023 16:26

dotnwat approved these changes Jun 3, 2023

View reviewed changes

dotnwat merged commit 750bc90 into redpanda-data:dev Jun 3, 2023

vbotbuildovich mentioned this pull request Jun 3, 2023

[v23.1.x] Node-wide throughput exemptions for clients #11188

Closed

dlex mentioned this pull request Jun 5, 2023

[v23.1.x] Node-wide throughput exemptions for clients #11213

Merged

7 tasks

dlex deleted the 10575_tp-exemptions-for-clients branch June 7, 2023 21:26

This was referenced Jun 10, 2023

CI Failure (Bad Request for url: /v1/cluster_config) in ThroughputLimitsSnc.test_configuration #11338

Closed

throughput control groups: fixed zero byte dumped to the log by noname group #11340

Merged

pgellert mentioned this pull request May 7, 2024

CORE-2752 - Fix Kafka quota throttling delay enforcement #18218

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node-wide throughput exemptions for clients #10755

Node-wide throughput exemptions for clients #10755

dlex commented May 15, 2023 •

edited

Loading

dlex commented May 16, 2023

dlex commented May 18, 2023

dlex commented May 19, 2023

dlex commented May 20, 2023

dlex commented May 20, 2023

dlex commented May 20, 2023

dotnwat left a comment

dotnwat May 20, 2023

dlex May 22, 2023 •

edited

Loading

dotnwat May 23, 2023 •

edited

Loading

dotnwat May 20, 2023

dlex May 22, 2023

dotnwat May 23, 2023

dotnwat May 20, 2023

dlex May 23, 2023

dlex May 23, 2023

dotnwat May 20, 2023

dlex May 23, 2023

dotnwat May 24, 2023

dlex May 25, 2023 •

edited

Loading

dlex commented May 23, 2023

dlex commented May 23, 2023

dlex commented May 23, 2023

dlex commented May 23, 2023

dlex commented May 28, 2023

dlex commented May 28, 2023

dlex commented May 29, 2023

dlex commented May 30, 2023

dlex commented Jun 2, 2023

dotnwat commented Jun 2, 2023

dotnwat commented Jun 3, 2023

vbotbuildovich commented Jun 3, 2023

vbotbuildovich commented Jun 3, 2023

		struct unspecified_tag {};
		std::variant<unspecified_tag, copyable_RE2> v;

Node-wide throughput exemptions for clients #10755

Node-wide throughput exemptions for clients #10755

Conversation

dlex commented May 15, 2023 • edited Loading

Backports Required

Release Notes

Improvements

dlex commented May 16, 2023

dlex commented May 18, 2023

dlex commented May 19, 2023

dlex commented May 20, 2023

dlex commented May 20, 2023

dlex commented May 20, 2023

dotnwat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlex May 22, 2023 • edited Loading

Choose a reason for hiding this comment

dotnwat May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlex May 25, 2023 • edited Loading

Choose a reason for hiding this comment

dlex commented May 23, 2023

dlex commented May 23, 2023

dlex commented May 23, 2023

dlex commented May 23, 2023

dlex commented May 28, 2023

dlex commented May 28, 2023

dlex commented May 29, 2023

dlex commented May 30, 2023

dlex commented Jun 2, 2023

dotnwat commented Jun 2, 2023

dotnwat commented Jun 3, 2023

vbotbuildovich commented Jun 3, 2023

vbotbuildovich commented Jun 3, 2023

dlex commented May 15, 2023 •

edited

Loading

dlex May 22, 2023 •

edited

Loading

dotnwat May 23, 2023 •

edited

Loading

dlex May 25, 2023 •

edited

Loading