Support controller-runtime integration for pprof

[JorTurFer]

Summary

We are facing with some performance issues that we want to troubleshoot. For going deeper, we want to profile the app for detecting the bottlenecks, but I've noticed that controller-runtime built-in integration isn't configured.

Controller-runtime can enable (or not) the profiling just configuring the address (or not). This is really easy to integrate/configure and allows profiling ArgoCD components just changing a value on cm and restarting a pod, which is so much better than adding the profiling code in the old (and hard) way.

Motivation

I have to profile applicationset-controller and it requires changes in code for enabling the profiler. It also means that I have to checkout the exact version I've deployed to base my changes on it, which is more effort. Integrating this built-in feature I'd not need to change a line of code and I can profile the components easily when I need it

Proposal

I'd just configure PprofBindAddress on controllers. IDK if reading the value from cm or maybe from args

[JorTurFer]

Tomorrow I'll try to draft a PR with this feature, you can assign the issue to me if you want :)

[JorTurFer]

This will be more complicated than I expected:

The built-in feature was added in [email protected] but there are too much replaces to k8s 0.26 and there are several incompatibilities.

Apart from that, only applicationset-controller uses controller-runtime to generate the managers, other components use listers/informers generated using raw kube-client (for instance):

config, err := clientConfig.ClientConfig()
....

kubeClient := kubernetes.NewForConfigOrDie(config)
appClient := appclientset.NewForConfigOrDie(config)

I need to think about how to solve it in a single way for all the
components. Maybe writing a helper like controller-runtime does but not directly using it... 🤔
WDYT @blakepettersson @crenshaw-dev ?

[blakepettersson]

@JorTurFer I think those replaces are caused by having k8s.io/kubernetes as a dependency and ideally would be solved with argoproj/gitops-engine#535, or with someone else continuing the work on that.

[JorTurFer]

The problem is that the replaces aren't the only problem I faced with. As only the applicationset-controller is directly build on top of controller-runtime, only it can enable this feature. Other components use kubernetes client, but don't looks as built on top of controller-runtime because they don't have any point where that dep is used.

Based on that, my proposal is to create a helper shared for all ArgoCD components that starts up a server with the same endpoints as controller-runtime's server, adding the same features. Basically my idea is to port that feature from controller-runtime to ArgoCD instead of integrate it (due to the replaces and also the controller-runtime usage stuff)

WDYT?

[blakepettersson]

@JorTurFer it makes sense to me, especially given that something like this would be useful for argocd-server and the repo server. Perhaps if for now this feature is limited to the applicationset-controller (since that's where your original pain point was) and we thereafter can port this to the other Argo components? WDYT @crenshaw-dev?

[JorTurFer]

I think that we can include something like controller-runtime did: https://github.com/kubernetes-sigs/controller-runtime/pull/1943/files

Basically with a few lines we could create an optional server that we enable based on a given config. I think that it's something easy and "clean"

[blakepettersson]

@JorTurFer makes sense to me 👍

[JorTurFer]

nice! I'll draft a PR when I have some time :)

[JorTurFer]

Late is always better than never, so I've just drafted the PR: #17068 🤣

Let's move the discussion there :)

[agilgur5]

Noting here that per #17068 (comment), pprof was already supported after #7533, but it was running unconditionally (which is a security and perf issue) as well as always on the metrics port.

EDIT: well, it was only enabled for the Application Controller and the Server (not all the current components may have existed at the time, the Repo Server may have been the only one missing then).
#17068 adds pprof support for the rest of the current components as well