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
Add rate limiter and update tests. #10987
Conversation
a5ad4d9
to
62b195c
Compare
62b195c
to
6b63be1
Compare
I need to update this to support disabling the ratelimiting. |
LGTM. Works well, tested the rate limiting on microk8s with ~50 agents. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this overall, I would like to see an experiment with 1000+ agents so we are comfortable with the behavior with and without this change.
@@ -29,29 +29,31 @@ func (s *ControllerSuite) TestControllerAndModelConfigInitialisation(c *gc.C) { | |||
c.Assert(err, jc.ErrorIsNil) | |||
|
|||
optional := set.NewStrings( | |||
controller.IdentityURL, | |||
controller.IdentityPublicKey, | |||
controller.AgentRateLimitMax, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is just Sort() the list alphabetically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I hit my OCD limit
srv.updateAgentRateLimiter(data.Config) | ||
}) | ||
if err != nil { | ||
logger.Criticalf("programming error in subscribe function: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Criticalf in this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is critical by design.
@@ -534,6 +534,8 @@ func (s *JujuConnSuite) setUpConn(c *gc.C) { | |||
for key, value := range s.ControllerConfigAttrs { | |||
s.ControllerConfig[key] = value | |||
} | |||
// Explicitly disable rate limiting. | |||
s.ControllerConfig[controller.AgentRateLimitMax] = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
|
|
1 similar comment
|
#11001 Merge 2.7 forward for: - #10996 from wallyworld/lookup-action-prefix - #10995 from anastasiamac/missing-cons-lp1854510 - #10968 from anastasiamac/add-cred-UR-force - #10987 from howbazaar/2.7-apiserver-agent-ratelimiting - #10992 from howbazaar/2.7-test-races - #10933 from jameinel/2.7-delay-done - #10966 from jameinel/2.7-handle-start-error-1854338
Add configurable rate limiting for agent connections.
This work took a sideways step as I needed to write tests for the rate limiting, but all the login tests were using JujuConnSuite. So I changed the login tests to instead be based on statetetesting.StateSuite, which is much lighter.
QA steps
Documentation changes
Will need to document the two new controller config values.