Add a flag for enabling pprof on the controller manager#4449
Add a flag for enabling pprof on the controller manager#4449Okabe-Junya wants to merge 1 commit intoactions:masterfrom
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) }} |
| {{- with .Values.pprof }} | ||
| - "--pprof-addr={{ .addr }}" | ||
| {{- end }} |
There was a problem hiding this comment.
The with .Values.pprof block will render --pprof-addr={{ .addr }} even if addr is unset/empty (e.g. pprof: {}), which can produce --pprof-addr=<no value> and later an invalid containerPort. Consider conditioning on .Values.pprof.addr and/or using required to ensure addr is provided when pprof is configured.
|
|
||
| ## To enable pprof, uncomment the following lines. | ||
| # pprof: | ||
| # addr: ":6060" |
There was a problem hiding this comment.
The example pprof.addr: ":6060" binds on all interfaces inside the pod, which makes it easy to accidentally expose pprof broadly in-cluster (pprof can reveal sensitive runtime details). Consider updating the example to 127.0.0.1:6060 (or adding a note recommending localhost + kubectl port-forward, or a NetworkPolicy) so the default guidance is safer.
| # addr: ":6060" | |
| # addr: "127.0.0.1:6060" |
| {{- with .Values.pprof }} | ||
| - "--pprof-addr={{ .addr }}" | ||
| {{- end }} |
There was a problem hiding this comment.
New behavior is introduced when .Values.pprof.addr is set (adds a flag + port), and this chart already has extensive Helm template tests. Adding a test case that enables pprof.addr and asserts the rendered --pprof-addr arg and pprof port would help prevent regressions.
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.