Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

local rate limiter: backport the non-timer based bucket #357

Merged
merged 50 commits into from
Aug 16, 2024

Conversation

nfuden
Copy link
Collaborator

@nfuden nfuden commented Jul 23, 2024

backport the first 3 rate limit pieces of upstream's 34774 issue

This allows us to run local rate limiting not on the main thread if an environment variable is set to true.

In the future can be disabled via envoy_reloadable_features_no_timer_based_rate_limit_token_bucket
for now can be ENABLED via envoy_reloadable_features_no_timer_based_rate_limit_token_bucket

@solo-changelog-bot
Copy link

Issues linked to changelog:
solo-io/gloo#9564

@ashishb-solo
Copy link
Contributor

ashishb-solo commented Jul 26, 2024

I'm not sure exactly how to review/test this so any guidance you have would be appreciated. So far, envoy-static seems to compile successfully. I'm also looking at the tests that were modified in the patch. It looks like here's a list of test files that were updated:

test/common/common/BUILD
test/common/common/token_bucket_impl_test.cc
test/extensions/filters/common/local_ratelimit/BUILD
test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc
test/extensions/filters/http/local_ratelimit/BUILD
test/extensions/filters/http/local_ratelimit/config_test.cc
test/extensions/filters/http/local_ratelimit/filter_test.cc
test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc
test/extensions/filters/listener/local_ratelimit/local_ratelimit_test.cc
test/extensions/filters/network/local_ratelimit/local_ratelimit_integration_test.cc
test/extensions/filters/network/local_ratelimit/local_ratelimit_test.cc

So we can test this by running

bazel test \
@envoy//test/common/common:token_bucket_impl_test \
@envoy//test/extensions/filters/common/local_ratelimit/... \
@envoy//test/extensions/filters/http/local_ratelimit/... \
@envoy//test/extensions/filters/listener/local_ratelimit/... \
@envoy//test/extensions/filters/network/local_ratelimit/...

I'm in the process of running this locally. If it's possible to add this to CI, that would be amazing, but if it's too much of a lift, then don't worry about it.

I'm also wondering if we want to do some sort of performance test before merging this into Gloo and/or releasing it. Do you have any thoughts here?

@nfuden
Copy link
Collaborator Author

nfuden commented Jul 26, 2024

solo-io/gloo#9822 shows that basic local rate limit works with this change

Copy link
Contributor

@ashishb-solo ashishb-solo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good to me. I don't see any major issues and found a couple tiny nits.

My biggest question at the moment is what the role of lbucket.iff is. Is this something that we need in this patch/branch? Was it added by mistake?

For future reference, it looks like the majority of the complexity in the diffs here lies in these two files:

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

(and their associated headers too, of course).

Copy link
Contributor

@ashishb-solo ashishb-solo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a few more comments surrounding testing

@nfuden
Copy link
Collaborator Author

nfuden commented Aug 3, 2024

/kick aml-cpp

@nfuden nfuden changed the base branch from main to release-v1.30 August 15, 2024 12:53
nfuden and others added 10 commits August 15, 2024 09:41
* Backport deallocate slots on worker threads

* Add changelog

* Up-port another change from the 1.27 patch

* Comment upstream source of the patch

* Whoops. commented the wrong line
ci/cloudbuild.yaml Show resolved Hide resolved
ci/cloudbuild.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@ashishb-solo ashishb-solo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great! i really like having additional actions like this to test the veracity of the upstream patches that we backport

@nfuden
Copy link
Collaborator Author

nfuden commented Aug 15, 2024

/kick 92f51882

@nfuden nfuden merged commit a8b973d into release-v1.30 Aug 16, 2024
4 checks passed
@nfuden nfuden deleted the back/local-rate-limit-1.30 branch August 16, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants