1.7 backports 2020-10-08 - API rate limiting fixes #13477
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[ upstream commit 208c69c ] So far, the rate limiting has been enforced before enforcing parallel requests. In theory, this is better because we can return error code 429 earlier to tell callers to slow down. In practice, many callers will retry immediately anyway which means that all the limiting will happen by the rate limiter. The rate limiter relies on reservations that need to be canceled. If too many requests happen in parallel, reservations can't be canceled quickly enough. By swapping the enforcement of parallel requests with the rate limiter, all requests will block for at least MaxWaitDuration if more than the allowed number of parallel requests are pending which will naturally pace the callers. Swapping the enforcement order requires the acquired semaphore to be released in error cases of the rate limiter. This requires to change the structure of Wait() to have a single error handling structure. By reusing finishRequest(), the metrics handler has to be adjusted slightly to account for new outcomes as it now bumps the metric for canceled requests as well. What remains unchanged is that only successful API requests are used to calculate the mean processing duration. Fixes: 3141e65 ("rate: Add API rate limiting system") Signed-off-by: Thomas Graf <thomas@cilium.io>
[ upstream commit 75159c6 ] Simulate stress by feeding many parallel requests through the API rate limiter. The maximum wait duration will soon be hit for requests causing them to fail. Without the fix, this test would often fail. If it succeeded, the number of retries is in the range of 60-80K. With the fix, the test succeeds consistently and retries are in the range of 6-8K. That's a 10x reduction of retries. Signed-off-by: Thomas Graf <thomas@cilium.io>
[ upstream commit 269aa1b ] Test that failing requests will not impact the rate limiter. This is already working correctly so this test only adds additional coverage. Signed-off-by: Thomas Graf <thomas@cilium.io>
maintainer-s-little-helper
bot
added
the
kind/backports
|
test-backport-1.7 |
joestringer
approved these changes
Oct 8, 2020
|
If you don't have the section in the PR description like this then the release notes will not be generated correctly: See https://docs.cilium.io/en/latest/contributing/release/backports/#via-github-web-interface for more details. |
christarazi
approved these changes
Oct 8, 2020
|
test-upstream-k8s |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#13450 -- Follow-up fixes for the API rate limiter
Once this PR is merged, you can update the PR labels via: