fix(ROSAENG-64778): dvo and metrics clients leaking goroutines - #693
fix(ROSAENG-64778): dvo and metrics clients leaking goroutines #693holysoles wants to merge 3 commits into
Conversation
strings.HasPrefix with "openshift-*" never matches, so all PDBs were validated including core openshift platform PDBs
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR configures reusable HTTP transports for DVO and Prometheus clients, adds request and goroutine behavior tests, and corrects OpenShift namespace prefix matching in the PDB filter. ChangesClient transport and validation
PDB namespace filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@pkg/dvo/builder.go`:
- Around line 47-57: The reusable transports in pkg/dvo/builder.go lines 47-57
and pkg/metrics/metrics.go lines 161-173 need bounded full-request cancellation:
add an appropriate overall transport timeout and ensure the HTTP requests
created by the DVO GetMetrics() and Prometheus Query() flows use
deadline-bearing contexts. Preserve the existing connection, keep-alive, proxy,
and TLS timeout settings while applying the same protection at both sites.
In `@pkg/dvo/client_test.go`:
- Around line 27-28: Check URL parsing and port conversion errors in the test
helper at pkg/dvo/client_test.go:27-28 and apply the same validation at
pkg/metrics/metrics_test.go:26-28. In each helper, handle url.Parse errors
before accessing the parsed URL, then handle strconv.Atoi errors before using
the port, reporting failures through the existing test context.
- Line 82: Migrate the four DVO scenarios in pkg/dvo/client_test.go at lines
82-82, 111-111, and 133-133, and the three metrics scenarios in
pkg/metrics/metrics_test.go at lines 94-94, 123-123, and 151-151 from standalone
Test* functions into Ginkgo v2 Describe/It blocks. Replace their assertions with
Gomega assertions and preserve each scenario’s existing behavior.
🪄 Autofix
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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: a0916b1d-feb9-41b5-92e5-2a05603242fd
📒 Files selected for processing (6)
pkg/dvo/builder.gopkg/dvo/client.gopkg/dvo/client_test.gopkg/metrics/metrics.gopkg/metrics/metrics_test.gopkg/upgraders/healthcheck_pdb.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #693 +/- ##
==========================================
+ Coverage 54.33% 55.77% +1.44%
==========================================
Files 123 123
Lines 6212 6218 +6
==========================================
+ Hits 3375 3468 +93
+ Misses 2631 2533 -98
- Partials 206 217 +11
🚀 New features to boost your workflow:
|
Creating a new http.Transport per request in RoundTrip leaks ~3 goroutines per call. Move transport construction to the dvo and metrics client builders so it's created once and reused. Tests Generated by: Claude Code (opus 4.6)
13bb02c to
720e8cd
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/dvo/dvo_suite_test.go (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Ginkgo v2 in the updated suite files.
Update the consolidated suite imports for the V2 tests:
pkg/dvo/dvo_suite_test.go#L6-L6pkg/metrics/metrics_suite_test.go#L6-L6Use
github.com/onsi/ginkgo/v2instead ofgithub.com/onsi/ginkgo, and remove the unused legacygithub.com/onsi/ginkgomodule requirement.🤖 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 `@pkg/dvo/dvo_suite_test.go` at line 6, Update the Ginkgo imports in pkg/dvo/dvo_suite_test.go (line 6) and pkg/metrics/metrics_suite_test.go (line 6) to use github.com/onsi/ginkgo/v2 instead of the legacy package, then remove the unused github.com/onsi/ginkgo module requirement.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@pkg/dvo/dvo_suite_test.go`:
- Line 6: Update the Ginkgo imports in pkg/dvo/dvo_suite_test.go (line 6) and
pkg/metrics/metrics_suite_test.go (line 6) to use github.com/onsi/ginkgo/v2
instead of the legacy package, then remove the unused github.com/onsi/ginkgo
module requirement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: d7d4ced6-bab6-4c7b-bfc5-dd484b8fbd7c
📒 Files selected for processing (6)
pkg/dvo/builder.gopkg/dvo/client_test.gopkg/dvo/dvo_suite_test.gopkg/metrics/metrics.gopkg/metrics/metrics_suite_test.gopkg/metrics/metrics_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/dvo/builder.go
- pkg/metrics/metrics.go
36933bf to
720e8cd
Compare
|
@holysoles: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/lgtm Looks good to me, holding to give SMEs a chance to review if they want. |
openshift-ci
Bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: holysoles, joshbranham The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
3 participants
What type of PR is this?
(bug/feature/cleanup/documentation/test/refactor)
Bug
What this PR does / why we need it?
Both dvoRoundTripper.RoundTrip and prometheusRoundTripper.RoundTrip create a new http.Transport on every HTTP request as a local variable.
Each transport spawns background goroutines for connection management. Since this is done in a local variable and no reference to the transport is retained, CloseIdleConnections() is never/unable to be called and the goroutines and their associated buffers become unreachable but uncollectable. The newly added tests validate that these goroutines are being leaked.
I found this issue when investigating MUO getting OOM killed on a production cluster when attempting to run upgrade preflight checks. Claude estimates each leaked transport at ~40-50KB of memory (goroutine stacks + connection buffers + TLS state). I am not able to confirm this is the root cause of that production issue, but was the only memory leak I could find and feels worth fixing.
Since the transport settings are static for the life of the client, we should be able to initialize the transport settings once and reuse it. Additionally, we this should let us benefit from connection pooling, reducing a small amount of cpu overhead of a new TLS handshake for every client request.
additional fixes
checkPodDisruptionBudgets namespace filter is using strings.HasPrefix(ns, "openshift-*"), but * will be treated as a literal character, not a wildcard. No namespace will ever match that, so all PDBs are being checked regardless of namespace.
Per coderabbit review, I also added request-level timeouts to these clients since we could block the reconciliation loop if a server hung while responding.
Which Jira/Github issue(s) this PR fixes?
Fixes Jira https://redhat.atlassian.net/browse/ROSAENG-64778
Special notes for your reviewer:
Pre-checks (if applicable):
Summary by CodeRabbit