-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Rate limit replica creation #7869
Conversation
@@ -64,6 +64,10 @@ const ( | |||
// 3000 pods. Just creation is limited to 30qps, and watching happens with | |||
// ~10-30s latency/pod at scale. | |||
ExpectationsTimeout = 6 * time.Minute |
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.
Can we reduce this timeout now?
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, was going to send a follow up pr, and observe latencies in production for a bit. That timeout isn't that critical unless we drop events, would you prefer I did so in this one?
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, why not fix it now, since the TODO will be truthful-but-misleading after this merges.
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.
done
I took a quick look and it generally looks good to me. |
Wanted to run density, then ran into a situtation I've been trying to repro for a while and decided to dig into that before destroying the cluster. Density is now done though, PTAL. |
bf4942d
to
f4b9f31
Compare
Removed Min function |
controllerSpec.Spec.Replicas = numReplicas | ||
manager.controllerStore.Store.Add(controllerSpec) | ||
|
||
for i := 0.0; i < scaleFactor; i++ { |
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.
suggest don't use floating point; i := 0; i < numReplicas; i += burstReplicas
?
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.
Done, I guess it's clearer this way. Curious if you have another reason for this suggestion?
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.
Allergic to using comparison operations on floating point numbers :)
(also found the "scale factor" variable name confusing)
LGTM |
Travis failed with the following error:
@bprashanth : Can you take a look? |
@nikhiljindal I think that's #6878, I kicked travis, we'll see |
Its green now. Merging. |
Rate limit replica creation
Suspend rcs at BurstReplicas until they observe all watch events, so they don't stress out the apiserver. @lavalamp @wojtek-t