[WIP] ✨ Adding timeout while waiting for cache to sync #580

shawn-hurley · 2019-09-01T14:13:59Z

fixes #562

Adding a timeout based on the context while waiting for the cache to sync. The default value is 30 seconds as I didn't have a better guess on what it should be.

Discussion:

What should the correct default be?
Should we create a specific error that one could check, I would think we would ultimately want this to return true.
Need help adding a test for this. All I can think to do is change the user that is making the calls to a user that does not have access to a resource. This seemed daunting, is there some other way to trigger the cache sync failure?

k8s-ci-robot · 2019-09-01T14:14:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: shawn-hurley
To complete the pull request process, please assign directxman12
You can assign the PR to them by writing /assign @directxman12 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

shawn-hurley · 2019-09-01T14:15:22Z

/cc @DirectXMan12 @joelanford

k8s-ci-robot · 2019-09-01T14:15:23Z

@shawn-hurley: GitHub didn't allow me to request PR reviews from the following users: joelanford.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @DirectXMan12 @joelanford

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.

Waiting for cache to sync happens when getting a new informer in the informer map.

DirectXMan12

default timeout

See inline

specific error

Yeah, let's return a timeout error so that returns true trivially.

test

Preferably, fake out the HasSynced method or have a dummy informer or something. Alternatively, maybe fake out the restmapper to contain a fake resource, then it won't ever sync cause it can't get anything out of the server?

DirectXMan12 · 2019-09-03T18:23:32Z

pkg/cache/informer_cache.go

 	if err != nil {
 		return err
 	}

 	return cache.Reader.List(ctx, out, opts...)
 }

+// addTimeout adds a default 30 second timeout to a child contxt
+// if one does not exist.
+func addTimeout(ctx context.Context) (context.Context, context.CancelFunc) {


maybe change this to be addTimeoutIfUnset or something.

Also, do we want a default timeout? I'm unsure

Also, do we want a default timeout? I'm unsure

That could also be an option it might be easier to punt the decision to the implementer as they probably know their operator and what it would expect better.

On the other hand, I think a case where we are never going to sync permissions error especially and will hang a worker is a bug that should be fixed.

I am torn :)

That could also be an option it might be easier to punt the decision to the implementer as they probably know their operator and what it would expect better.

That's my opinion -- defaults might fail weirdly in certain conditions, but hanging forever is bad too. The problem is that with defaults, there's not way to say "no, never time out" except to go astronomically large. 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.

Relatedly: what's the practical timeout on client->server requests? If you just eat everything after the handshake on a direct client call, how long will we wait? Forever? If not, we can match that by default

I agree with @DirectXMan12. My opinion is that when context.Context is exposed to callers, its generally to explicitly hand over full control of deadlining. I'd be little confused if I passed context.Background() and it timed out after 30s.

If we're not going to default the timeout, how do we handle GetInformerForKind and GetInformer, which don't accept a context.Context?

Interally use context.Background()

Make a breaking change to the Informers interface to include a Context parameter

Add a new interface that has methods that accept a Context and check whether the cache implementation supports it when calling.

Then there's the concern of how that propagates up the call stack. For example, what would we pass here?

On the other hand, I think a case where we are never going to sync permissions error especially and will hang a worker is a bug that should be fixed.

Is it possible to propagate these errors back up to the caller so that a worker doesn't hang?

I will remove the defaults and add the timeout error.

Is it possible to propagate these errors back up to the caller so that a worker doesn't hang?

We would have to change the underlying informer to do this, I don't know how we could do that without a backward incompatabile change ATM?

I like @DirectXMan12 idea of using the client's default timeout for all of these as it should be expected. note we should add go doc that one of the reasons this can timeout is permissions

we pass here

context.TODO, with a note & bug that when we go to make breaking changes, we fix the signature of start to include a context

(any internal uses where you say "we really should have a real context here, but can't because of the interface" should be TODO, not Background)

as for propagating those errors up, it would be nice eventually, but we'd need to get wrapping errors from 1.13 before that becomes even slightly close to possible.

shawn-hurley · 2019-09-16T16:43:36Z

Hopefully getting back to this today, sorry about the delay

alvaroaleman · 2019-10-25T22:19:18Z

@shawn-hurley Anything one can do to help out here? The current behavior of just continuing to run but not doing anything when CRDs or RBAC are wrong is pretty terrible IMO.

Maybe a simple approach to avoiding that case would be to extend the manager.Options with a cacheSyncTimeout that if non-zero will add another runnable that just does a GetCache().WaitForCacheSync(ctxWithTimeout.Done())?

djzager · 2019-10-28T17:25:41Z

Anything one can do to help out here? The current behavior of just continuing to run but not doing anything when CRDs or RBAC are wrong is pretty terrible IMO.

@alvaroaleman I'm working on creating a PR to finish up this patch based on existing comments.

k8s-ci-robot · 2020-01-22T20:33:46Z

@shawn-hurley: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-controller-runtime-test-master	`813459b`	link	`/test pull-controller-runtime-test-master`

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.

k8s-ci-robot · 2020-01-22T20:33:54Z

@shawn-hurley: PR needs rebase.

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.

📖 bumped up the latest stable version

DirectXMan12 · 2020-02-05T22:12:03Z

close this for now, if anyone wants to pick it up, feel free

(IIRC, it's just a matter for folks being busy)

DirectXMan12 · 2020-02-05T22:12:50Z

/help

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. labels Sep 1, 2019

k8s-ci-robot requested review from DirectXMan12 and pwittrock September 1, 2019 14:14

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

shawn-hurley force-pushed the cache-timeout branch from ed186eb to eb107ca Compare September 1, 2019 14:17

✨ Adding timeout to unset context for waiting for cache to sync.

813459b

Waiting for cache to sync happens when getting a new informer in the informer map.

shawn-hurley force-pushed the cache-timeout branch from eb107ca to 813459b Compare September 2, 2019 13:04

DirectXMan12 reviewed Sep 3, 2019

View reviewed changes

DirectXMan12 mentioned this pull request Sep 3, 2019

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

Closed

shawn-hurley mentioned this pull request Sep 17, 2019

Failed to list *v1.ServiceMonitor: v1.ListOptions is not suitable for converting to "monitoring.coreos.com/v1" operator-framework/operator-sdk#1927

Closed

djzager mentioned this pull request Oct 28, 2019

✨ Take context when getting informer. #663

Merged

1 task

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 22, 2020

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

Merge pull request #580 from droot/book/update-install-version

dd9e0fd

📖 bumped up the latest stable version

DirectXMan12 closed this Feb 5, 2020

DirectXMan12 added the out-of-time-pick-me-up Someone ran out of time, feel free to pick this up label Feb 5, 2020

[WIP] ✨ Adding timeout while waiting for cache to sync #580

[WIP] ✨ Adding timeout while waiting for cache to sync #580

Uh oh!

Conversation

shawn-hurley commented Sep 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Sep 1, 2019

Uh oh!

shawn-hurley commented Sep 1, 2019

Uh oh!

k8s-ci-robot commented Sep 1, 2019

Uh oh!

DirectXMan12 left a comment

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 Sep 3, 2019

Choose a reason for hiding this comment

Uh oh!

shawn-hurley Sep 3, 2019

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 Sep 3, 2019

Choose a reason for hiding this comment

Uh oh!

joelanford Sep 4, 2019

Choose a reason for hiding this comment

Uh oh!

shawn-hurley Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 Sep 9, 2019

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 Sep 9, 2019

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 Sep 9, 2019

Choose a reason for hiding this comment

Uh oh!

shawn-hurley commented Sep 16, 2019

Uh oh!

alvaroaleman commented Oct 25, 2019

Uh oh!

djzager commented Oct 28, 2019

Uh oh!

k8s-ci-robot commented Jan 22, 2020

Uh oh!

k8s-ci-robot commented Jan 22, 2020

Uh oh!

DirectXMan12 commented Feb 5, 2020

Uh oh!

DirectXMan12 commented Feb 5, 2020

Uh oh!

Uh oh!

shawn-hurley commented Sep 1, 2019 •

edited

Loading

shawn-hurley Sep 5, 2019 •

edited

Loading