Fix infinite loop on cyclic CSV replacement chains - #3891
Conversation
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
9d4f913 to
65fb2c9
Compare
|
Worth noting a migration concern: before this change,
The nil-before-FailForward dereference at line 2543 is a pre-existing latent bug, but this change makes it reachable in practice. A nil guard there (and/or a note in the commit message about the migration impact) would harden the fix. |
Thanks a lot for catching this. This reproduced exactly as you've described:
Per your recommendation I would add:
with nil guard the upgraded operator stays healthy on the test setup. CSV stays in Replacing with the "no replacement CSV found" sync error, which can be resolved by deleting the CSV. I will add a note of this migration behavior in the commit message. @tmshort Shall I squash before merging the new commits? |
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. 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. All three guards log at Debug: the controller handles the condition silently, so warning-level logging would only repeat an identical line on every reconcile of a permanently misconfigured CSV. Migration note: a CSV left in CSVPhaseReplacing by a previous OLM version whose spec.replaces names itself now resolves to no previous and no next CSV. The fail-forward branch of the Replacing phase guards against the nil next CSV (previously an unguarded dereference, a latent panic this change made reachable). Both upgrade strategies converge on the existing "marked as replacement, but no replacement CSV found" sync error. Recovery is deletion of the CSV. Fixes OCPBUGS-23954. Co-Authored-By: Claude <noreply@anthropic.com>
65fb2c9 to
6e31ea7
Compare
|
@tmshort force pushed the squashed commit with the changes as requested, br Hari |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tmshort 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
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