Fix memory leak due to misuse of K8s clients

[misberner]

When investigating high memory usage in the RHACS operator, profiling showed that most of the memory consumed by the discovery client's processing of the OpenAPI schema:

The profile seemed odd because an ever-growing amount of memory is allocated in a sync.Once, suggesting repeated execution.

After a debugging run, I found that the ActionClientGetter essentially creates:

• a restClientGetter, with a per-instance lazily created, cached discovery client (seems minor)
• one k8s.io/kubectl/pkg/cmd/util.Factory instance via the kube.New(rcg) call
• another Factory via the cmdutil.NewFactory(rcg) call (only used for the secrets clients)
• a kubernetes.ClientSet (seems minor)
  for each reconciliation of each CR managed by the operator. The major impact is that each factory has its own discovery client, which is initialized again and again, because it is guarded by different instances of the sync.Once.

When I provoked a reconciliation failure, and thereby having the operator perform one reconciliation after the other, I could get the process memory usage to over 2GB in less than a minute.

I haven't fully figured out why garbage collection was of so little use, but I suspect it has something to do with goroutines holding references to some of the objects.

Because all that the various clients need to differ on per CR instance is the namespace, the following PR changes the code to use a single rest client getter, factory, and clientset for everything, injecting the namespace in a lightweight manner when obtaining a per-CR instance action client. With this change, the memory usage hovered around 80MB, even in the above-described reconciliation failure situation.

I believe that what I found and fixed is also the culprit behind several reports of high memory usage:

• Possible memory leak fluxcd/helm-operator#582
• CPU runaway / memory leak in yaml parser fluxcd/helm-operator#427
• https://open.greenhost.net/stackspin/stackspin/-/issues/473
• https://groups.google.com/g/operator-framework/c/-d51fqtYjYk

[misberner]

@joelanford sorry, I forgot to explicitly assign a reviewer at PR creation time.

[coveralls]

Pull Request Test Coverage Report for Build 3049010052

• 39 of 44 (88.64%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.1%) to 89.223%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/client/actionconfig.go	20	21	95.24%
pkg/reconciler/reconciler.go	5	9	55.56%

Totals
Change from base Build 3045539330:	-0.1%
Covered Lines:	1631
Relevant Lines:	1828

---

💛 - Coveralls

[joelanford]

Beautiful! Thanks for finding and fixing this!

/approve

[misberner]

@joelanford thanks for approving, it looks like I cannot merge this. Also, is the go-apidiff failure of any concern?

If you get a chance, could you please take a look at the equivalent fix in operator-sdk? I believe this is required to help existing users of the (non-hybrid) Helm operator.

[joelanford]

I don't think the go-apidiff change is a concern. This repo is not yet 1.0, and the change seems minimal enough that the few number of projects depending on this library should be able to cope.

I'll go ahead and merge. Thanks again!