Fix infinite loop on cyclic CSV replacement chains - #3891
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @haklein. Thanks for your PR. I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
📝 WalkthroughWalkthroughThe CSV replacement finder now ignores self-references and stops cyclic replacement-chain traversal. Tests cover self-replacement, two-CSV cycles, cycle termination, and linear chains. ChangesCSV replacement safety
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: can't unmarshal config by viper (flags, file): 1 error(s) decoding:
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 Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/lib/csv/replace_finder_test.go (1)
46-71: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound each replacement-chain traversal.
Lines 51, 58, and 67 call
GetFinalCSVInReplacingsynchronously. If a future change restores the infinite loop, the package stays blocked until the externalgo testtimeout expires. Run each call in a goroutine and fail with a short test-local timeout.Proposed test helper
import ( "testing" + "time" ) +func getFinalWithin(t *testing.T, f func() *v1alpha1.ClusterServiceVersion) *v1alpha1.ClusterServiceVersion { + t.Helper() + result := make(chan *v1alpha1.ClusterServiceVersion, 1) + go func() { result <- f() }() + + select { + case csv := <-result: + return csv + case <-time.After(time.Second): + t.Fatal("replacement-chain traversal did not terminate") + return nil + } +} + - if got := finder.GetFinalCSVInReplacing(self, setOf(self)); got != nil { + if got := getFinalWithin(t, func() *v1alpha1.ClusterServiceVersion { + return finder.GetFinalCSVInReplacing(self, setOf(self)) + }); got != nil {🤖 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/lib/csv/replace_finder_test.go` around lines 46 - 71, Update TestGetFinalCSVInReplacing to execute each GetFinalCSVInReplacing call in a goroutine and enforce a short test-local timeout, failing the test if traversal does not return promptly. Apply this to the self-loop, two-CSV cycle, and linear-chain cases while preserving their existing result assertions.
🤖 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/lib/csv/replace_finder_test.go`:
- Around line 46-71: Update TestGetFinalCSVInReplacing to execute each
GetFinalCSVInReplacing call in a goroutine and enforce a short test-local
timeout, failing the test if traversal does not return promptly. Apply this to
the self-loop, two-CSV cycle, and linear-chain cases while preserving their
existing result assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0317cd24-a29c-45f8-88a1-08ebed4e536a
📒 Files selected for processing (2)
pkg/lib/csv/replace_finder.gopkg/lib/csv/replace_finder_test.go
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
A CSV with spec.replaces equal to its own name sends the replacement chain walk in GetFinalCSVInReplacing into an infinite loop: the CSV watch notification goroutine spins at 100% CPU and writes one Info log line per iteration. Deletion of the CSV does not recover the operator, only a pod restart does. The same walk loops on any cycle that spans two or more CSVs. Guard the finder at all three points: - IsBeingReplaced skips the input CSV, a CSV cannot replace itself. - IsReplacing returns nil for a self-referencing spec.replaces, with a warning that names the misconfigured CSV. A self-replacing CSV now installs normally instead of deadlocking in Pending, where it waited for itself to reach the Replacing phase. - GetFinalCSVInReplacing tracks visited CSVs and stops on the first repeat, which also covers multi-CSV cycles. Fixes OCPBUGS-23954. Co-Authored-By: Claude <noreply@anthropic.com>
9d4f913 to
65fb2c9
Compare
2 participants
Description of the change:
Guard the CSV replace finder (
pkg/lib/csv/replace_finder.go) against self-references and cycles:IsBeingReplacedskips the input CSV.IsReplacingreturns nil for a self-referencingspec.replacesand logs a warning.GetFinalCSVInReplacingtracks visited CSVs and stops on the first repeat.Motivation for the change:
A CSV with
spec.replacesequal to its own name sendsGetFinalCSVInReplacinginto an infinite loop. The CSV watch notification goroutine spins at 100% CPU and writes one log line per iteration. Deletion of the CSV does not stop the loop, only a pod restart does. The same walk loops on any cycle across two or more CSVs. A self-replacing CSV also deadlocks inPending, because the install gate waits for the previous CSV (itself) to reachReplacing. After this change, a self-replacing CSV installs normally.Fixes OCPBUGS-23954.
Architectural changes:
None.
Testing remarks:
pkg/lib/csv/replace_finder_test.gocover the self-loop, a two-CSV cycle, and a linear chain. The looping calls hang without the fix.--writePackageServerStatusNameset and a labeled self-replacing CSV: CPU 137–147%. With the fix: 0.1%, and the CSV reachesSucceeded. Details in OCPBUGS-23954.[FLAKE]suite (14 passed, 0 failed).Reviewer Checklist
/doc[FLAKE]are truly flaky and have an issueSummary by CodeRabbit
Bug Fixes
Tests