-
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
fix #3587: adding inform support for limit/batch fetching #3753
Conversation
Can one of the admins verify this patch? |
25ed385
to
2e20ebd
Compare
@metacosm @attilapiros Some additional thoughts - it seems to be a pretty advanced case to consider using the limit option with an informer. There are quite a few comments around the behavior in the go client. I don't see much in the literature about it being a built-in case, but it seems like you'd also look towards a sharding pattern - an overall/coordinating operator would create something like a statefulset of controllers, where each has responsibility for a subset of the resources - driven by an annotation (something that is filterable by informers/kubernetes) added by the overall operator. |
2e20ebd
to
518c425
Compare
I can't find now the source, but I recall that operators should be singletons (or work with some sort of leader-election mechanism). (Which is something that disappointment given the nature of K8s and its overall purpose) Is this new sharding approach something documented? I understand that this has nothing to do with the scope of the current PR, or does it? |
No, just a musing on how you would approach it assuming a "singleton" coordinating operator.
So far the only request for this limit/batching logic is to work towards scaling operators. If there are other applicable approaches we should determine what is best to ultimately peruse. The next step from here is to control the memory footprint of the cache - from #3636 that either looks like a mapping/pruning function or a store that's not fully in memory. I don't think the latter seems like the right direction. As for this change it does seem like it can stand alone based upon the go client. There are situations where it is beneficial to form smaller paginated results. |
To elaborate further - you'd have the coordinating operator set a hash on the resource as a label in the range of the number of statefulset replicas. The statefulset pods would be setup to provide the pod name via an env property, which means they could parse out the pod index to determine which bucket they were responsible for. The individual informers would then be setup to only look for resources with that index label. Any dependent resources would need to have that label transitively set as well for the sharding pattern to work all the way down. |
Yes, I figured some mechanism like this. However, I might still see some flaws depending on the use-case (from my undetailed, first-glance perspective). For some other uses, it might not be necessary to have an orchestrating process/controller (i.e. considering a StatefulSet, the Pod's ordinal name might be enough to devise a constant strategy to self-assign the bucket). Anyway, I guess this is really off topic for the PR. |
To self-assign the hash/index label would already need to be present on the resources.
Yes, it was just to add some more to the discussion about operator scaling. I'll add a reference to this to the other on-going issue. |
Well, in my imaginary example the hash (or input for the resource assignment) could be computed from the resource UID or something similar. |
Something (which I'm referring to as the overall operator) has to compute a hash to the index and apply it as a label to the resource - it needs to be filterable by an informer. I tried to summarize all the extra work this would entail at #3587 (comment) |
😅 I forgot about the filtering part :) From your detailed explanations I get the following:
Support for some sort of server-side dynamic labels or wildcard filtering would be of help to completely remove the orchestrating part (kubernetes/kubernetes#30755) |
518c425
to
9ab00d4
Compare
SonarCloud Quality Gate failed. |
Just wanted to say thanks for adding this feature! My team just ran into needing the ListerWatcher to paginate in the Informer's Reflector and we realized the ability was something we just got from upgrading versions 🙌 |
No problem, hope it works out well. |
Description
Related to #3587 #3616 - adds limit / batch fetching support to informers. This can cut down on the size of the responses from the server and eventually the memory footprint of an informer. To support the latter this will not fall back to fetching the entire list like the go client - this may need some refinement as currently it will just keep retrying.
If the limit is not specified, the default is to fetch everything in a single list.
A behavioral difference introduced here is that the affect of a relist is no longer atomic on the cache - rather each item in the relist will perform a cache update and create an event as it is seen. Since the cache is still eventually consistent either way, I think this change is not a concern.
The javadocs call out a performance warning - it seems that pagination hits etc.d directly and so there's a concern about performance and scaling.
These changes to conflict of course with the api module creation, but that will be easy to resolve.
Type of change
test, version modification, documentation, etc.)
Checklist