local rate limit: add cross local cluster rate limit support #34276

wbpcode · 2024-05-21T12:27:29Z

Commit Message: local rate limit: add cross local cluster rate limit support
Additional Description:

Envoy provides lots of rate limit related filters/features. The global rate limit provides the most powerful feature. But it also introduce additional dependencies (rate limit server, redis, etc) and latency (additional calling to rate limit server).

The local rate limit is more stable and has better performance, and has no dependency to external server. But the local rate limit is work at single instance or connection scope. That means that if local rate limit is used, we cannot get a stable total limit for a Envoy cluster. Because the total limit of local rate limit filter is single instance limit multiply the instance number of Envoy. But the instance number may changed when the cluster scaling.

This PR add a new interesting feature. It make the local rate limit filter could aware the membership of the local cluster (the cluster contains current Envoy self). That means the local rate limit could compute it's tokens based on the membership of local cluster and achieve the target to share the total limit between multiple Envoy instances (a Envoy cluster).

See #34230 for more discussion.

Risk Level: low. (nothing will be changed if we don't enable the feature explicitly)
Testing: unit.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

Signed-off-by: wbpcode <[email protected]>

…ross-instance-local-limit

repokitteh-read-only · 2024-05-21T12:27:38Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #34276 was opened by wbpcode.

see: more, trace.

Signed-off-by: wbpcode <[email protected]>

api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto

Signed-off-by: wbpcode <[email protected]>

api/envoy/extensions/common/ratelimit/v3/ratelimit.proto

jmarantz · 2024-05-29T12:48:08Z

Can we have Tianyu take a pass and then I will send to a maintainer?

wbpcode · 2024-05-30T01:49:20Z

Seems @markdroth is busy recently, re-assign this to @adisuissa for API review.

adisuissa

Interesting idea, thanks!
Left high-level comments/questions.

api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto

Signed-off-by: wbpcode <[email protected]>

…s-instance-local-limit

…ross-instance-local-limit

wbpcode · 2024-06-03T13:07:14Z

/retest

alyssawilk · 2024-06-10T12:32:48Z

@adisuissa looks like this is ready for review - the main merge is just for review notes

adisuissa

Thanks!
I've left a few high-level comments.

One other idea: will it be possible to redesign this so that each instance owns a reference to the endpointStats() object or the membership_total object of the static local cluster, and when tokensPerFill is called, it will fetch the current number of endpoints?
I think this will still need to by guarded (mutex), if the membership_total is not atomic. However, it will remove the additional share-monitor, and the member-update callback (may simplify the solution).

source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc

…ross-instance-local-limit

wbpcode · 2024-06-12T02:13:23Z

One other idea: will it be possible to redesign this so that each instance owns a reference to the endpointStats() object or the membership_total object of the static local cluster, and when tokensPerFill is called, it will fetch the current number of endpoints?

It's my initial implementation when doing the POC. It's actually simpler but means it's hard to use different algorithms to calculate the ratio. For example, we may want take the weight into the account in the future. Current design provides a well defined interface and abstraction for more complex share calculating.

source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc

Signed-off-by: wbpcode <[email protected]>

adisuissa

Thanks, overall LGTM.
Left some minor comments.

source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc

adisuissa · 2024-06-13T12:46:25Z

source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h

@@ -84,6 +124,9 @@ class LocalRateLimiterImpl {
 TokenState tokens_;
 absl::flat_hash_set<LocalDescriptorImpl, LocalDescriptorHash, LocalDescriptorEqual> descriptors_;
 std::vector<LocalDescriptorImpl> sorted_descriptors_;
+
+ ShareProviderSharedPtr share_provider_;


OOC does this really need to be a shared-ptr and not just a reference. AFAIK the singleton should outlive the filter, but I may be mistaken.

Yeah, the singleton has longer lifetime than the filter. But note the getShareProvider() not guarantee that the
singleton will keep a copy of returned shared pointer.

For example, if we support new algorithms in the future, then the singleton will only keep a map from the proto to the weak pointer of share provider.

So, I think a shared pointer here would be better.

api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto

api/envoy/extensions/common/ratelimit/v3/ratelimit.proto

Co-authored-by: Adi (Suissa) Peleg <[email protected]> Signed-off-by: code <[email protected]>

…e_limit.proto Co-authored-by: Adi (Suissa) Peleg <[email protected]> Signed-off-by: code <[email protected]>

Signed-off-by: wbpcode <[email protected]>

adisuissa

/lgtm api

adisuissa · 2024-06-14T12:32:55Z

test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc

@@ -315,5 +431,44 @@ TEST_P(LocalRateLimitFilterIntegrationTest, BasicTestPerRouteAndRds) {
 cleanUpXdsConnection();
 }

+#ifdef NDEBUG


Sorry for the late observation, but IMHO this is a red-flag when it comes to tests (especially integration tests). Which assertion in the singleton manager is triggered?

Specifically I would like to see a validation of the MAIN_THREAD assertions that were added being exercised in an integration test.

The get method of singleton manager. The singleton manager could only be accessed in the thread where it's created. (the server main thread for integration test).

But these tests will access singleton manger in the test thread which is not allowed and will trigger the assertion.

I have removed this macro because I created another PR to resolve this problem. See #34766

wbpcode · 2024-06-14T17:46:22Z

/retest

…ross-instance-local-limit

Signed-off-by: wbpcode <[email protected]>

adisuissa

LGTM, thanks!
/lgtm api

@tyxia can you PTAL?

wbpcode added 3 commits May 21, 2024 10:21

local rate limit: add cross local cluster rate limit support

5722326

Signed-off-by: wbpcode <[email protected]>

change log

c31d469

Signed-off-by: wbpcode <[email protected]>

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-c…

e4ab336

…ross-instance-local-limit

wbpcode requested a review from mattklein123 as a code owner May 21, 2024 12:27

repokitteh-read-only bot added the api label May 21, 2024

repokitteh-read-only bot assigned markdroth May 21, 2024

fix typo

45be840

Signed-off-by: wbpcode <[email protected]>

juanmolle reviewed May 21, 2024

View reviewed changes

api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto Outdated Show resolved Hide resolved

add integration tests

f8058b9

Signed-off-by: wbpcode <[email protected]>

ramaraochavali reviewed May 23, 2024

View reviewed changes

api/envoy/extensions/common/ratelimit/v3/ratelimit.proto Outdated Show resolved Hide resolved

api/envoy/extensions/common/ratelimit/v3/ratelimit.proto Show resolved Hide resolved

jmarantz assigned tyxia May 29, 2024

wbpcode assigned adisuissa and unassigned markdroth May 30, 2024

adisuissa reviewed May 31, 2024

View reviewed changes

api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto Show resolved Hide resolved

wbpcode added 3 commits June 3, 2024 09:27

fix test

7d98b4f

Signed-off-by: wbpcode <[email protected]>

Merge branch 'main' of https://github.com/wbpcode/envoy into dev-cros…

c42a8cc

…s-instance-local-limit

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-c…

e732b24

…ross-instance-local-limit

adisuissa reviewed Jun 11, 2024

View reviewed changes

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-c…

8ae7df3

…ross-instance-local-limit

adisuissa reviewed Jun 12, 2024

View reviewed changes

source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc Show resolved Hide resolved

source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc Show resolved Hide resolved

main thread assert

3325c55

Signed-off-by: wbpcode <[email protected]>

adisuissa reviewed Jun 13, 2024

View reviewed changes

wbpcode and others added 2 commits June 14, 2024 09:41

Update api/envoy/extensions/common/ratelimit/v3/ratelimit.proto

ba0cb7c

Co-authored-by: Adi (Suissa) Peleg <[email protected]> Signed-off-by: code <[email protected]>

Update api/envoy/extensions/filters/http/local_ratelimit/v3/local_rat…

03b0c93

…e_limit.proto Co-authored-by: Adi (Suissa) Peleg <[email protected]> Signed-off-by: code <[email protected]>

address comments

016c0c8

Signed-off-by: wbpcode <[email protected]>

adisuissa reviewed Jun 14, 2024

View reviewed changes

repokitteh-read-only bot removed the api label Jun 14, 2024

wbpcode added 3 commits June 18, 2024 07:03

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-c…

0945195

…ross-instance-local-limit

remove macro after envoyproxy#34766

abc5443

Signed-off-by: wbpcode <[email protected]>

resolve confliction after merge main

e9178f4

Signed-off-by: wbpcode <[email protected]>

adisuissa approved these changes Jun 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

local rate limit: add cross local cluster rate limit support #34276

local rate limit: add cross local cluster rate limit support #34276

wbpcode commented May 21, 2024 •

edited

repokitteh-read-only bot commented May 21, 2024

jmarantz commented May 29, 2024

wbpcode commented May 30, 2024

adisuissa left a comment

wbpcode commented Jun 3, 2024

alyssawilk commented Jun 10, 2024

adisuissa left a comment

wbpcode commented Jun 12, 2024

adisuissa left a comment

adisuissa Jun 13, 2024

wbpcode Jun 14, 2024 •

edited

adisuissa left a comment

adisuissa Jun 14, 2024

wbpcode Jun 14, 2024 •

edited

wbpcode Jun 18, 2024

wbpcode commented Jun 14, 2024

adisuissa left a comment

local rate limit: add cross local cluster rate limit support #34276

Are you sure you want to change the base?

local rate limit: add cross local cluster rate limit support #34276

Conversation

wbpcode commented May 21, 2024 • edited

repokitteh-read-only bot commented May 21, 2024

jmarantz commented May 29, 2024

wbpcode commented May 30, 2024

adisuissa left a comment

Choose a reason for hiding this comment

wbpcode commented Jun 3, 2024

alyssawilk commented Jun 10, 2024

adisuissa left a comment

Choose a reason for hiding this comment

wbpcode commented Jun 12, 2024

adisuissa left a comment

Choose a reason for hiding this comment

adisuissa Jun 13, 2024

Choose a reason for hiding this comment

wbpcode Jun 14, 2024 • edited

Choose a reason for hiding this comment

adisuissa left a comment

Choose a reason for hiding this comment

adisuissa Jun 14, 2024

Choose a reason for hiding this comment

wbpcode Jun 14, 2024 • edited

Choose a reason for hiding this comment

wbpcode Jun 18, 2024

Choose a reason for hiding this comment

wbpcode commented Jun 14, 2024

adisuissa left a comment

Choose a reason for hiding this comment

wbpcode commented May 21, 2024 •

edited

wbpcode Jun 14, 2024 •

edited

wbpcode Jun 14, 2024 •

edited