-
Notifications
You must be signed in to change notification settings - Fork 39.3k
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
lease manager limit max objects attached to a lease #98257
lease manager limit max objects attached to a lease #98257
Conversation
Hi @lingsamuel. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
5541099
to
5bbb303
Compare
Thanks for the PR! #97480 (comment) I meant that we may want to play with '--lease-reuse-duration-seconds' value, before we introduce, but after thinking about that, with this PR we will solve it more precisely (after this PR we will create new lease either after 1m from the previous lease or 10k objects, which should spread. e.g. first lease from that comment into 6-7 smaller leases spread evenly in the first minute). The value 10k looks reasonable to me (this is ~what we are seeing during the test when there are no performance problems). /lgtm |
5bbb303
to
095e4ed
Compare
@mborsz I fixed a wrong test, need a new lgtm label |
/assign @lavalamp |
I would really prefer not to add another flag. Can we experiment and find a good value and then hard code it? |
/lgtm This seems reasonable to me now. Thanks! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lavalamp, lingsamuel, ptabor The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest Review the full test history for this PR. Silence the bot with an |
LGTM - I guess we can expose an option to configure it if needed later. |
Thanks for this change! It reduced CPU usage of events etcd from ~10-15 cores to 1-2 cores: This is really impressive! |
wow - this is amazing. I expected improvements, but definitely not that huge... |
Really nice. Do you have a chart that shows tail latency of e.g. api-server requests ? |
Thank you. There is hope it will mitigate the huge unpredictive spikes. |
Signed-off-by: Ling Samuel lingsamuelgrace@gmail.com
What type of PR is this?
/kind feature
What this PR does / why we need it:
Details: #96836
Which issue(s) this PR fixes:
Part of #96836
Special notes for your reviewer:
Default value set to 10k per #97480 (comment)
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: