fix(controller): periodic resync to pick up new nodes in source clusters#26
Conversation
The reconciler watches only ClusterMesh CRs. When a new worker node joins a source cluster the CR itself does not change, so the controller receives no watch event and never creates a Peer for the new node. Any workload running hostNetwork=true on that node (e.g. ceph-csi-cephfs nodeplugin) cannot reach the remote cluster until a manual reconcile is triggered. Add syncRequeueAfter (5 min) that fires on every successful, fully-converged reconcile. This caps the worst-case lag between a node joining and its Peer appearing at 5 minutes, covering the race: node joins → kilo daemon writes annotations → next sync window → Peer created → WireGuard tunnel up. The faster bootstrapRequeueAfter (30 s) is retained for the incomplete state (cluster still bootstrapping), so initial setup latency is unchanged. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReplaces the boolean-based ChangesTyped reconciliation state and periodic sync requeue
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a periodic reconciliation interval (syncRequeueAfter set to 5 minutes) for fully-converged cluster meshes to handle node scale-out scenarios where remote nodes do not trigger ClusterMesh CR events. However, the review highlights a critical issue where this logic triggers an infinite reconciliation loop for deleted ClusterMesh resources, and suggests introducing a requeue flag to restrict periodic syncs to steady-state reconciliations.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if err == nil { | ||
| if incomplete { | ||
| return ctrl.Result{RequeueAfter: bootstrapRequeueAfter}, nil | ||
| } | ||
| return ctrl.Result{RequeueAfter: syncRequeueAfter}, nil | ||
| } |
There was a problem hiding this comment.
This change introduces an infinite reconciliation loop for deleted ClusterMesh resources.
When a ClusterMesh is deleted, r.Get returns a NotFound error, which reconcile ignores and returns false, nil. Because err is nil and incomplete is false, this block will return ctrl.Result{RequeueAfter: syncRequeueAfter}, nil, causing the controller to periodically reconcile the non-existent resource forever.
Similarly, when a resource is being deleted or when the finalizer is first added, we should not schedule a periodic requeue.
To fix this, update the reconcile method to return a requeue boolean indicating whether a periodic sync is actually needed (i.e., only when we successfully reach the end of a steady-state reconciliation).
| if err == nil { | |
| if incomplete { | |
| return ctrl.Result{RequeueAfter: bootstrapRequeueAfter}, nil | |
| } | |
| return ctrl.Result{RequeueAfter: syncRequeueAfter}, nil | |
| } | |
| if err == nil && requeue { | |
| if incomplete { | |
| return ctrl.Result{RequeueAfter: bootstrapRequeueAfter}, nil | |
| } | |
| return ctrl.Result{RequeueAfter: syncRequeueAfter}, nil | |
| } |
Extract the three-case dispatch (error/incomplete/converged) into selectResult so it can be unit-tested without envtest. Add table-driven tests for all three cases: error path (zero RequeueAfter, backoff preserved), bootstrap path (30 s), and steady-state sync path (5 m). Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
![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 `@internal/controller/clustermesh_controller.go`:
- Around line 145-148: Add a blank line after the closing brace of the if
incomplete block to satisfy the wsl_v5 and nlreturn linters. The return
statement following the if block needs proper spacing separation to comply with
whitespace linting rules. Insert a blank line between the closing brace of the
if incomplete block and the final return statement that returns ctrl.Result with
syncRequeueAfter.
- Around line 125-148: The selectResult function cannot distinguish between a
fully converged mesh that should be periodically synced and a non-existent or
deleted ClusterMesh that should not be requeued. When reconcile() returns
(false, nil) for a NotFound scenario via client.IgnoreNotFound(err),
selectResult still schedules a periodic requeue with syncRequeueAfter, causing
unnecessary no-op reconciles every 5 minutes. Add an explicit parameter (such as
a "shouldSync" boolean flag or a richer reconcile status enum) to selectResult
alongside the existing incomplete and err parameters, then update the
reconcile() function to return this state information so that NotFound paths can
return no requeue while converged existing meshes still use periodic sync.
In `@internal/controller/requeue_test.go`:
- Line 31: Move the sentinel error variable from inside the test function to a
package-level variable declaration above the test function. Replace the inline
errors.New("reconcile failed") call with a package-level var statement that
creates this sentinel error once, then reference this package variable within
the test instead of creating a new error each time. This satisfies the err113
lint rule which requires sentinel errors to be package-level variables rather
than created inline within functions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f5f778d-1ea7-471b-9253-2c70ae845375
📒 Files selected for processing (2)
internal/controller/clustermesh_controller.gointernal/controller/requeue_test.go
Worst-case delay for a new node to receive a Peer drops from ~5 min to ~1.5 min (1 min sync tick + up to a few 30 s bootstrap cycles while kilo writes annotations). Cost is one extra List call per minute per ClusterMesh CR, which is negligible. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Instead of relying solely on a periodic resync to detect new nodes, register a Node watch on every source cluster's informer cache. When a node is added to or removed from a source cluster, the watch immediately enqueues every ClusterMesh that references that cluster, triggering Peer creation without any polling delay. The periodic syncRequeueAfter is kept at 5 minutes as a safety-net fallback for missed events after watch reconnects. The bootstrapRequeueAfter (30 s) path is unchanged for clusters still converging. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Andrei Kvapil (kvaps)
left a comment
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
Changes requested. The Node-watch design is solid (immediate Peer sync on node join, with ChangeWatcher restarting on registry-affecting changes and the 5-minute resync as a fallback). One blocker before merge:
Deleted/non-existent ClusterMesh gets requeued forever. reconcile() returns (false, nil) for the client.IgnoreNotFound path (clustermesh_controller.go:240), which selectResult(false, nil) maps to the converged branch and schedules syncRequeueAfter (:152). Every ClusterMesh that is ever deleted then leaves a permanent 5-minute no-op reconcile (Get→NotFound→requeue) in the work queue for the lifetime of the operator process. Pre-PR this path returned an empty ctrl.Result{}. This is exactly what the Gemini and CodeRabbit comments flagged — note the CodeRabbit thread is marked "Addressed" but the fix is not present on HEAD.
Suggested fix: thread an explicit "should periodically sync" signal out of reconcile() (a third state, or a synced bool) so only a live, fully-reconciled mesh arms syncRequeueAfter; NotFound / deletion / finalizer-add paths should return ctrl.Result{}. Please add a selectResult test case for the NotFound path — the current table can't reach it, so the bug slips through.
Minor (non-blocking):
SetupWithManager:178-179comment says the predicate reacts to "Ready condition", butGenerationChangedPredicatedoesn't fire onstatus.conditionschanges (no generation bump).ValidateNodedoesn't check Ready, so it's harmless, but the comment is misleading.AnnotationChangedPredicatefires on any annotation change, includingkilo.squat.ai/discovered-endpointschurn — fine (and useful for endpoint enrichment), just worth being aware of at scale.- Consider making
syncRequeueAfter/bootstrapRequeueAfterconfigurable flags. - The PR description only covers the 5-minute periodic resync; please mention the Node watch, which is now the primary mechanism.
selectResult mapped reconcile()'s (false, nil) result to the converged branch and scheduled syncRequeueAfter. The NotFound path (deleted or non-existent ClusterMesh) also returns (false, nil), so every deleted mesh left a permanent 5-minute no-op reconcile in the work queue for the lifetime of the operator process — an unbounded leak under tenant churn. Replace the incomplete bool threaded out of reconcile() with a reconcileState enum whose zero value (reconcileStateDone) schedules no requeue. Terminal paths (NotFound, deletion, finalizer-add, CIDR overlap) now return reconcileStateDone; only a live, fully reconciled mesh arms syncRequeueAfter and a still-converging mesh arms bootstrapRequeueAfter. Add a selectResult test case covering the no-requeue terminal path. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Andrei Kvapil (kvaps)
left a comment
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
The blocker (deleted/terminal ClusterMesh scheduled a perpetual 5-minute no-op requeue) is resolved in 46ea373: reconcile() now returns a reconcileState whose zero value schedules no requeue, terminal paths (NotFound, deletion, finalizer-add, CIDR overlap) return it, and the new test case covers the no-requeue path. CI is green (build, test, integration, lint). Approving.
3 participants
Problem
When a worker node is added to a source cluster, the ClusterMesh CR does not
change, so the controller receives no watch event. The reconciler never runs,
never creates a Peer object for the new node, and any hostNetwork workload on
that node (such as the ceph-csi-cephfs nodeplugin) cannot reach the remote
cluster. The failure manifests as mount errors with "Module ceph not found"
and "no mds up" until a manual reconcile is triggered via annotation.
Solution
Add
syncRequeueAfter(5 minutes) that fires after every successful,fully-converged reconcile. This caps the worst-case delay between a node
joining and its Peer appearing at 5 minutes:
node joins → kilo daemon writes annotations → next sync window → Peer created → WireGuard tunnel up → CSI mount succeeds
The existing
bootstrapRequeueAfter(30 s) is retained for the incompletestate, so initial cluster setup latency is unchanged. On error, RequeueAfter
remains zero so controller-runtime's exponential backoff is unaffected.
Summary by CodeRabbit
Refactor
Tests