support removal of event handlers from SharedIndexInformers#98657
support removal of event handlers from SharedIndexInformers#98657mandelsoft wants to merge 6 commits intokubernetes:masterfrom
Conversation
|
Hi @mandelsoft. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
/assign @kevindelgado |
|
@fedebongio: GitHub didn't allow me to assign the following users: kevindelgado. Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. DetailsIn 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. |
|
/assign @DirectXMan12 |
There was a problem hiding this comment.
Hi Uwe, great minds think alike (or fools seldom differ)! I submitted an almost identical PR a bit before yours over here at #98536. We can go ahead with yours though since I think it might be a bit more complete than mine, but I’ve left a few comments/questions on some of the parts where we differ.
I was adding this change as part of some other work I had been doing related to giving finer grained control over the lifecycle of SharedIndexInformers at #97214 I’d love for you to take a look or give it a review that if you have time and are interested.
I’m tagging Yu Liao from sig-api-machinery on this because he has been helping review my other work in this area (and I’m not an org member so I don’t have approval access).
There was a problem hiding this comment.
I'm not sure what the advantages are of extending the SharedIndexInformer Interface (rather than just adding to the SharedIndexInformer interface directly. I can't think of a reason why an informer would not want to expose this and it seems a bit unnecessary to manage more than one interface.
There was a problem hiding this comment.
I did not know whether the informer interface is used by other implementations, also. In such a case extending the interface would break all those other implementations.
This was the reason I decided not to extend the existing interface, but introduce an extended one.
If you don't see a compatibility problem here, I surely can extend the existing interface, instead,
There was a problem hiding this comment.
I know in kubernetes/kubernetes there are no other implementations, not sure about outside of k/k. I'll leave it @yliaog or someone from sig-api-machinery to decide.
There was a problem hiding this comment.
it is a breaking change if extending the SharedIndexInformer Interface directly. For breaking change, a few options: 1) send an email to kubernetes-sig-api-machinery@google.com, or kubernetes-dev@google.com 2) discuss it in biweekly sig-api-machinery meeting. 3) add to release notes about the breaking change
I think it is cleaner to extend the SharedIndexInformer directly, but it's better to check the community to ensure it won't break people unintentionally.
There was a problem hiding this comment.
I've sent I mail to kubernetes-sig-api-machinery@google.com referring to this PR
There was a problem hiding this comment.
I'll put it on the agenda; I can see either breaking compat or adding a "EventHandlerRemovable" as above, but I can't see making this "ExtendedSharedIndexInformer" interface; what will we do when we add another method, make a "ExtendedExtendedSharedIndexInformer"?
There was a problem hiding this comment.
I'm curious what your reasoning and specific use-case is for exposing IsStopped and IsStarted publicly. I hadn't needed or thought to do so.
There was a problem hiding this comment.
It gives you more control managing informers besides factories, once you have bare informers in your hands. and you want to explicitly create/delete watches and/or add/remove handlers again.
There was a problem hiding this comment.
do you have specific use case in mind for EventHandlerCount, IsStopped, IsStarted? add/remove handlers are already supported via AddEventHandler/RemoveEventHandler, could you elaborate on the need for create/delete watches?
There was a problem hiding this comment.
The creation and deletion of watches is not part of the PR, it could completely be done on top. The scenario for the usage of such a feature are controllers that are controlled by other resources. Just an example, you want to provide indices of arbitrary kubernetes resources, based on a dedicated CRD. Once an index is created for a resource, a watch has to be established (or if already existent, only an additional handler might be configured). If the index is deleted again, the handler and, if the watch is not used by other controllers in the same controller manager, the watch should be removed again. Similar actions would be required, if a controller is potentially responsible for multiple clusters, depending on certain conditions provided by other resources.
The deletion of watches is possible just by a dedicated cancel context (on top) used to initiate the watch and the informer. This works fine (beside the problem described in #98653) and is minimal invasive. So, all this is potentially possible just by adding the feature of being able to remove handlers again.
If a traditional InformerFactory is used this is for sure not possible without extending the factory, too. But this PR is the precondition to propagate such a feature up the abstraction levels and focuses on the informer itself.
There was a problem hiding this comment.
thanks for the explanations. i'm not sure i fully understand the use case, two quick questions:
- do you still need a separate watch if informer is used? put it another way, wouldn't an informer for a resource sufficient, hence no need to use both an informer and a watch on the same resource?
- since this PR is about removing event handlers, probably it's better to have a separate PR for supporting watches creation/deletion if needed. what do you think?
|
/unassign @DirectXMan12 |
|
lgtm % a final decision on extending/addding to the shared informer interface. Leaving that up to a reviewer sig-api-machinery. |
|
/ok-to-test |
There was a problem hiding this comment.
do you have specific use case in mind for EventHandlerCount, IsStopped, IsStarted? add/remove handlers are already supported via AddEventHandler/RemoveEventHandler, could you elaborate on the need for create/delete watches?
There was a problem hiding this comment.
it is a breaking change if extending the SharedIndexInformer Interface directly. For breaking change, a few options: 1) send an email to kubernetes-sig-api-machinery@google.com, or kubernetes-dev@google.com 2) discuss it in biweekly sig-api-machinery meeting. 3) add to release notes about the breaking change
I think it is cleaner to extend the SharedIndexInformer directly, but it's better to check the community to ensure it won't break people unintentionally.
staging/src/k8s.io/client-go/tools/cache/shared_informer_test.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/client-go/tools/cache/shared_informer_test.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/client-go/tools/cache/shared_informer_test.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/client-go/tools/cache/shared_informer_test.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/client-go/tools/cache/shared_informer_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
thanks for the explanations. i'm not sure i fully understand the use case, two quick questions:
- do you still need a separate watch if informer is used? put it another way, wouldn't an informer for a resource sufficient, hence no need to use both an informer and a watch on the same resource?
- since this PR is about removing event handlers, probably it's better to have a separate PR for supporting watches creation/deletion if needed. what do you think?
|
@yliaog With the actual state for the pure watch removal no further changes are required. The additional The optional extension of the upper API layer, the informer factories, to support cancelling of dedicated informers should certainly be done separately. But the factory so far does not know about handler registrations, which are directly performed on the bare informers. Therefore it might be difficult to completely hide the informer management in the factories, besides the feature of stopping an informer (and removing it from the factory again). |
lavalamp
left a comment
There was a problem hiding this comment.
Sorry for the delay. The interface seems OK now so I reviewed the implementation, too.
There was a problem hiding this comment.
Does it do any harm to remove it anyway?
In any case, this doesn't seem worth returning an error to the user over.
There was a problem hiding this comment.
Yes it does, if this is removed the addCh of the listener is closed twice, during the stop and the remove. This could be fixed by restructuring the existing code. But I would prefer to delegate this to a separate PR.
There was a problem hiding this comment.
Remove this, we don't set these to nil anymore so this won't optimize anything, right?
There was a problem hiding this comment.
It's just to avoid nil pointer dereferencing in case of some strange problem.
There was a problem hiding this comment.
Is there any reason not to switch the lists to maps/sets? e.g. make the type map[*processorListener]struct{}. Then this check and the removal are easy.
There was a problem hiding this comment.
That is the original coding, I just kept this. May be this could be refactored in a separate PR.
There was a problem hiding this comment.
You should only need to do this second search if found is true, right?
There was a problem hiding this comment.
just kept to handle this in a more defensive way. This should really not be performance critical.
There was a problem hiding this comment.
You can break here unless the listener can get in the loop twice? (another reason to switch to maps)
There was a problem hiding this comment.
break added and i-- removed. Basically the former coding was more defensive.
staging/src/k8s.io/client-go/tools/cache/shared_informer_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Personally I would just wait in a loop until the condition is true, sleep 50ms between each check. Waiting a second and then failing might be flaky.
There was a problem hiding this comment.
I've generalized the ok methods with for a polled check with timeout to use this here, also.
There was a problem hiding this comment.
As stated above, I don't think this should be an error.
There was a problem hiding this comment.
error case removed. But I left the error return to be prepared for further checks. For example whether the handle is valid for the actual informer.
There was a problem hiding this comment.
Missing test: add and remove handlers while an informer is running and processing items, to detect any locking problems.
There was a problem hiding this comment.
tests added for nested add and remove of handlers. May be this is what you wanted.
To be able to implement controllers that are dynamically deciding on which resources to watch, it is required to get rid of dedicated watches and event handlers again. This requires the possibility to remove event handlers from SharedIndexInformers again. Stopping an informer is not sufficient, because there might be multiple controllers in a controller manager that independently decide which resources to watch. Unfortunately the ResourceEventHandler interface encourages to use value objects for handlers (like the ResourceEventHandlerFuncs struct, that uses value receivers to implement the interface). Go does not support comparison of function pointers and therefore the comparison of such structs is not possible, also. To be able to remove all kinds of handlers and to solve the problem of multi-registrations of handlers a registration handle is introduced. It is returned when adding a handler and can later be used to remove the registration again. This handle directly stores the created listener to simplify the deletion.
|
Gentle ping @mandelsoft |
|
@atoato88 I added requested changes and rebases in October and added some comments to conversations to ask whether this is ok. But I didn't get answers up to now. Should I resolve those conversations? From my point of view I'm done, I just add requested changes to bring this PR to an end. |
|
/assign @lavalamp |
|
@mandelsoft: PR needs rebase. DetailsInstructions 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. |
|
@mandelsoft |
|
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:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
|
/remove-lifecycle stale |
|
/reopen I think I was OOO when this last had activity and didn't see it |
|
👀 on this. We have an issue kubernetes-sigs/controller-runtime#1884 that proposes to remove watches dynamically, but controller-runtime couldn't do this unless client-go supports removal of event handlers. |
|
@mandelsoft Do you have plans to continue contributing to this PR? If not I am happy to continue this work in another PR. /assign alexzielenski |
|
other pr is merged so closing this /close |
|
@alexzielenski: Closed this PR. DetailsIn 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. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
To be able to implement controllers that are dynamically deciding
on which resources to watch, it is required to get rid of
dedicated watches and event handlers again. This requires the
possibility to remove event handlers from
SharedIndexInformers again.Stopping an informer is not sufficient, because there might
be multiple controllers in a controller manager that independently
decide which resources to watch.
Unfortunately the ResourceEventHandler interface encourages to use
value objects for handlers (like the ResourceEventHandlerFuncs
struct, that uses value receivers to implement the interface).
Go does not support comparison of function pointers and therefore
the comparison of such structs is not possible, also. To be able
to remove all kinds of handlers and to solve the problem of
multi-registrations of handlers a registration handle is introduced.
It is returned when adding a handler and can later be used to remove
the registration again. This handle directly stores the created
listener to simplify the deletion.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Remark: If as the result of a handler removal a complete informer
should be disabled it is higly recommended to incorporate
pull request #98653, which fixes a race condition when stopping
watches for an informer using the stop channel.
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: