:sparkles: multiNamespaceCache: support custom newCache funcs per namespace

[joelanford]

This PR adds more flexibility to multi-namespace caches by introducing a new cache builder function (cache.ByNamespaceBuilder) that enables callers to specify individual cache builder functions on a per-namespace basis (with additional support for specifying a catch-all cache builder and a cluster-scoped cache builder)

The primary use case of this is to enable caching the same GVK with a different set of selectors/transformers/deepcopy config based on which namespace the objects are in.

Concretely, imagine a controller that needs to read configmaps in its own namespace and also manage configmaps during reconciliation of its primary object, where each set has effectively nothing to do with the other (i.e. a shared label selector is not feasible). In order to efficiently cache both sets, one might need to:

• Cache all configmaps in <controller>-system namespace
• In all other namespaces, cache only configmaps with label managed-by: <controller>

I originally considered implementing this out-of-tree, but because it so closely resembles and builds upon the existing in-tree multiNamespaceCache, I thought it made more sense to include here.

Signed-off-by: Joe Lanford [email protected]

[joelanford]

#1980 merged, so I rebased to latest master. This PR is ready for review again.

[joelanford]

/retest

[joelanford]

/retest

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: joelanford, varshaprasad96

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:

• ~~OWNERS~~ [joelanford]

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

[sbueringer]

Thx! /lgtm

[joelanford]

@alvaroaleman I'm still interested in getting this merged. I think my overall feeling on this is that it's just a refactoring of the existing MultiNamespaceCacheBuilder that gives users more flexibility in that it allows the NewCache functions to be provided by the user rather than hard-coded (and it adds the optional catch-all cache for namespaced objects)

From my PR description:

I originally considered implementing this out-of-tree, but because it so closely resembles and builds upon the existing in-tree multiNamespaceCache, I thought it made more sense to include here.

I did look into implementing this out-of-tree, but IIRC it would have meant one of:

• exporting the multiNamespaceCache struct and its fields to enable and out-of-tree caller to manipulate them to support different cache building functions
• copying basically all of the multiNamespaceCache implementation out-of-tree AND exporting some of the internal cache code that it uses.

I completely understand your point about not wanting controller-runtime to be a dumping ground for exotic solutions to niche problems.

1. I don't think this is that. I think the use case I'm solving for is probably quite common, but I'd imagine that lots of controllers just end up using a cluster-wide cache and unknowingly consume more memory than necessary.
2. We might need to think about ways to make it easier to build caches like this out-of-tree. It seems to me that there's a good chunk of internal code that would be really useful for building other cache implementations.

[joelanford]

For what it's worth, I've been able to workaround not having this PR merged by making use of a separate Cluster, which is configured with the same apiserver credentials, but with a different namespace scoping.

This workaround means there are two separate caches:

• The "main" cache, which is scoped to all namespaces but is configured with a label selector for objects.
• A cache for the operator's namespace, which is scoped to just the operator's namespace but without any selectors.

As a result of the separate caches, I have to be careful to setup my watches. Specifically, I need to use source.NewKindWithCache for watches of objects in my operator's namespace.

Nonetheless, IMO this workaround is non-intuitive and isn't really an intended use of the Cluster object, so I still believe this PR should be accepted and merged.

[alvaroaleman]

Okay, I am trying to reason through all the possible use-cases here:

1. Limit the namespaces that can be used globally (implemented in multi-ns cache today)
2. Configure a PerNS+PerObject selector (not available today)
3. Configure a per-object selector (available today, if we implement 2 this would only be used for all namesapces that do not have a perNS+perObject selector)
4. Limit namespaces per object, this isn't really possible today (there is no or field selector from what I can see, might be possible to propose upstream? Not sure)
5. Configure a per-namespace selector, this would be used for all namespace+object combination where we don't have a namesapce+object or object level selector
6. Configure a global default for everything else (available today)

Is the above approximately correct or did I miss anything? If yes, I would probably suggest that we end up extenting the cache.Options with:

1. Adding a NamespacesByObject option that limits the namespaces by object, probably with a useSingleInformer setting if it is somehow possible to do that
2. Adding a Namespaces option that globally limits the namespaces and makes the constructor use the multi-namespace cache underneath for objects that do not have a NamespacesByObject configuration
3. If this is possible add a UseSingleInformer option that will be used in the multi ns cache
4. Adding an SelectorByNamespaceAndObject option. This has precedence over the existing SelectorByObject for the configured namespaces. This is basically what you want with this change, IIUIC.
5. Adding a SelectorByNamespace option that we will use unless there is a SelectorByNamespaceAndObject or SelectorByObject config.
6. Finally, we will fall through to DefaultSelector if nothing more specific exists
7. Eventually someone will likely ask for DefaultSelectorExclude lol

What do you think, did I miss anything? I am btw not asking you to implement this, I would just like to have a coherent design of where we want to get to in the end rather than a series of ad-hoc changes that don't really fit in together and are confusing to use.

[k8s-ci-robot]

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

@joelanford: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-controller-runtime-test	40e2e1f93643bebe8943368d2e49c9d9528a3558	link	true	/test pull-controller-runtime-test

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[vincepri]

@joelanford We should chat about a design document on how the above can become a result of reworking the options, the multi-namespace constructor got deprecated on main, and it'll be removed later on. We should work on an overall design that makes sense around the cache.Options

[vincepri]

Closing this for now, let's rally behind a feature design in a hackmd of sorts, or a different PR based on the latest changes, once ready

/close

[k8s-ci-robot]

@vincepri: Closed this PR.

In response to this:

Closing this for now, let's rally behind a feature design in a hackmd of sorts, or a different PR based on the latest changes, once ready

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.