-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
WIP fix #3587: adding a construct between a Watch and an Informer #3616
Conversation
Can one of the admins verify this patch? |
Thinking about this some more, it seems like this should just be added to informer functionality:
I'll update this pr with something that looks more like that. |
6d99576
to
7aab4b4
Compare
SonarCloud Quality Gate failed. |
I've updated this to be a "targeted" informer. To do that you pass a function to save only the state that is needed to make decisions when things change / are deleted. To expand on something making the Store/Cache pluggable in general for informers leaves open the interpretation of events / state. The contract of the informer is that it will generate a series of events using its previously observed state as a reference - so you know what changed from one observation to the next. With a plugable cache if a user is allowed to remove entries, either explicitly or via cache expiration, that could change the meaning of future events - a modification becomes an add for example. If a lookup were added to read the current state from the api server it would change the meaning of modification events by referencing the wrong prior state - that is especially problematic for edge triggered systems. That was what was guiding my thinking of trying to come up with something between an informer and a watch and not expose a cache at all and make it clear that it was only for use with level triggered systems. The sticking point there is giving the user control over how much state is needed in the corner case of delete via a relist, it seems clearer to present this like an informer. |
Here's an attempt of a summary of constructs:
So what we're really trying to determine here is:
|
To keep the ball moving forward, @metacosm @attilapiros based upon the api module work - here's what the natural Cache interface looks like: https://github.com/fabric8io/kubernetes-client/pull/3654/files#diff-b146dfd2b20edaf10226fb2541645bcf2790a6af52550b975c89c4255e1bbd4b Or are you leaning more toward the targeted approach where the memory footprint is less of a concern? |
Looking at things on the go side again, they now have support for pagination built into their informer/reflector: https://github.com/kubernetes/kubernetes/blob/6f896dec4f45ffb82491bc9ce2393e7452886562/staging/src/k8s.io/client-go/tools/cache/reflector.go#L92 - there's a note there on the performance implications. So I'll move ahead with the change to support batch fetching in the base informer, then we can revisit the other differentiation above. After re-reading their code, this is only done to reduce the size of the result constructed by the api server - the client still creates a full in-memory list of the result before processing it with the sync logic, so it's not quite the support I was thinking. |
After several discussions, the work here to have a pruning function will be combined with anything still relevant from #3479 to allow for some modifications to the cache, but without the expectation that user will provide their own implementation. |
replaced by #3943 |
Description
This is a rough cut of supporting something in-between a Watch and an Informer. For now it's easier to develop/show this independently of the informer code as to avoid larger refactorings. This will be rebased after #3609 as there will be some shared changes.
The result is a never ending watch that the user interacts with using a Watcher with an additional method: deletedStateUnknown(String key, String resourceVersion) that conveys that a particular resource has been deleted prior to the relist, so you don't know its full state - more on this below.
This would be constructed off the same context as Informable possibly via new interface (I haven't added that just yet). The key function could always be the uid - note the choice of function affects what shows up in the deletedStateUnknown.
The only other new piece of information needed is a batchSize, which is used to control the pagination size of the batched list fetching.
So this could be as simple as:
A couple of thoughts / alternatives - the handling of deletedStateUnknown could be done like a bookmark. Then rather than passing a key function, we'd assume the use of uid and instead lookup the builder for resource to construct a new instance. Then we'd call deletedStateUnknown(resourceWithUidAndVersion).
Building on that, I think that for delete with unknown state to be useful we need more state - for a typical dependent resource scenario that would at least be the owner references. That could be assumed, or supported via accepting a Unary function for stripping the resource down to only what is needed in the delete case. So you still end up with an in-memory cache, but it will be much smaller than keeping the full objects - likely just uid, resourceVersion, and ownerReferences.
So we can either keep refining this or look to merge some of this into what informers support - at the very least batching.
@attilapiros @manusa @rohanKanojia WDYT?
Type of change
test, version modification, documentation, etc.)
Checklist