8000 operator, metrics-exporter: opt-in pprof endpoint on a separate listener by Slach · Pull Request #2133 · Altinity/clickhouse-operator · GitHub
[go: up one dir, main page]
Skip to content

operator, metrics-exporter: opt-in pprof endpoint on a separate listener - #2133

Open
Slach wants to merge 3 commits into
0.28.0from
feature/2016-metrics-exporter-pprof
Open

operator, metrics-exporter: opt-in pprof endpoint on a separate listener#2133
Slach wants to merge 3 commits into
0.28.0from
feature/2016-metrics-exporter-pprof

Conversation

@Slach
@Slach Slach commented Nov 2, 2026 •
edited
Loading
Copy link
Copy Markdown
Collaborator

fix #2016

Summary

Adds an opt-in Go pprof endpoint to both clickhouse-operator and metrics-exporter, so OOMs like the one in #2016 can be profiled. Both binaries use the same mechanism, from one shared helper (pkg/util/profiling):

  • The same flag in both binaries: -pprof-address host:port. It is empty by default, which means disabled.
  • An env override for each container: OPERATOR_PPROF_ADDRESS and METRICS_EXPORTER_PPROF_ADDRESS. The constants are in pkg/apis/deployment/env_vars.go. If both the flag and the env var are set, the flag wins.
  • When enabled, pprof runs on a separate listener with its own http.NewServeMux(). The handlers are registered explicitly: pprof.Index (which also serves heap, goroutine, allocs, block, mutex and threadcreate), Cmdline, Profile, Symbol and Trace. The server shuts down when the process context is cancelled.
  • A Warning is logged whenever pprof is enabled. A second warning is logged if the address is not loopback.
  • Operator: pprof is started once per process from cmd/operator/app/main.go. The controller-runtime managers keep PprofBindAddress unset, which means disabled, so there is no second mechanism inside each manager.
  • metrics-exporter: the /metrics + /chi listener(s) now serve a private mux instead of http.DefaultServeMux. This is required because net/http/pprof's init() registers on DefaultServeMux, whether it is imported blank or named. The operator metrics port already had this fix (0.27.2). Both paths are still registered on one mux served by both listeners, so behaviour is unchanged when -metrics-endpoint and -chi-list-endpoint differ.

How to enable

Helm:

helm upgrade ... --set operator.pprof.enabled=true --set metrics.pprof.enabled=true
# defaults: operator.pprof.address=127.0.0.1:6060, metrics.pprof.address=127.0.0.1:6061
kubectl -n <ns> port-forward deploy/clickhouse-operator 6060:6060 6061:6061
go tool pprof http://localhost:6060/debug/pprof/heap   # operator
go tool pprof http://localhost:6061/debug/pprof/heap   # metrics-exporter

Without Helm, set OPERATOR_PPROF_ADDRESS / METRICS_EXPORTER_PPROF_ADDRESS in the container env, or pass -pprof-address. Both containers share the pod network, so they need different ports. The docs are in docs/monitoring_setup.md#profiling-the-operator-and-metrics-exporter-pprof, and the Helm README values table is regenerated.

Security

  • Off by default. The default manifests and chart render no new env vars, ports or listeners. The install bundle is not changed.
  • Separate listener and dedicated mux. pprof is never registered on DefaultServeMux, and DefaultServeMux is not served anywhere. Ports 9999 and 8888 and the clickhouse-operator-metrics Service are unaffected: they return 404 for /debug/pprof/*, and tests check this.
  • Loopback is recommended and is the chart default. Access is through kubectl port-forward. Any other address logs a warning.
  • pprof is not on the /chi listener, so the IPC Secure-mode middleware cannot be bypassed through it.

Relation to #2017

#2017 was an earlier attempt at the same feature. Its review (#2017 (review)) is addressed as follows:

Review point How this PR handles it
The blank net/http/pprof import registers on DefaultServeMux, which is served via a nil handler on 9999/8888 (info disclosure, DoS, IPC Secure-mode bypass) Neither binary serves DefaultServeMux any more. The exporter's rest_server.go now serves a private mux; operator/machinery.go already did. pprof has its own listener. Tests check that both metrics muxes return 404 for /debug/pprof/*. As a precondition, they also check that DefaultServeMux does serve it, which proves net/http/pprof is linked and the test means something.
"Just a separate mux" is not enough while the metrics servers use nil Done as the review's alternative suggests: net/http/pprof is kept, and the :8888/:9999 servers use explicit non-nil muxes.
Bind loopback-only, never on the metrics ports or behind the Service It is never on the metrics ports or the Service. Loopback is the chart default and the documented setup, and a non-loopback address logs a warning. The address is not forced to loopback, as the requirements for this change specify. Forcing it would also block people who profile outside Kubernetes.
Default OFF, opt-in via a flag consistent with -debug / -fips-info -pprof-address is empty by default, in both binaries. There is an env override (for the Helm and manifest env) and Helm *.pprof.enabled: false.
Consider omitting profile/trace unless separately gated They are kept on purpose: a CPU profile is often needed to explain memory growth. The DoS concern came from these handlers sharing the public metrics listener. Here they are only on an opt-in, loopback-recommended listener, so reaching them already needs port-forward or exec access to the pod.
Log a warning when pprof is enabled A Warning is always logged when pprof is enabled, plus another one for a non-loopback address.
"This PR does not even compile" go build ./..., go vet and go test pass on all touched packages (see the test plan).

Test plan

  • go build ./...
  • go vet on ./cmd/... ./pkg/metrics/... ./pkg/util/profiling ./pkg/apis/deployment
  • go test on the same packages. New tests:
    • pkg/util/profiling: flag/env precedence, loopback detection, every pprof route on the dedicated mux returns 200, Start serves and then shuts down on context cancel, an empty address does nothing.
    • pkg/metrics/clickhouse and pkg/metrics/operator: the metrics mux serves /metrics (and /chi) and returns 404 for /debug/pprof/*; the pprof mux serves it.
  • -h of both binaries lists -pprof-address.
  • Helm: helm lint passes. The default helm template renders no *_PPROF_ADDRESS. With operator.pprof.enabled=true,metrics.pprof.enabled=true both env vars render, also alongside user-supplied metrics.env.
  • The chart was regenerated with dev/generate_helm_chart.sh (source changes are in the script, values.yaml and the hand-maintained values.schema.json). The local yq version also rewrote CRD files, adding !!merge / blank-line differences unrelated to this change. Those were reverted, so only the Deployment template and the README come from the generator.
  • Not run: profiling in a real cluster with port-forward.

Pre-existing failures that this change does not touch: pkg/controller/chi, pkg/controller/chk and pkg/controller/common/statefulset do not build because vet reports "non-constant format string". pkg/model/chbackup and pkg/model/clickhouse (TestEnforceVerifiedLegacyTLS) have failing tests.

Fixes #2016

🤖 Generated with Claude Code

Slach and others added 2 commits November 2, 2026 18:34
@Slach @claude
operator, metrics-exporter: opt-in pprof endpoint on a separate listener
b9a4cf0 
Add a shared pkg/util/profiling helper used by both binaries:
-pprof-address flag (env OPERATOR_PPROF_ADDRESS / METRICS_EXPORTER_PPROF_ADDRESS),
disabled by default, served from a dedicated mux on its own listener,
warning logged when enabled and when bound to a non-loopback address.

The metrics-exporter metrics/CHI-list listener now serves a private mux
instead of http.DefaultServeMux, so the net/http/pprof init() registration
never reaches port 8888 (the operator metrics port already did this).

Helm: operator.pprof.{enabled,address} and metrics.pprof.{enabled,address},
default off, generated via dev/generate_helm_chart.sh.

Fixes #2016

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Slach @claude
CHANGELOG: link #2133
c7afa77 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Slach Slach linked an issue Nov 3, 2026 that may be closed by this pull request
Feature request: register pprof endpoint for profiling usage #2016
Open
@Slach
Merge remote-tracking branch 'origin/0.28.0' into feature/2016-metric…
ebb246a 
…s-exporter-pprof

# Conflicts:
#	CHANGELOG.md

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Feature request: register pprof endpoint for profiling usage

1 participant

@Slach
0