Fix createOrUpdate conflict loop and no-op update storm - #76
Conversation
The createOrUpdate helper performed a naive Get→Update without retry-on-conflict and without checking whether the spec actually changed. This caused two problems on live clusters: 1. Optimistic concurrency conflicts: when another controller (e.g. OVN-K CUDN controller) modified the same object between Get and Update, the resourceVersion mismatch caused a conflict error. The reconciler marked itself Degraded, which triggered a new reconcile, creating a self-sustaining Degraded/Ready flap storm. 2. No-op update loop: even when spec and labels were identical, the unconditional Update bumped resourceVersion, which triggered watches on CUDN/RouteAdvertisements/FRRConfiguration objects, which triggered new reconciles in an infinite loop. Fix by: - Wrapping the Update in retry.RetryOnConflict so transient conflicts are retried automatically with exponential backoff. - Adding specUnchanged() to compare spec and labels before updating, skipping the write entirely when nothing changed. Validated on a live OCP cluster with CUDNBgpRouting and CUDNBgpConfig. Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe controller now compares existing and desired FRR resource specs and labels using semantic equality. It skips updates when no relevant values changed. When an update is required, it refreshes the resource version and retries after conflicts by rereading the current object. Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jpinsonneau 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 |
|
PR needs rebase. 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. |
|
@jpinsonneau: The following tests failed, say
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. |
Summary
While testing netobserv with bgp-cloud-connector, I noticed the operator was stuck in an infinite reconcile loop.
retry.RetryOnConflictso transient optimistic concurrency conflicts (e.g. when OVN-K's CUDN controller modifies the same object concurrently) are retried automatically instead of causing the reconciler to mark itself Degraded and trigger a reconcile storm.specUnchanged()to compare spec and labels before issuing an Update. When nothing changed, the write is skipped entirely, preventing the infinite reconcile loop caused by unconditional Updates bumpingresourceVersionand re-triggering watches.Affects all resources managed through
createOrUpdate: ClusterUserDefinedNetwork, FRRConfiguration, and RouteAdvertisements.Test plan
go build ./...passesgo test ./internal/controller/... -count=1 -shortpassesMade with Cursor