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

✨ adjust loadbalancer wait.Backoff #853

Conversation

chrischdi
Copy link
Member

What this PR does / why we need it:

Adjusts the wait.Backoff definition which is used in waitForLoadBalancerActive and waitForListener.
With the current implementation the controller waits 30s also for tasks which are pretty fast like adding a load balancer member.

The old values did result in:

  • executing the wait function up to 10 times
  • having a maximum of 9 sleeps with 30s each
  • in worst case sleep for 9*30s = 270s

The new values result in a exponential backoff:

  • executing the wait function up to 20 times
  • having a maximum of 19 sleeps, which vary between 1s to a maximum of ~55s
  • in worst case sleep for ~273s
    1 sleep=1.000000 sum=1.000000 2 sleep=1.250000 sum=2.250000 3 sleep=1.562500 sum=3.812500 4 sleep=1.953125 sum=5.765625 5 sleep=2.441406 sum=8.207031 6 sleep=3.051758 sum=11.258789 7 sleep=3.814697 sum=15.073486 8 sleep=4.768372 sum=19.841858 9 sleep=5.960464 sum=25.802322 10 sleep=7.450581 sum=33.252903 11 sleep=9.313226 sum=42.566129 12 sleep=11.641532 sum=54.207661 13 sleep=14.551915 sum=68.759576 14 sleep=18.189894 sum=86.949470 15 sleep=22.737368 sum=109.686838 16 sleep=28.421709 sum=138.108547 17 sleep=35.527137 sum=173.635684 18 sleep=44.408921 sum=218.044605 19 sleep=55.511151 sum=273.555756

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

  1. Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

  • squashed commits
  • if necessary:
    • includes documentation
    • adds unit tests

/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 30, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @chrischdi. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 30, 2021
@chrischdi chrischdi changed the title ✨ adjust loadbalancer wait.Backoff ✨ adjust loadbalancer wait.Backoff Apr 30, 2021
@chrischdi chrischdi changed the title ✨ adjust loadbalancer wait.Backoff ✨ adjust loadbalancer wait.Backoff Apr 30, 2021
@chrischdi chrischdi changed the title ✨ adjust loadbalancer wait.Backoff ✨ adjust loadbalancer wait.Backoff Apr 30, 2021
@chrischdi chrischdi changed the title ✨ adjust loadbalancer wait.Backoff ✨ adjust loadbalancer wait.Backoff Apr 30, 2021
@chrischdi chrischdi changed the title ✨ adjust loadbalancer wait.Backoff ✨ adjust loadbalancer wait.Backoff Apr 30, 2021
@chrischdi chrischdi changed the title ✨ adjust loadbalancer wait.Backoff ✨ adjust loadbalancer wait.Backoff Apr 30, 2021
@chrischdi chrischdi changed the title ✨ adjust loadbalancer wait.Backoff ✨ adjust loadbalancer wait.Backoff Apr 30, 2021
@chrischdi
Copy link
Member Author

I don't get the sparkles part, sorry :D

@sbueringer
Copy link
Member

sbueringer commented Apr 30, 2021

/retitle ✨ adjust loadbalancer wait.Backoff

It's not that hard :)

P.S. just learned yesterday that both versions are okay for the release note generation

@k8s-ci-robot k8s-ci-robot changed the title ✨ adjust loadbalancer wait.Backoff ✨ adjust loadbalancer wait.Backoff Apr 30, 2021
@sbueringer
Copy link
Member

/approve

Very nice fix, should bring down the cluster reconciliation time quite a bit. (we had min. 30s wait per lb resources, which explains why it took so long)

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrischdi, sbueringer

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 30, 2021
@sbueringer
Copy link
Member

/retest
(looks like Prow didn't trigger the jobs properly)

@chrischdi
Copy link
Member Author

@sbueringer : needs ok-to-test ;-)

@sbueringer
Copy link
Member

/ok-to-test

You should apply to org member ship :)

You have to check if the prereqs are fullfilled: #853 (comment)

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 30, 2021
@chrischdi
Copy link
Member Author

/ok-to-test

You should apply to org member ship :)

You have to check if the prereqs are fullfilled: #853 (comment)

I assume you would sponsor me ;-) I'd still need a second sponsor to fulfill the requirements

@sbueringer
Copy link
Member

/ok-to-test
You should apply to org member ship :)
You have to check if the prereqs are fullfilled: #853 (comment)

I assume you would sponsor me ;-) I'd still need a second sponsor to fulfill the requirements

Yup exactly. I think make a few more PRs/Issues here or in cloud-provider-openstack and it should be fine according to the guidelines. Either @jichenjc or someone from cloud-provider-openstack will probably sponsor you. (WDYT @jichenjc ?)

@jichenjc
Copy link
Contributor

jichenjc commented May 4, 2021

/ok-to-test
You should apply to org member ship :)
You have to check if the prereqs are fullfilled: #853 (comment)

I assume you would sponsor me ;-) I'd still need a second sponsor to fulfill the requirements

Yup exactly. I think make a few more PRs/Issues here or in cloud-provider-openstack and it should be fine according to the guidelines. Either @jichenjc or someone from cloud-provider-openstack will probably sponsor you. (WDYT @jichenjc ?)

yes, I will do , my pleasure :)

@jichenjc
Copy link
Contributor

jichenjc commented May 4, 2021

/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 4, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 4, 2021
@k8s-ci-robot k8s-ci-robot merged commit d520355 into kubernetes-sigs:master May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants