Add periodic resync to operator reconciler - #1504
Conversation
kubevirt-bot
added
release-note
|
[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 |
![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 resync mechanism for the NMState operator to ensure that externally modified resources, such as namespace annotations, are restored. The implementation adds a manual resync loop via a Start method and includes a watch for Namespace resources. Feedback suggests simplifying the implementation by using the idiomatic RequeueAfter result in the Reconcile function instead of a manual loop, which would also eliminate the need to register the reconciler as a separate runnable. Additionally, the watch on Namespace resources should be removed as they are not owned by the NMState CR, making the Owns call ineffective.
| func (r *NMStateReconciler) Start(ctx context.Context) error { | ||
| ticker := time.NewTicker(ResyncPeriod) | ||
| defer ticker.Stop() | ||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return nil | ||
| case <-ticker.C: | ||
| r.Log.Info("Periodic resync triggered") | ||
| instanceList := &nmstatev1.NMStateList{} | ||
| if err := r.List(ctx, instanceList, &client.ListOptions{}); err != nil { | ||
| r.Log.Error(err, "failed listing NMState instances during periodic resync") | ||
| continue | ||
| } | ||
| for i := range instanceList.Items { | ||
| instance := &instanceList.Items[i] | ||
| if _, err := r.Reconcile(ctx, ctrl.Request{ | ||
| NamespacedName: types.NamespacedName{ | ||
| Name: instance.Name, | ||
| Namespace: instance.Namespace, | ||
| }, | ||
| }); err != nil { | ||
| r.Log.Error(err, "failed to reconcile NMState instance during periodic resync", | ||
| "name", instance.Name) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Implementing a manual resync loop via manager.Runnable adds unnecessary complexity to the controller. This approach bypasses the controller's workqueue and safety mechanisms. A more idiomatic and simpler way to achieve periodic reconciliation is to return ctrl.Result{RequeueAfter: ResyncPeriod}, nil at the end of the Reconcile function. This leverages the built-in queueing mechanism and adheres to the preference for simpler implementations.
References
- Changes that add complexity to 'future-proof' the code against hypothetical scenarios require strong justification. Without it, prefer the simpler implementation that covers known requirements.
| For(&nmstatev1.NMState{}). | ||
| Owns(&appsv1.Deployment{}). | ||
| Owns(&appsv1.DaemonSet{}). | ||
| Owns(&corev1.Namespace{}). |
There was a problem hiding this comment.
The Owns method is intended for resources that have an OwnerReference pointing to the primary resource being reconciled (NMState). Since the Namespace is not owned by the NMState CR, this watch will not trigger any reconciliation. If immediate reconciliation on namespace changes is not a strictly defined requirement, this watch should be removed to keep the implementation simple, relying on the periodic resync instead.
References
- Changes that add complexity to 'future-proof' the code against hypothetical scenarios require strong justification. Without it, prefer the simpler implementation that covers known requirements.
| // Register periodic resync to ensure externally modified resources | ||
| // (e.g. namespace annotations) are restored. | ||
| if err = mgr.Add(reconciler); err != nil { | ||
| return fmt.Errorf("failed adding periodic resync runnable: %w", err) | ||
| } |
There was a problem hiding this comment.
If the manual Start method is removed from the reconciler in favor of the simpler RequeueAfter approach, this call to mgr.Add is no longer necessary and should be removed to reduce code complexity.
References
- Changes that add complexity to 'future-proof' the code against hypothetical scenarios require strong justification. Without it, prefer the simpler implementation that covers known requirements.
0f3341a to
f2d7d8f
Compare
f2d7d8f to
9923a61
Compare
qinqon
left a comment
qinqon
left a comment
There was a problem hiding this comment.
@mkowalski I would do a belt and suspenders and do both, Namespace reconcile (you have to be sure that we filter the cache to just our namespace) and the 5 min thing you have already do, this way we have the best of both words.
|
@mkowalski let's re-purpose this PR to watch for the kubernetes-nmstate namespace instead. |
kubevirt-prow
Bot
added
the
needs-rebase
|
@qinqon done — kept the 5 min periodic resync and added a This message was generated using AI. Please verify before acting on it. |
Return RequeueAfter from the Reconcile function to schedule periodic re-reconciliation every 5 minutes. This ensures that externally modified resources, such as the openshift.io/node-selector namespace annotation, are periodically restored. Previously, the operator only reconciled on NMState CR, Deployment, or DaemonSet changes. If the namespace annotation was removed externally, the reconciler would never fire and pods scheduled after the removal would remain stuck in Pending state on clusters with defaultNodeSelector configured. Fixes: https://redhat.atlassian.net/browse/OCPBUGS-67277 Signed-off-by: Mateusz Kowalski <mko@redhat.com> Generated-by: OpenClaw OpenClaw 2026.4.15 (041266a) AI-model: claude-opus-4.6
In addition to the periodic resync, watch the handler namespace and map its events to NMState reconcile requests so externally modified namespace metadata (e.g. annotations) is restored immediately. The manager cache is restricted to the handler namespace for Namespace objects to avoid caching every namespace in the cluster. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
a23975e to
d92203a
Compare
kubevirt-prow
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Pull request overview
Adds periodic reconciliation to restore externally modified operator-managed resources.
Changes:
- Requeues successful reconciliations every five minutes.
- Watches handler namespace changes for immediate reconciliation.
- Restricts namespace caching and adds relevant tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
controllers/operator/nmstate_controller.go |
Adds periodic requeue and namespace event mapping. |
controllers/operator/nmstate_controller_test.go |
Tests requeue results and namespace mapping. |
cmd/operator/main.go |
Limits the namespace cache to the handler namespace. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@mkowalski: The following test failed, say
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. |
|
Hey @qinqon, let's try moving this one forward. I applied your last comment and hopefully we are ready to go |
|
/approve Human review, looks good. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mkowalski 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 |
kubevirt-prow
Bot
added
the
approved
Is this a BUG FIX or a FEATURE?:
/kind bug
What this PR does / why we need it:
Problem: The operator only reconciles on NMState CR, Deployment, or DaemonSet changes. If the
openshift.io/node-selector: ""namespace annotation is removed externally, the reconciler never fires and pods remain stuck in Pending state on clusters withdefaultNodeSelectorconfigured.Fix (belt and suspenders, per review):
ctrl.Result{RequeueAfter: 5 * time.Minute}fromReconcile, adding a periodic resync similar to what CNO does with its 3-minute resync timerWatches+EnqueueRequestsFromMapFunc, mapping its events to NMState reconcile requests so external modifications are reverted immediatelymetadata.name == HANDLER_NAMESPACE, so the operator does not cache/watch every namespace in the clusterThis ensures that externally modified resources are restored to their desired state — immediately on namespace events, and at worst within the resync period for anything else.
Ref: https://redhat.atlassian.net/browse/OCPBUGS-67277
Special notes for your reviewer:
Inspired by CNO's
ResyncPeriodapproach. The 5-minute interval is conservative enough to not cause excessive API server load but responsive enough to fix the issue within a reasonable time window.Release note: