refactor: use deferred status patch in ValkeyNode controller - #274
refactor: use deferred status patch in ValkeyNode controller#274daanvinken wants to merge 1 commit into
Conversation
Replace scattered Status().Patch calls with a single deferred patch at the end of Reconcile. All condition mutations happen in memory via local closures (setCondition, removeCondition, setReady) and are flushed by the defer. This eliminates: - setReadyCondition (re-fetched + patched on every error) - setLiveConfigCondition (re-fetched + patched per call) - clearLiveConfigCondition (re-fetched + patched per call) - updateStatus (re-fetched + patched per call) The deferred pattern is the common approach in Kubernetes operators (e.g. CNPG, Strimzi) and reduces API calls from up to 4 per reconcile to exactly 1. A follow-up PR will apply the same pattern to the ValkeyCluster controller. Signed-off-by: Daan Vinken <daanvinken@tythus.com>
|
Closing this for now. After looking at the diff more carefully, I don't think the refactor justifies itself at the current codebase size. With only 4 status update paths, the old code with #257's setReadyCondition fix is already clear and each helper method is independently testable. The deferred pattern adds closures, named return values, and error merging in the defer, which makes the Reconcile function harder to follow without solving a real problem at this scale. The extra API calls (4 vs 1 per reconcile) are negligible. If the controller grows significantly more status update paths in the future, this pattern would make more sense. For now the current code is fine. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| node.Status.ObservedGeneration = node.Generation | ||
|
|
||
| pvc, err := r.getPersistentVolumeClaim(ctx, node) | ||
| if err != nil { | ||
| return ctrl.Result{}, err | ||
| } | ||
| if node.Spec.Persistence != nil { | ||
| pvcStatus, pvcReason, pvcMessage := pvcStatusCondition(pvc) | ||
| setCondition(valkeyiov1alpha1.ValkeyNodeConditionPersistentVolumeClaimReady, pvcStatus, pvcReason, pvcMessage) | ||
| pvcSizeStatus, pvcSizeReason, pvcSizeMessage := pvcSizeStatusCondition(node, pvc) | ||
| setCondition(valkeyiov1alpha1.ValkeyNodeConditionPersistentVolumeClaimSizeReady, pvcSizeStatus, pvcSizeReason, pvcSizeMessage) | ||
| } else { | ||
| removeCondition(valkeyiov1alpha1.ValkeyNodeConditionPersistentVolumeClaimReady) | ||
| removeCondition(valkeyiov1alpha1.ValkeyNodeConditionPersistentVolumeClaimSizeReady) | ||
| } | ||
|
|
||
| if err := r.updateStatus(ctx, node); err != nil { | ||
| pod, err := r.getPod(ctx, node) | ||
| if err != nil { | ||
| return ctrl.Result{}, err |
There was a problem hiding this comment.
ObservedGeneration committed before error-path returns
node.Status.ObservedGeneration is stamped at line 177, before the getPersistentVolumeClaim (line 179) and getPod (line 193) calls. When either of those returns an error, the reconcile exits early but the deferred patch still fires—writing ObservedGeneration = node.Generation to the API server without a corresponding Ready condition update.
In the old updateStatus implementation, an error from getPersistentVolumeClaim caused the function to return before any patch was issued, so ObservedGeneration was never advanced on partial failures. Any downstream controller (e.g., ValkeyCluster) that reads node.Status.ObservedGeneration == node.Generation as "this generation has been fully reconciled" can draw an incorrect conclusion during the window between the failed reconcile and the next retry.
Move the ObservedGeneration assignment to after the PVC and Pod are successfully fetched, or gate it behind the success path only.
| defer func() { | ||
| if err := r.Status().Patch(ctx, node, patchBase); err != nil { | ||
| log.Error(err, "failed to patch ValkeyNode status") | ||
| if retErr == nil { | ||
| retErr = err | ||
| } | ||
| } | ||
| }() |
There was a problem hiding this comment.
reconcilePersistenceFinalizer requeue
When reconcilePersistenceFinalizer adds or removes the cleanup finalizer and returns requeue=true (lines 146–150), the function exits before any status mutation has occurred. The deferred Status().Patch() still runs at that point, sending a no-op diff to the API server on every finalizer-transition cycle.
This is an extra sub-resource API call per requeue that was absent in the old code. Consider moving the patchBase snapshot and defer registration to after the reconcilePersistenceFinalizer early returns.
| // Snapshot for deferred status patch. | ||
| patchBase := client.MergeFrom(node.DeepCopy()) | ||
|
|
||
| defer func() { | ||
| if err := r.Status().Patch(ctx, node, patchBase); err != nil { | ||
| log.Error(err, "failed to patch ValkeyNode status") | ||
| if retErr == nil { | ||
| retErr = err | ||
| } | ||
| } | ||
| }() |
There was a problem hiding this comment.
The old updateStatus used reflect.DeepEqual(patchBase.Status, current.Status) to skip the Status().Patch() call when nothing changed. That explicit guard is gone. Every reconcile now unconditionally issues a status patch, relying on the Kubernetes API server to not bump ResourceVersion when the JSON merge-patch body resolves to an empty diff.
The corresponding test was also weakened—it dropped the "status write should be skipped when nothing changed" failure message and no longer asserts the skip, only that ResourceVersion is equal.
1 participant
Summary
Replace scattered
Status().Patchcalls in the ValkeyNode controller with a single deferred patch at the end of Reconcile, following the common pattern in operators like CNPG and Strimzi. Idea was discussed at #257 (comment).Implementation
client.MergeFrom(node.DeepCopy())node.Statusin memorydeferflushes the patch, with named return values to propagate errorssetReadyCondition,setLiveConfigCondition,clearLiveConfigCondition,updateStatusTesting
Unit tests pass. Added Reconcile-level tests for Ready=false when no pod exists and idempotency.