Add a flag for enabling pprof on the controller manager#4449
Conversation
Okabe-Junya
requested review from
a team,
Steve-Glass,
nikola-jokic,
rentziass and
toast-gear
as code owners
There was a problem hiding this comment.
Pull request overview
This PR exposes controller-runtime’s built-in pprof server configuration through a new controller-manager flag and wires it through the Helm chart so operators can enable profiling without patching the controller.
Changes:
- Add
--pprof-addrflag to controller-manager and pass it to controller-runtime manager options (PprofBindAddress). - Add Helm
values.yamlconfiguration stub for pprof. - Update the controller Deployment template to optionally pass the flag and expose a
pprofcontainer port.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| main.go | Adds --pprof-addr and forwards it into ctrl.Options.PprofBindAddress. |
| charts/gha-runner-scale-set-controller/values.yaml | Documents new pprof.addr values block for enabling profiling. |
| charts/gha-runner-scale-set-controller/templates/deployment.yaml | Renders --pprof-addr and conditionally adds a pprof container port when configured. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - containerPort: {{regexReplaceAll ":([0-9]+)" .controllerManagerAddr "${1}"}} | ||
| protocol: TCP | ||
| name: metrics | ||
| {{- end }} | ||
| {{- with .Values.pprof }} | ||
| - containerPort: {{regexReplaceAll ":([0-9]+)" .addr "${1}"}} |
There was a problem hiding this comment.
containerPort is derived from .Values.pprof.addr using regexReplaceAll ":([0-9]+)" ..., which breaks for common bind addresses that include a host (e.g. 127.0.0.1:6060 becomes 127.0.0.16060). This will render an invalid containerPort and Kubernetes will reject the Deployment. Consider extracting the trailing port number (e.g. via regexFind on [0-9]+$ or splitList and take the last element), and optionally fail fast if no port can be parsed.
| - containerPort: {{regexReplaceAll ":([0-9]+)" .controllerManagerAddr "${1}"}} | |
| protocol: TCP | |
| name: metrics | |
| {{- end }} | |
| {{- with .Values.pprof }} | |
| - containerPort: {{regexReplaceAll ":([0-9]+)" .addr "${1}"}} | |
| - containerPort: {{ required "Values.metrics.controllerManagerAddr must end with a numeric port" (regexFind "[0-9]+$" .controllerManagerAddr) }} | |
| protocol: TCP | |
| name: metrics | |
| {{- end }} | |
| {{- with .Values.pprof }} | |
| - containerPort: {{ required "Values.pprof.addr must end with a numeric port" (regexFind "[0-9]+$" .addr) }} |
There was a problem hiding this comment.
This might be a good suggestion to be honest
nikola-jokic
left a comment
nikola-jokic
left a comment
There was a problem hiding this comment.
Overall looks good! Maybe we can apply the suggestion from the copilot to wrap it in require, but otherwise, looks good!
|
Let me check CI red... 👀 |
|
Hope e859905 would fix |
cf. mercari#3
What
Add a
--pprof-addrflag to the controller manager to expose Go's built-in pprof endpoint.When the flag is set (e.g.
--pprof-addr=:6060), the controller-runtime manager starts a pprof HTTP server on the specified address. When the flag is omitted or empty, pprof remains disabled (no behavior change).The Helm chart is also updated to support configuring this via
values.yaml.Why
When operating ARC at scale, profiling the controller is essential for diagnosing memory leaks, goroutine issues, and CPU bottlenecks. controller-runtime already supports
PprofBindAddressin its manager options, but ARC does not expose it as a flag, so there is currently no way to enable pprof without modifying the source code.