✨ Take context when getting informer. #663

djzager · 2019-10-28T19:26:02Z

Follow up to #580
Fixes #562

If the context's deadline is exceeded waiting for the cache to sync, then we should return an appropriate error.

This intentionally punts on what a sensible default should be for this timeout based on this comment

Let's punt on the default for now (do it in a separate PR) so we can at least get the capability in and unbreak people that want to use this.

What this PR needs

Testing. Currently pursuing faking out the HasSynced method.

k8s-ci-robot · 2019-10-28T19:26:11Z

Hi @djzager. 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 · 2019-10-28T19:26:11Z

Welcome @djzager!

It looks like this is your first PR to kubernetes-sigs/controller-runtime 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-runtime has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

djzager · 2019-10-28T19:30:27Z

/assign @DirectXMan12

DirectXMan12 · 2019-11-05T23:06:51Z

pkg/cache/internal/informers_map.go

@@ -162,9 +164,22 @@ func (ip *specificInformersMap) Get(gvk schema.GroupVersionKind, obj runtime.Obj
 	}

 	if started && !i.Informer.HasSynced() {
+		syncReturn := make(chan bool)


make this buffered so that we don't leak the goroutine when we time out

Suggested change

syncReturn := make(chan bool)

syncReturn := make(chan bool, 1 /* don't leak goroutines on timeout */)

DirectXMan12 · 2019-11-05T23:07:06Z

pkg/cache/internal/informers_map.go

+		select {
+		case <-ctx.Done():
+			//end the polling for cache to sync
+			done <- struct{}{}


Suggested change

done <- struct{}{}

close(done)

pkg/cache/internal/informers_map.go

🏃 build k8s 1.13.5 bundle

DirectXMan12 · 2020-02-05T22:24:46Z

hey, are you still interested in working on this?

djzager · 2020-02-06T15:25:45Z

hey, are you still interested in working on this?

Yes I am. Apologies for letting this fall off my radar. Will take today to catch up on the comments and start adding the test.

vincepri · 2020-02-11T15:36:38Z

/ok-to-test
@djzager Can you squash commits down to 1?

djzager · 2020-02-11T18:35:16Z

/retest

vincepri

/lgtm
/assign @DirectXMan12

pkg/cache/cache_test.go

alvaroaleman

/lgtm

vincepri · 2020-02-12T16:03:26Z

/assign @DirectXMan12 @gerred

DirectXMan12 · 2020-02-26T01:13:01Z

pkg/cache/cache_test.go

+
+					By("verifying that an error is returned")
+					Expect(err).To(HaveOccurred())
+					Expect(err.Error()).To(Equal(fmt.Sprintf("Timeout: failed waiting for %T Informer to sync", &kcorev1.Pod{})))


In case this comes up in posterity: error messages are generally not part of our api guarantees.

In cases like this we should be checking errors.Is(err, context.DeadlineExceeded), which is a more type-checkable thing that we actually can guarantee.

I generally think of tests as doubling as the human-readable part of the API contract, and so it's good to stick as close to what we guarantee as possible

DirectXMan12 · 2020-02-26T01:13:41Z

pkg/cache/informer_cache.go

@@ -120,7 +120,8 @@ func (ip *informerCache) GetInformerForKind(gvk schema.GroupVersionKind) (Inform
 	if err != nil {
 		return nil, err
 	}
-	_, i, err := ip.InformersMap.Get(gvk, obj)
+
+	_, i, err := ip.InformersMap.Get(context.TODO(), gvk, obj)


can you add a TODO here explaining what we'd need to do to plumb this through?

My best attempt at an explanation. WDYT?

sure, looks good. Do you mind putting together a follow-up to do that?

Passing the context down to the `ip.InformersMap.Get()` makes it possible to handle scenarios where the cache takes too long to sync or will never sync.

DirectXMan12

/approve

DirectXMan12 · 2020-02-26T22:02:06Z

pkg/cache/informer_cache.go

@@ -120,7 +120,8 @@ func (ip *informerCache) GetInformerForKind(gvk schema.GroupVersionKind) (Inform
 	if err != nil {
 		return nil, err
 	}
-	_, i, err := ip.InformersMap.Get(gvk, obj)
+
+	_, i, err := ip.InformersMap.Get(context.TODO(), gvk, obj)


sure, looks good. Do you mind putting together a follow-up to do that?

DirectXMan12 · 2020-02-26T22:02:37Z

/lgtm

k8s-ci-robot · 2020-02-26T22:02:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, djzager

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:

~~OWNERS~~ [DirectXMan12]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

**Description** Add timeout in the watch feature for Ansible based-operators proxy to avoid appears that the reconcile is stuck and hang when the operator has not the correct permissions to List and Watch the resources. **Motivation for the change:** - #1638 - https://bugzilla.redhat.com/show_bug.cgi?id=1701041 **Note** Also, solved by kubernetes-sigs/controller-runtime#663.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 28, 2019

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 28, 2019

k8s-ci-robot requested review from droot and pwittrock October 28, 2019 19:26

djzager force-pushed the cache-timeout branch from a188379 to bd510e1 Compare October 28, 2019 19:28

k8s-ci-robot assigned DirectXMan12 Oct 28, 2019

DirectXMan12 suggested changes Nov 5, 2019

View reviewed changes

joelanford reviewed Nov 22, 2019

View reviewed changes

pkg/cache/internal/informers_map.go Outdated Show resolved Hide resolved

joelanford mentioned this pull request Dec 3, 2019

Adding context timeout for Ansible proxy operator-framework/operator-sdk#2264

Merged

DirectXMan12 pushed a commit that referenced this pull request Jan 31, 2020

Merge pull request #663 from mengqiy/bumpk8sdeps

a077df6

🏃 build k8s 1.13.5 bundle

djzager force-pushed the cache-timeout branch from 593a2d4 to 22b906a Compare February 7, 2020 02:37

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

djzager force-pushed the cache-timeout branch from bf2f190 to eeb187c Compare February 11, 2020 15:29

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 Feb 11, 2020

djzager changed the title ~~[WIP] ✨ Adding timeout to unset context for waiting for cache to sync.~~ ✨ Adding timeout to unset context for waiting for cache to sync. Feb 11, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 11, 2020

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 Feb 11, 2020

djzager force-pushed the cache-timeout branch 2 times, most recently from 6fa0ae5 to 5c3a188 Compare February 11, 2020 15:44

djzager force-pushed the cache-timeout branch from 5c3a188 to dd1d52a Compare February 11, 2020 18:20

vincepri approved these changes Feb 11, 2020

View reviewed changes

k8s-ci-robot assigned vincepri Feb 11, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2020

alvaroaleman reviewed Feb 12, 2020

View reviewed changes

pkg/cache/cache_test.go Show resolved Hide resolved

djzager force-pushed the cache-timeout branch from dd1d52a to 9887317 Compare February 12, 2020 14:48

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 12, 2020

alvaroaleman approved these changes Feb 12, 2020

View reviewed changes

k8s-ci-robot assigned alvaroaleman Feb 12, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 12, 2020

k8s-ci-robot assigned gerred Feb 12, 2020

vincepri mentioned this pull request Feb 20, 2020

pkg/cache methods should implement deadline semantics for context parameter #41

Closed

vincepri added this to the v0.5.x milestone Feb 21, 2020

DirectXMan12 suggested changes Feb 26, 2020

View reviewed changes

✨ Take context when getting informer

a7df8ba

Passing the context down to the `ip.InformersMap.Get()` makes it possible to handle scenarios where the cache takes too long to sync or will never sync.

djzager force-pushed the cache-timeout branch from 9887317 to a7df8ba Compare February 26, 2020 15:39

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2020

DirectXMan12 approved these changes Feb 26, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 26, 2020

k8s-ci-robot merged commit 741745a into kubernetes-sigs:master Feb 26, 2020

This was referenced Feb 27, 2020

Update Informers interface to accept context on GetInformer(ForKind) #828

Closed

⚠ update Informers interface to accept context #830

Merged

shawn-hurley mentioned this pull request Feb 28, 2020

client.Get will hung when service-account lack of get and list permissions #550

Closed

djzager mentioned this pull request Mar 11, 2020

REQUEST: New membership for @djzager kubernetes/org#1698

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Take context when getting informer. #663

✨ Take context when getting informer. #663

djzager commented Oct 28, 2019 •

edited

Loading

k8s-ci-robot commented Oct 28, 2019

k8s-ci-robot commented Oct 28, 2019

djzager commented Oct 28, 2019

DirectXMan12 Nov 5, 2019

DirectXMan12 Nov 5, 2019

DirectXMan12 commented Feb 5, 2020

djzager commented Feb 6, 2020

vincepri commented Feb 11, 2020

djzager commented Feb 11, 2020

vincepri left a comment

alvaroaleman left a comment

vincepri commented Feb 12, 2020

DirectXMan12 Feb 26, 2020

DirectXMan12 Feb 26, 2020

djzager Feb 26, 2020

DirectXMan12 Feb 26, 2020

DirectXMan12 left a comment

DirectXMan12 Feb 26, 2020

DirectXMan12 commented Feb 26, 2020

k8s-ci-robot commented Feb 26, 2020

	syncReturn := make(chan bool)
	syncReturn := make(chan bool, 1 /* don't leak goroutines on timeout */)

✨ Take context when getting informer. #663

✨ Take context when getting informer. #663

Conversation

djzager commented Oct 28, 2019 • edited Loading

k8s-ci-robot commented Oct 28, 2019

k8s-ci-robot commented Oct 28, 2019

djzager commented Oct 28, 2019

DirectXMan12 Nov 5, 2019

Choose a reason for hiding this comment

DirectXMan12 Nov 5, 2019

Choose a reason for hiding this comment

DirectXMan12 commented Feb 5, 2020

djzager commented Feb 6, 2020

vincepri commented Feb 11, 2020

djzager commented Feb 11, 2020

vincepri left a comment

Choose a reason for hiding this comment

alvaroaleman left a comment

Choose a reason for hiding this comment

vincepri commented Feb 12, 2020

DirectXMan12 Feb 26, 2020

Choose a reason for hiding this comment

DirectXMan12 Feb 26, 2020

Choose a reason for hiding this comment

djzager Feb 26, 2020

Choose a reason for hiding this comment

DirectXMan12 Feb 26, 2020

Choose a reason for hiding this comment

DirectXMan12 left a comment

Choose a reason for hiding this comment

DirectXMan12 Feb 26, 2020

Choose a reason for hiding this comment

DirectXMan12 commented Feb 26, 2020

k8s-ci-robot commented Feb 26, 2020

djzager commented Oct 28, 2019 •

edited

Loading