support removal of event handlers from SharedIndexInformers

[alexzielenski]

What type of PR is this?

/kind feature

What this PR does / why we need it:

Text from original PR:

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:

Continuation of #98657 after work there no longer took place.

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/cc @FillZpp
/cc @kevindelgado

[alexzielenski]

/sig api-machinery
/triage accepted

[kevindelgado]

This looks pretty good to me!

I was struggling to give meaningful feedback, mostly because I was trying to understand the diff between this and where mandelsoft left off but it’s a little hard because a lot has changed between then and now.

My understanding is that the main changes are:

1. Using the opaque interface as the registration handle
2. Turning the shared processor’s listeners slice into a map of listener to a bool of if they’re syncing (making the syncing listeners slice redundant).

Let me know if I’m missing anything else, but overall looks good, I’ll leave it for others to review.

[alexzielenski]

/assign @lavalamp

please take a look when you can

[alexzielenski]

/retest

[lavalamp]

I'm a little concerned about the locking -- often for stuff like this I like to have a test that repeatedly and randomly adds and removes thingies to look for locking errors. Given the tests you added, it's probably not mandatory if you're super confident, but I think that there's a reasonable chance adding this could e.g. expose an existing such bug?

[apelisse]

Thanks, I've looked at the code, will take a look at the tests next.

[apelisse]

Looks good to me, I'll let someone more knowledgable of this area tag for me. Thanks!

[alexzielenski]

/assign @deads2k

[lavalamp]

only one minor comment, probably ready to go otherwise?

[lavalamp]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski, lavalamp

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:

• pkg/controller/replication/OWNERS [lavalamp]
• staging/src/k8s.io/client-go/tools/cache/OWNERS [lavalamp]

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

[alexzielenski]

/retest

[apelisse]

/lgtm

Thanks!