Support specifying custom LB retry period from cloud provider #94021

timoreimann · 2020-08-15T08:09:41Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
This change allows cloud providers to specify a custom retry period by returning a RetryError. The purpose is to bypass the work queue-driven exponential backoff algorithm when there is no need to back off.

Specifically, this can be the case when a cloud load balancer operation such as a create or delete is still pending and the cloud API should be polled for completion at a constant interval. A backoff algorithm would not always be reasonable to apply here since there is no API or performance degradation warranting an increasing wait time between API requests.

Which issue(s) this PR fixes:
Fixes #88902

Special notes for your reviewer:
For now, the PR is meant to provide a starting point for discussing. Hence, I have not invested into adding tests. Once/if we have consent on the general direction of this PR, I am going to complete the missing pieces. (Tests have been added.)

Does this PR introduce a user-facing change?:

Support specifying a custom retry period for cloud load-balancer operations

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
Not sure if needed. None so far.

/sig cloud-provider
/cc @andrewsykim

k8s-ci-robot · 2020-08-15T08:09:49Z

Hi @timoreimann. 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 /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.

staging/src/k8s.io/cloud-provider/controllers/service/controller.go

andrewsykim

cc @MrHohn @bowei @cheftako

staging/src/k8s.io/cloud-provider/controllers/service/controller.go

andrewsykim

/ok-to-test

timoreimann · 2020-10-28T17:42:55Z

@andrewsykim @MrHohn does this proposal warrant a KEP, or would it suffice to move forward with the PR? In the latter case, I'd like to invest additional time to update the tests and bring the code into a mergeable state.

andrewsykim · 2020-11-11T01:01:14Z

@andrewsykim @MrHohn does this proposal warrant a KEP, or would it suffice to move forward with the PR? In the latter case, I'd like to invest additional time to update the tests and bring the code into a mergeable state.

I don't think we need a KEP for this, let's get the tests added and try to merge this for v1.20

fejta-bot · 2021-02-09T01:21:17Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

timoreimann · 2021-02-09T06:13:06Z

/remove-lifecycle stale

fejta-bot · 2021-05-10T06:15:57Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

timoreimann · 2021-05-10T07:39:53Z

/remove-lifecycle stale

k8s-triage-robot · 2021-08-08T08:30:21Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

This change allows cloud providers to specify a custom retry period by returning a RetryError. The purpose is to bypass the work queue-driven exponential backoff algorithm when there is no need to back off. Specifically, this can be the case when a cloud load balancer operation such as a create or delete is still pending and the cloud API should be polled for completion at a constant interval. A backoff algorithm would not always be reasonable to apply here since there is no API or performance degradation warranting an increasing wait time between API requests.

timoreimann · 2023-05-01T18:56:07Z

/retest

aojea · 2023-05-01T20:39:56Z

staging/src/k8s.io/cloud-provider/api/retry_error.go

+// fixed duration (as opposed to backing off exponentially).
+type RetryError struct {
+ msg string
+ retryAfter time.Duration


what should be the interpretation of 0?

There'd be no special interpretation. Instead, the retry would be immediate (see also where the value is used).

The need to retry right away may be uncommon or even rare, but I personally wouldn't want to disallow it. Maybe a user's network is very slow, or there are already some natural / drive-by delays that don't warrant another extra wait at the client side?

I think a zero delay can be legitimate, but let me know if you think differently.

I think a zero delay can be legitimate, but let me know if you think differently.

I think is ok, I just wanted to double check we all have the same interpretation

aojea · 2023-05-01T20:46:55Z

staging/src/k8s.io/cloud-provider/controllers/service/controller_test.go

@@ -2281,3 +2362,66 @@ func (l *fakeNodeLister) Get(name string) (*v1.Node, error) {
 }
 return nil, nil
 }
+
+type fakeServiceLister struct {


you don't need to create these mocks, you can use a cache

serviceCache := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) serviceLister := v1listers.NewServiceLister(serviceCache) for i := range test.services { if err := serviceCache.Add(test.services[i]); err != nil { t.Fatalf("%s unexpected service add error: %v", test.name, err) } }

That's much better -- adjusted, thanks!

timoreimann · 2023-05-02T05:58:42Z

@aojea PTAL.

timoreimann · 2023-05-02T06:00:21Z

(Updated the copyright year to 2023 real quick)

k8s-ci-robot · 2023-05-02T06:59:34Z

@timoreimann: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-bazel-build	c8aa7ecd98a42b4d5d6b89b182f5c89a04ec3542	link	`/test pull-kubernetes-bazel-build`
pull-kubernetes-bazel-test	c8aa7ecd98a42b4d5d6b89b182f5c89a04ec3542	link	`/test pull-kubernetes-bazel-test`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

timoreimann · 2023-05-02T07:03:51Z

/retest

aojea · 2023-05-02T07:17:26Z

/lgtm
/approve

Thanks

k8s-ci-robot · 2023-05-02T07:17:32Z

LGTM label has been added.

Git tree hash: c97695cb43c46f39dd3542af7bbf6e9f964c05d9

k8s-ci-robot · 2023-05-02T07:17:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, aojea, timoreimann

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:

~~staging/src/k8s.io/cloud-provider/OWNERS~~ [andrewsykim]

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 requested a review from andrewsykim August 15, 2020 08:09

k8s-ci-robot added area/cloudprovider sig/network Categorizes an issue or PR as relevant to SIG Network. labels Aug 15, 2020

andrewsykim reviewed Sep 25, 2020

View reviewed changes

staging/src/k8s.io/cloud-provider/controllers/service/controller.go Show resolved Hide resolved

timoreimann force-pushed the support-specifying-custom-lb-retry-period-from-cloud-provider branch from e802d2d to adceb65 Compare September 25, 2020 19:01

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 25, 2020

andrewsykim reviewed Sep 25, 2020

View reviewed changes

staging/src/k8s.io/cloud-provider/controllers/service/controller.go Outdated Show resolved Hide resolved

staging/src/k8s.io/cloud-provider/controllers/service/controller.go Outdated Show resolved Hide resolved

andrewsykim reviewed Sep 29, 2020

View reviewed changes

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 Sep 29, 2020

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 9, 2021

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 9, 2021

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 10, 2021

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 10, 2021

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 24, 2023

timoreimann added 8 commits April 30, 2023 22:37

Alias api/errors instead of stdlib errors

ea4ce5d

Add tests

fc8b465

Improve RetryError GoDocs

18e978a

Improve error logging in processNextServiceItem

73bc986

Document RetryError properly

a15013e

Localize SpyWorkQueue to cloud-provider for now

208af2d

Move test double queue to test file and unexport

0fcf42f

timoreimann force-pushed the support-specifying-custom-lb-retry-period-from-cloud-provider branch from 3aec5f5 to 0fcf42f Compare May 1, 2023 18:19

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 1, 2023

aojea reviewed May 1, 2023

View reviewed changes

timoreimann added 2 commits May 2, 2023 07:59

Remove custom fake svc lister in favor of regular indexer

b464d08

timoreimann force-pushed the support-specifying-custom-lb-retry-period-from-cloud-provider branch from b0ebdc0 to 2ad2c15 Compare May 2, 2023 06:00

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 2, 2023

k8s-ci-robot merged commit f51dad5 into kubernetes:master May 2, 2023
12 checks passed

k8s-ci-robot added this to the v1.28 milestone May 2, 2023

timoreimann deleted the support-specifying-custom-lb-retry-period-from-cloud-provider branch May 2, 2023 09:16

llDrLove mentioned this pull request Aug 21, 2023

Specify retry duration when DO LB is in a being created ("new" state) digitalocean/digitalocean-cloud-controller-manager#647

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support specifying custom LB retry period from cloud provider #94021

Support specifying custom LB retry period from cloud provider #94021

timoreimann commented Aug 15, 2020 •

edited

k8s-ci-robot commented Aug 15, 2020

andrewsykim left a comment

andrewsykim left a comment

timoreimann commented Oct 28, 2020

andrewsykim commented Nov 11, 2020

fejta-bot commented Feb 9, 2021

timoreimann commented Feb 9, 2021

fejta-bot commented May 10, 2021

timoreimann commented May 10, 2021

k8s-triage-robot commented Aug 8, 2021

timoreimann commented May 1, 2023

aojea May 1, 2023

timoreimann May 2, 2023

aojea May 2, 2023

aojea May 1, 2023

timoreimann May 2, 2023

timoreimann commented May 2, 2023

timoreimann commented May 2, 2023

k8s-ci-robot commented May 2, 2023 •

edited

timoreimann commented May 2, 2023

aojea commented May 2, 2023

k8s-ci-robot commented May 2, 2023

k8s-ci-robot commented May 2, 2023

Support specifying custom LB retry period from cloud provider #94021

Support specifying custom LB retry period from cloud provider #94021

Conversation

timoreimann commented Aug 15, 2020 • edited

k8s-ci-robot commented Aug 15, 2020

andrewsykim left a comment

Choose a reason for hiding this comment

andrewsykim left a comment

Choose a reason for hiding this comment

timoreimann commented Oct 28, 2020

andrewsykim commented Nov 11, 2020

fejta-bot commented Feb 9, 2021

timoreimann commented Feb 9, 2021

fejta-bot commented May 10, 2021

timoreimann commented May 10, 2021

k8s-triage-robot commented Aug 8, 2021

timoreimann commented May 1, 2023

aojea May 1, 2023

Choose a reason for hiding this comment

timoreimann May 2, 2023

Choose a reason for hiding this comment

aojea May 2, 2023

Choose a reason for hiding this comment

aojea May 1, 2023

Choose a reason for hiding this comment

timoreimann May 2, 2023

Choose a reason for hiding this comment

timoreimann commented May 2, 2023

timoreimann commented May 2, 2023

k8s-ci-robot commented May 2, 2023 • edited

timoreimann commented May 2, 2023

aojea commented May 2, 2023

k8s-ci-robot commented May 2, 2023

k8s-ci-robot commented May 2, 2023

timoreimann commented Aug 15, 2020 •

edited

k8s-ci-robot commented May 2, 2023 •

edited