-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
✨Use correct context to cancel "list and watch" & wait for all informers to complete #2121
✨Use correct context to cancel "list and watch" & wait for all informers to complete #2121
Conversation
Hi @inteon. 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 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alvaroaleman: GitHub didn't allow me to request PR reviews from the following users: set, of, eyes, for, a, second. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
/retest |
@inteon: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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. |
/ok-to-test |
11170e1
to
a3a7b62
Compare
50c6e20
to
9a2f037
Compare
… to complete Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
9a2f037
to
e466b38
Compare
pkg/cache/internal/informers_map.go
Outdated
@@ -310,17 +334,17 @@ func (ip *InformersMap) addInformerToMap(gvk schema.GroupVersionKind, obj runtim | |||
// Start the Informer if need by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the top of this function, shouldn't we check first if we're stopped already before getting all the way down here and return an error?
If I got the flow right, this function would still return correctly even if the informermap has been stopped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I still return correctly. I did this to make sure the behavior remained identical to before the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably return an error instead of treating it as a no-op, given that the code will silently fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't fail silently, it will error here (identical to current behavior):
return started, nil, apierrors.NewTimeoutError(fmt.Sprintf("failed waiting for %T Informer to sync", obj), 0) |
I think that in order to prevent making a breaking change, we should return a valid informer (that has not been started) from addInformerToMap
.
There are multiple ways to get to that timeout error, eg. if the cache is terminated + all informers are terminated before cache.WaitForCacheSync
returns true. Ideally, we figure out how to cancel this cache.WaitForCacheSync
call when ip.ctx
is canceled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, although it sounds to me it might be a user error to add after Stop has been called. Could we mark this as a breaking change and return that error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll need some more time to figure out how to reliably return an error. I agree that it would be useful to make a breaking change here.
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/lgtm
/hold cancel
Merging this now, the current set of changes look good. Let's follow-up for the next iteration to return an error
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, inteon, vincepri 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 |
This PR fixes this TODO item:
controller-runtime/pkg/cache/internal/informers_map.go
Lines 320 to 322 in b756161
It also make sure that all informers finish (using a
sync.WaitGroup
) before returning from theStart
function.