Disable caching for Helm Operator when watching all namespaces to avoid OOMKilled in big clusters

[haywoodsh]

Feature Request

Describe the problem you need a feature to resolve.

When a Helm operator is installed in a cluster with a lot of namespaces and k8s objects deployed, the controller caches all the k8s objects, consumes a lot of memory and causes OOMKilled events. Watching one namespace only could potentially reduce memory usage, but our use case requires watching multiple namespaces.

Describe the solution you'd like.

Disable caching for helm-operator when watching all namespaces.

/language helm

[jberkhahn]

So, we are aware of some memory issues in the Helm operator. We actually just merged a pull request that might help with this problem. We should be cutting a release this week or so, could you try rebuilding your operator with the new version and see if that helps? If not we can trying adding something like this.

[madorn]

@jberkhahn @joelanford Thanks for the follow up. Will try the latest PR.

Any info on the possibility of disabling manager cache with helm-operator?

[joelanford]

Another thought: when we create dynamic watches for the operand objects, do we use a label selector to ensure we only watch and cache the subset of those objects that are actually managed by the operator? I'm guessing we don't and that if we did, it would be a significant improvement, especially on larger clusters.

[varshaprasad96]

@jberkhahn Adding some links here in case it helps during investigation:
Using of label selector while starting manager to selectively cache objects: https://github.com/kubernetes-sigs/controller-runtime/blob/main/designs/use-selectors-at-cache.md
Place where we set up the controller to watch the Helm CR:

operator-sdk/internal/helm/controller/reconcile.go

Line 48 in 484013d

type HelmOperatorReconciler struct {

Place where we set up reconciler for each API defined in watches.yaml:

operator-sdk/internal/cmd/helm-operator/run/cmd.go

Line 209 in e859a56

err := controller.Add(mgr, controller.WatchOptions{

This is where we add watches for resources in released Helm charts:

operator-sdk/internal/helm/controller/controller.go

Line 127 in e859a56

func watchDependentResources(mgr manager.Manager, r *HelmOperatorReconciler, c controller.Controller) {

. Since we use the upstream Helm logic to install/uninstall/upgrade the release, I think adding label selectors to cache objects would be in "watchDependentResources".

[jberkhahn]

so, it looks like you can make a predicate based on a label selector and apply it in WatchDependentResources(), but how would you make all the dependent resources have the same label if they're created by just chunking the default helm chart?

[joelanford]

I've been taking a look at this. There are two things to do:

1. For the cache configuration, we need to configure a custom NewCache function on the manager options prior to calling manager.New. This is actually not possible right now due to a bug in controller runtime. I've got a PR up that fixes an issue that helm-operator runs into since the primary CR is unstructured.Unstructured. 🐛 Preserve unstructured object GVKs in cache.Options.ByObject kubernetes-sigs/controller-runtime#2246

2. As @jberkhahn says, we need some label to key off of. My suggestion would be for us to add a new label, something along the lines of helm.sdk.operatorframework.io/chart: <chartName>-<chartVersion>. The easiest spot to add that is in the ownerRefInjectingClient, but it feels a bit dirty to inject a label via a client that says it injects owner refs. Perhaps we rename that client to something a bit more generic that makes us feel better about injecting a few different things.

I'll try to get a PR up here, that uses my controller-runtime fork so that folks can test it.