✨ Add ClusterFilter to ClusterCache Options #12665
Conversation
k8s-ci-robot
added
area/clustercache
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/S
|
Please write the test about this. |
| // ClusterFilter is a function that can be used to filter which clusters should be handled | ||
| // by the ClusterCache. If nil, all clusters will be handled. If set, only clusters for which | ||
| // the filter returns true will be handled. | ||
| ClusterFilter ClusterFilter |
There was a problem hiding this comment.
Let's add some simple unit test coverage (for the reconciler logic)
lentzi90
force-pushed
the
lentzi90/cluster-cache-filter
branch
from
2e015ff to
165bddb
Compare
|
Now maybe the code looks better? |
Yup! I'm wondering if we need test coverage for cases where the filter returns different results for the same cluster over time, as that seems to be supported now? |
k8s-ci-robot
added
size/M
|
My naive attempt to test this did not go so well 😓 |
The problem is simply that you are trying to patch labels with the status subresource :) g.Expect(env.Status().Patch(ctx, testCluster, patch)).To(Succeed())(in l.129 + l.143) |
|
Oh my 🤦 |
This allows filtering the Clusters that are handled by the cache. It can be used for example by providers that only want to cache Clusters of the relevant type to them. Signed-off-by: Lennart Jern <[email protected]>
lentzi90
force-pushed
the
lentzi90/cluster-cache-filter
branch
from
5ac3629 to
ed97655
Compare
|
I'll try to take a closer look next week |
| } | ||
|
|
||
| // Apply cluster filter if set | ||
| if cc.clusterFilter != nil && !cc.clusterFilter(cluster) { |
There was a problem hiding this comment.
wondering if we can avoid to create an accessor entirely if the cluster is filtered out (~like what we can achieve with predicates).
There was a problem hiding this comment.
Not sure how this would look like. The problem is that we should also handle the case where a clusterFilter starts filtering out a cluster that it didn't filter out before
Creating the clusterAccessor is super cheap
There was a problem hiding this comment.
But maybe there's a better option, I assume it requires some refactoring though (and accordingly maybe some risk of introducing new issues)
There was a problem hiding this comment.
Maybe we can move getOrCreateClusterAccessor to line 476 and use getClusterAccessor (+ a nil check) instead "locally" in the if branches where we use accessor above line 476
What this PR does / why we need it:
This allows filtering the Clusters that are handled by the cache. It can be used for example by providers that only want to cache Clusters of the relevant type to them.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #12657
/area clustercache