OCPBUGS-125793: Add opt-in pprof endpoint to machine-controller-manager

[perdasilva]

Description

Add an opt-in pprof profiling endpoint to the AWS machine controller manager, gated behind the --enable-pprof flag (default: disabled).

The pprof server listens on plaintext HTTP at 127.0.0.1:6060 (loopback only). TLS termination and authentication are expected to be handled by a kube-rbac-proxy sidecar configured in the machine-api-operator deployment, consistent with how other serving endpoints are secured across machine-api controllers.

Changes

• cmd/manager/pprof.go — pprofServer runnable that serves net/http/pprof handlers via the manager lifecycle with graceful shutdown on context cancellation.
• cmd/manager/main.go — --enable-pprof and --pprof-bind-address CLI flags. When enabled, registers the pprof server as a manager runnable (no leader election required).
• cmd/manager/main_test.go — unit tests for server response, graceful shutdown, and leader election opt-out.

Dependencies

• Requires a corresponding change in machine-api-operator to inject a kube-rbac-proxy sidecar for the pprof port in the machine-api-controllers deployment (AWS only).

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added optional profiling support with CLI configuration flags: --enable-pprof to activate profiling and --pprof-bind-address to specify the listen address (default: 127.0.0.1:6060).

• Tests

  • Added integration tests for profiling server startup, graceful shutdown, and leader-election behavior.

[openshift-ci-robot]

@perdasilva: This pull request explicitly references no jira issue.

Details

In response to this:

Serve net/http/pprof handlers over TLS on a dedicated port (default :6060) so heap, goroutine, and allocs profiles are available via oc port-forward without rebuilding. The server runs on all replicas regardless of leader election.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[coderabbitai]

Caution

Review failed

An error occurred during the review process. Please try again later.

Walkthrough

The manager binary now includes an HTTP pprof profiling server. CLI flags --enable-pprof and --pprof-bind-address control whether and where the server listens (default 127.0.0.1:6060). When enabled, a pprofServer runnable is registered with the manager, listening on the configured address with graceful 5-second shutdown on context cancellation. Integration tests verify HTTP endpoint reachability and shutdown behavior.

Changes

pprof profiling server feature

Layer / File(s)	Summary
pprof server implementation cmd/manager/pprof.go	pprofServer type with Addr field. Start(ctx) creates an http.Server bound to Addr using http.DefaultServeMux, starts ListenAndServe() in a goroutine, waits for context cancellation, and gracefully shuts down the server with a 5-second timeout. NeedLeaderElection() returns false.
CLI flags and manager registration cmd/manager/main.go	Blank import of net/http/pprof registers standard pprof HTTP handlers. Adds --enable-pprof (default false) and --pprof-bind-address (default 127.0.0.1:6060) flags. Conditionally registers pprofServer with the manager when --enable-pprof is enabled.
pprof server tests cmd/manager/main_test.go	TestPprofServerResponds starts the server on an ephemeral port, polls http://<addr>/debug/pprof/ until reachable, asserts HTTP 200, cancels context, and verifies Start() exits within 10 seconds. TestPprofServerGracefulShutdown verifies shutdown completes within 5 seconds. TestPprofServerNeedLeaderElection asserts NeedLeaderElection() returns false.

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 13 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Test Structure And Quality	⚠️ Warning	TestPprofServerGracefulShutdown lacks defer for context cancellation cleanup, and both multi-purpose tests violate single responsibility by testing startup+shutdown in same test.	Use defer cancel() in TestPprofServerGracefulShutdown; split TestPprofServerResponds into separate startup and shutdown tests; consider using Gomega patterns consistent with repo's existing tests.

✅ Passed checks (13 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: adding an opt-in pprof endpoint to the machine-controller-manager, which aligns directly with the changeset.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	Test file uses standard Go testing, not Ginkgo. All three test names are static and descriptive with no dynamic values.
Microshift Test Compatibility	✅ Passed	Tests are standard Go unit tests, not Ginkgo e2e tests. No Ginkgo patterns found. Tests use only standard crypto/TLS/HTTP packages, no OpenShift or Kubernetes APIs.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	The PR adds standard Go unit tests in main_test.go, not Ginkgo e2e tests. The check only applies to Ginkgo tests (It(), Describe(), Context(), When()). Therefore, the check is not applicable.
Topology-Aware Scheduling Compatibility	✅ Passed	PR adds pprofServer runnable for profiling endpoints, not pod scheduling constraints. No affinity, topology spread, node selectors, tolerations, or PDB configurations.
Ote Binary Stdout Contract	✅ Passed	PR adds pprof server without OTE Stdout Contract violations. New code only logs via klog in async goroutines, not in process-level code. klog properly configured to stderr.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	PR adds unit tests (not Ginkgo e2e tests). Custom check applies only to Ginkgo tests (Describe/It/Context/When), which are absent here.
No-Weak-Crypto	✅ Passed	Code uses only secure cryptography: ECDSA P256, SHA-256, and Go's default TLS configuration. No weak ciphers, custom crypto, or insecure comparisons detected.
Container-Privileges	✅ Passed	PR modifies only Go source code. No Kubernetes manifests or container security context configurations with privileged settings, host namespaces, SYS_ADMIN, or allowPrivilegeEscalation are introduced.
No-Sensitive-Data-In-Logs	✅ Passed	No passwords, tokens, API keys, PII, or sensitive data logged. Logs only bind address (:6060) and generic errors. TLS cert/key paths hardcoded and never exposed.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

Tools execution failed with the following error:

Failed to run tools: Ping-pong health check failed

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (1)

cmd/manager/main_test.go (1)

93-93: ⚡ Quick win

Avoid InsecureSkipVerify in TLS test clients.

Even in tests, prefer trusting the generated self-signed cert via a dedicated RootCAs pool instead of disabling verification entirely.

Also applies to: 155-155

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/manager/main_test.go` at line 93, Replace the insecure TLS configuration
that sets TLSClientConfig: &tls.Config{InsecureSkipVerify: true} with a
verification-friendly setup: create an x509.CertPool, add the test server's
generated self-signed certificate (the PEM bytes used to start the test server)
to that pool, and set TLSClientConfig: &tls.Config{RootCAs: certPool} for the
test HTTP/TLS client; update both occurrences (the TLSClientConfig at the
earlier location and the similar one at the later occurrence) so the client
trusts only the test cert instead of disabling verification.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cmd/manager/main_test.go`:
- Around line 70-75: The tests currently call net.Listen(...) and
client.Get(...) without contexts which can hang; create a bounded context (e.g.,
ctx, cancel := context.WithTimeout(context.Background(), time.Second*5); defer
cancel()) before listening, replace net.Listen("tcp", ...) with
net.ListenConfig{}.Listen(ctx, "tcp", "127.0.0.1:0") (keep the listener variable
and Addr().String() usage), and replace client.Get(url) calls with an
http.NewRequestWithContext(ctx, "GET", url, nil) followed by client.Do(req) to
perform the request; ensure you handle and close response bodies and propagate
errors as before (references: net.ListenConfig{}.Listen,
http.NewRequestWithContext, client.Do, ctx/cancel).
- Around line 46-58: The test currently ignores error returns from pem.Encode
and from certOut.Close()/keyOut.Close(), and also ignores listener.Close() and
resp.Body.Close() in the pprof tests; update the self-signed cert generation
(the blocks calling pem.Encode(certOut, ...) and pem.Encode(keyOut, ...)) to
check and handle the returned error (return or t.Fatalf as appropriate) and
likewise check and handle errors from certOut.Close() and keyOut.Close();
additionally, locate the pprof test code that calls listener.Close() and
resp.Body.Close() and capture their errors (check and fail the test or log them)
instead of discarding them so no I/O failures are ignored.

In `@cmd/manager/main.go`:
- Around line 139-143: The pprof bind default currently uses
flag.String("pprof-bind-address", ":6060", ...) which listens on all interfaces;
change the default value to "127.0.0.1:6060" so pprofAddr defaults to
loopback-only, keeping the same flag name ("pprof-bind-address") and help text
so users must explicitly opt-in to expose it more widely.

---

Nitpick comments:
In `@cmd/manager/main_test.go`:
- Line 93: Replace the insecure TLS configuration that sets TLSClientConfig:
&tls.Config{InsecureSkipVerify: true} with a verification-friendly setup: create
an x509.CertPool, add the test server's generated self-signed certificate (the
PEM bytes used to start the test server) to that pool, and set TLSClientConfig:
&tls.Config{RootCAs: certPool} for the test HTTP/TLS client; update both
occurrences (the TLSClientConfig at the earlier location and the similar one at
the later occurrence) so the client trusts only the test cert instead of
disabling verification.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 731611e2-a703-406c-93d9-64d3a1b6b73b

📥 Commits

Reviewing files that changed from the base of the PR and between 2e4196b and 8354842.

📒 Files selected for processing (2)

• cmd/manager/main.go
• cmd/manager/main_test.go

[coderabbitai]

Actionable comments posted: 0

[perdasilva]

/hold wip

[perdasilva]

/retest

[perdasilva]

/retest

[perdasilva]

/unhold

[damdo]

Thanks Per,
I personally like the change.
Unfortunately we need to tweak it for the TLS constraints we have.

[perdasilva]

/retest

[perdasilva]

/retest

[perdasilva]

/retest

[perdasilva]

/retest

[damdo]

/approve
/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damdo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [damdo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[perdasilva]

/verified by @perdasilva

[openshift-ci-robot]

@perdasilva: This PR has been marked as verified by @perdasilva.

Details

In response to this:

/verified by @perdasilva

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[perdasilva]

/retest

[perdasilva]

/retest

[openshift-merge-bot]

/retest-required

Remaining retests: 0 against base HEAD 2e4196b and 2 for PR HEAD b1d0852 in total

[perdasilva]

/retest

[damdo]

/test e2e-aws

[openshift-ci]

@perdasilva: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[openshift-ci-robot]

@perdasilva: Jira Issue OCPBUGS-125793: Some pull requests linked via external trackers have merged:

The following pull request, linked via external tracker, has not merged:

• openshift/machine-api-operator#1556 is open

All associated pull requests must be merged or unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with /jira refresh.

Jira Issue OCPBUGS-125793 has not been moved to the MODIFIED state.

This PR is marked as verified. If the remaining PRs listed above are marked as verified before merging, the issue will automatically be moved to VERIFIED after all of the changes from the PRs are available in an accepted nightly payload.

Details

In response to this:

Description

Add an opt-in pprof profiling endpoint to the AWS machine controller manager, gated behind the --enable-pprof flag (default: disabled).

The pprof server listens on plaintext HTTP at 127.0.0.1:6060 (loopback only). TLS termination and authentication are expected to be handled by a kube-rbac-proxy sidecar configured in the machine-api-operator deployment, consistent with how other serving endpoints are secured across machine-api controllers.

Changes

• cmd/manager/pprof.go — pprofServer runnable that serves net/http/pprof handlers via the manager lifecycle with graceful shutdown on context cancellation.
• cmd/manager/main.go — --enable-pprof and --pprof-bind-address CLI flags. When enabled, registers the pprof server as a manager runnable (no leader election required).
• cmd/manager/main_test.go — unit tests for server response, graceful shutdown, and leader election opt-out.

Dependencies

• Requires a corresponding change in machine-api-operator to inject a kube-rbac-proxy sidecar for the pprof port in the machine-api-controllers deployment (AWS only).

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

• Added optional profiling support with CLI configuration flags: --enable-pprof to activate profiling and --pprof-bind-address to specify the listen address (default: 127.0.0.1:6060).

• Tests

• Added integration tests for profiling server startup, graceful shutdown, and leader-election behavior.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.