controllers: rework status writes#4083
Conversation
openshift-ci
Bot
added
the
approved
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughBoth NUMAResourcesOperator and NUMAResourcesScheduler controller reconcilers are refactored to capture a deep copy of the CR at reconciliation start and use it as the merge base for all status patches, replacing a previous status-update helper and centralizing patch logic into degradeStatus. ChangesStatus Patching Strategy Refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 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 |
move from Update() to Patch() to set Status in the CR owned by controllers. This move enables us to remove the NeedsUpdate functions and use controller-runtime or client-go primitives, which is simpler and more robust. The controllers own the resources anyway, so we don't expect conflicts, and the status writes are always authoritative. Signed-off-by: Francesco Romani <fromani@redhat.com>
Should a status write fail, just return error and let the controller-runtime handle with exponential backoff. This is safe and simple to do once we migrated to use Patch() to send status writes, because in this flow now status write failures are (ven) more likely to be transient errors, so the default backoff curve offers a friendlier time progression. Signed-off-by: Francesco Romani <fromani@redhat.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: 1
🤖 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/numaresourcesoperator_controller.go`:
- Line 159: degradeStatus currently swallows the original reconcile error by
returning only the status-patch error; change its contract so it returns only
the error from the status patch (patchErr) and do not discard the original
reconcile error in callers. Update callers (e.g., the places that call
degradeStatus from getTreesByNodeGroup and the other listed call sites) to
capture patchErr := r.degradeStatus(ctx, instanceBase, instance,
status.ConditionTypeIncorrectNUMAResourcesOperatorResourceName, err) and then
return patchErr if patchErr != nil else return the original err so transient
reconcile failures still trigger retries.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 2e769bad-c73f-4430-9933-98b2abf75ed9
📒 Files selected for processing (2)
internal/controller/numaresourcesoperator_controller.gointernal/controller/numaresourcesscheduler_controller.go
| if req.Name != objectnames.DefaultNUMAResourcesOperatorCrName { | ||
| err := fmt.Errorf("incorrect NUMAResourcesOperator resource name: %s", instance.Name) | ||
| return r.degradeStatus(ctx, initialStatus, instance, status.ConditionTypeIncorrectNUMAResourcesOperatorResourceName, err) | ||
| return r.degradeStatus(ctx, instanceBase, instance, status.ConditionTypeIncorrectNUMAResourcesOperatorResourceName, err) |
There was a problem hiding this comment.
Don’t let degradeStatus swallow transient reconcile failures.
This helper now returns only the status-patch error. That is fine for terminal validation/name problems, but it also turns transient degraded paths into nil returns once the status patch succeeds. In particular, the getTreesByNodeGroup() path can fail on the MCP List() call, and after Line 173 patches status the reconcile stops retrying until some unrelated event fires.
Consider making degradeStatus report only whether the status patch failed, and let each caller decide whether to return the original reconcile error.
Suggested shape
-func (r *NUMAResourcesOperatorReconciler) degradeStatus(ctx context.Context, instanceBase, instance *nropv1.NUMAResourcesOperator, reason string, stErr error) (ctrl.Result, error) {
+func (r *NUMAResourcesOperatorReconciler) degradeStatus(ctx context.Context, instanceBase, instance *nropv1.NUMAResourcesOperator, reason string, stErr error) error {
info := conditioninfo.DegradedFromError(stErr)
if reason != "" { // intentionally overwrite
info.Reason = reason
}
instance.Status.Conditions, _ = status.ComputeConditions(instance.Status.Conditions, info.ToMetav1Condition(instance.Generation), time.Now())
err := r.Client.Status().Patch(ctx, instance, client.MergeFrom(instanceBase))
if err != nil {
klog.InfoS("Failed to update numaresourcesoperator status", "error", err)
}
- return ctrl.Result{}, err
+ return err
} if err := validation.NodeGroups(instance.Spec.NodeGroups, r.Platform); err != nil {
- return r.degradeStatus(ctx, instanceBase, instance, validation.NodeGroupsError, err)
+ if patchErr := r.degradeStatus(ctx, instanceBase, instance, validation.NodeGroupsError, err); patchErr != nil {
+ return ctrl.Result{}, patchErr
+ }
+ return ctrl.Result{}, nil
}
trees, err := getTreesByNodeGroup(ctx, r.Client, instance.Spec.NodeGroups, r.Platform)
if err != nil {
- return r.degradeStatus(ctx, instanceBase, instance, validation.NodeGroupsError, err)
+ if patchErr := r.degradeStatus(ctx, instanceBase, instance, validation.NodeGroupsError, err); patchErr != nil {
+ return ctrl.Result{}, patchErr
+ }
+ return ctrl.Result{}, err
}Also applies to: 168-168, 173-173, 178-178, 190-190, 201-214
🤖 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 `@internal/controller/numaresourcesoperator_controller.go` at line 159,
degradeStatus currently swallows the original reconcile error by returning only
the status-patch error; change its contract so it returns only the error from
the status patch (patchErr) and do not discard the original reconcile error in
callers. Update callers (e.g., the places that call degradeStatus from
getTreesByNodeGroup and the other listed call sites) to capture patchErr :=
r.degradeStatus(ctx, instanceBase, instance,
status.ConditionTypeIncorrectNUMAResourcesOperatorResourceName, err) and then
return patchErr if patchErr != nil else return the original err so transient
reconcile failures still trigger retries.
make the scheduler controller code shape more like the operator/rte controller for better code clarity. Includes the status write requeue fix using the builtin exponential backoff. Other than this, no intended changes in behavior. Signed-off-by: Francesco Romani <fromani@redhat.com>
ffromani
force-pushed
the
handle-status-update-with-patch
branch
from
ec43514 to
e72e14a
Compare
Tal-or
left a comment
Tal-or
left a comment
There was a problem hiding this comment.
/approve
/lgtm
Looks really nice. we may consider (in future PR ofc) to switch to Patch() also for the resources themselves and not only the status.
Following up #1502 which intended to minimize API server load and CPU churn
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ffromani, Tal-or 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 |
|
@ffromani: 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. |
2 participants
we bundle two changes about how controllers write back their status to the apiserver: