WIP: Initial Ref counting #1
Conversation
There was a problem hiding this comment.
I’ve got a bunch of questions around ref counting in c-r and I’d like to get some feedback on these thoughts before I steer too far off course. I had typed all these questions out, but realized it would make more sense to show them in code. This is the minimal working code I could write to demonstrate the questions I have, but I have a feeling it doesn’t make a whole lot sense so let me know what parts I can clarify.
One top level question I have is whether all of this just goes away if we implement EventHandler (de)registering in client-go?
My original thought was that even if ref-counting and EventHandler de-registering exists in the underlying informers in client-go, we would still need to access it from c-r in order to know when to safely stop informers.
But now I’m starting to think that if client-go is responsible for ensuring that informers can only be stopped once all EventHandlers are de-registered, then it seems like we don’t need to track any of that in c-r and only need to remove the individual informers and let client-go tell us (and stop us) when we try to remove an informer with dangling references.
That's not to say that implementing references counting in c-r is a complete waste of time, but it would be helpful to know what parts of any c-r changes are meant to be thrown away once changes are made upstream vs which parts are expected to persist even after support is added upstream.
Can you help clarify my confusion and help me understand how to best divide my effort up between trying to design reference counting for c-r vs for client-go?
pkg/cache/cache.go
Outdated
| // ref counting functions down the stack. | ||
|
|
||
| // Alternatly, we could have sepearte inc, dec, get functions | ||
| ModifyEventHandlerCount(obj runtime.Object, delta int) int |
There was a problem hiding this comment.
Should the functions to increment/decrement/get the reference count for an individual informer live on the cache's Informers() interface or on the underlying Informer() interface?
I currently have it here on Informers() but there’s no reason why it couldn’t live on the Informer() interface and anywhere we call it we’d just have to preface with a GetInformer(obj)
There was a problem hiding this comment.
I'm trying to recall the convo you and Oren had in the meeting yesterday around implementing our own Informer that is a wrapper around the underlying SharedIndexInformer (rather than what we are doing currently, which is to use the SharedIndexInformer directly but shield it behind our own interface that only exposes some of the functions of a client-go Informer).
Would this wrapper struct be a better place to store the reference counts for a given Informer instead of in the informers_map? I'm going to experiment with that now, but wanted confirmation that I was correctly remembering what the gist of what that conversation was about.
There was a problem hiding this comment.
I'd probably put the incrementing on AddEventHandler on the individual informer, then have some sort of decrement and fetch operation there as well, and then we can just roll decrement into Remove if/when we get that upstream.
pkg/cache/internal/informers_map.go
Outdated
|
|
||
| // we can get rid of this if apimachinery adds the ability to retrieve this from the SharedIndexInformer | ||
| // but until then, we have to track it ourselves | ||
| refCount int |
There was a problem hiding this comment.
Does it make sense to store the references as just an integer or are we hoping to actually be able to identify the specific EventHandlers that are referencing the informer? For the sake of simplicity it seems like a count is more straightforward and I’m unclear on how we would identify EventHandlers if we wanted to (and what advantage we’d get by doing so).
If so, does it make sense to store the ref count for an Informer on the MapEntry struct of the informers map? I put it here primarily because I couldn't think of anywhere else to put it (apart from the above comment about a wrapper around the Informer), but I'm not confident that this is a good spot.
| @@ -208,6 +221,11 @@ func (c *Controller) Start(ctx context.Context) error { | |||
| } | |||
|
|
|||
| <-ctx.Done() | |||
| // decrement counts | |||
| for obj, count := range refCounts { | |||
There was a problem hiding this comment.
In terms of reasoning about how to decrement the ref count, I know the idea is to decrement when the controller’s Start() call has stopped.
I’m confused about the cleanest way to do this because the internal controller only has access to a list of watches (which have no understanding of the cache).
My naive and hacky thought is to type assert the watch to a source.Kind within Start (similar to how we’re already casting to source.SyncingSource) and then publicly expose the Cache on the Kind which we can use to decrement ref counts.
I’m not sure it’s safe to assume all controller startWatches will be of type source.Kind nor if there are any concerns around exposing the Cache that was previously private, but I struggled to think how else we could decrement the count once
There was a problem hiding this comment.
Should do it on the source, probably -- sources themselves are runnables, so the source shutting down can manage the removal of the handle, and the controller doesn't need to know anything about it. Should try to avoid breaking the abstraction if possible.
And yeah, not all controller watches are going to be Kind watches -- the intention was that they could be anything.
|
In case it's unclear, the point of this is so then we can then implement Shomron's informer removal solution, but modify the remove call such that it enforces the constraint that any informer being removed must have 0 references (or maybe enforce that the caller is aware of how many references exist on the informer). |
A no-good, dirty-rotten implementation of informer reference counting in controller-runtime to hopefully help answer some questions I have before submitting a more serious interface design to the community.