OCPBUGS-62277: Requeue PerformanceStatus update when status write fails#1495
OCPBUGS-62277: Requeue PerformanceStatus update when status write fails#1495MarSik wants to merge 1 commit into
Conversation
WalkthroughAdds a package-level ChangesPerformance Profile Controller Reconciliation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/RHsyseng/operator-utils@v1.4.13: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/coreos/go-systemd@v0.0.0-20191104093116-d3cd4ed1dbcf: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/coreos/ignition@v0.35.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/coreos/ignition/v2@v2.26.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/docker/go-units@v0.5.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/go-logr/stdr@v1.2.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/google/go-cmp@v0.7.0 ... [truncated 19340 characters] ... is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/legacy-cloud-providers: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/metrics: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/mount-utils: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/pod-security-admission: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-apiserver: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tgithub.com/onsi/ginkgo/v2: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" 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 |
MarSik
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@MarSik: This pull request references Jira Issue OCPBUGS-62277, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MarSik 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 |
openshift-ci
Bot
added
the
approved
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@MarSik: This pull request references Jira Issue OCPBUGS-62277, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@MarSik: This pull request references Jira Issue OCPBUGS-62277, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
The old code logged an error when not able to save status, but it never retried. This caused stale conditions to be reported until the next reconcile (possibly long time in the future).
MarSik
force-pushed
the
fix-state-status
branch
from
40bd7af to
06227d4
Compare
|
/cherry-pick release-4.22 release-4.21 release-4.20 release-4.19 release-4.18 |
|
@MarSik: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions 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. |
|
/retest-required |
1 similar comment
|
/retest-required |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![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 `@pkg/performanceprofile/controller/performanceprofile_controller.go`:
- Around line 619-623: The status-update branch currently discards the original
deployment error by returning nil; capture the original deployment error (e.g.,
store the error returned from the deployment step before calling
r.StatusWriter.Update), call r.StatusWriter.Update(ctx, instance, conditions) as
now, and if that update fails return reconcile.Result{RequeueAfter:
statusUpdateRequeueAfter}, <original deployment error> so the reconcile reports
the real failure; specifically, preserve the error produced before the
r.StatusWriter.Update call and return it alongside the RequeueAfter instead of
returning nil.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6d598bdd-c6e2-4f50-8268-769b2d7886a9
📒 Files selected for processing (1)
pkg/performanceprofile/controller/performanceprofile_controller.go
| if err := r.StatusWriter.Update(ctx, instance, conditions); err != nil { | ||
| klog.Errorf("failed to update performance profile %q status: %v", instance.GetName(), err) | ||
| return reconcile.Result{}, err | ||
| return reconcile.Result{RequeueAfter: statusUpdateRequeueAfter}, nil | ||
| } | ||
| return reconcile.Result{}, err |
There was a problem hiding this comment.
Critical: Deployment error is discarded when status update fails.
When the status update fails on line 619, line 621 returns nil error, discarding the original component deployment error from line 607. This causes controller-runtime to treat the reconcile as successful even though deployment failed, masking the real issue.
Controller-runtime supports returning both RequeueAfter and an error. The deployment error must be preserved so the failure is properly recorded.
🔧 Proposed fix to preserve the deployment error
conditions := status.GetDegradedConditions(status.ConditionReasonComponentsCreationFailed, err.Error())
if err := r.StatusWriter.Update(ctx, instance, conditions); err != nil {
klog.Errorf("failed to update performance profile %q status: %v", instance.GetName(), err)
- return reconcile.Result{RequeueAfter: statusUpdateRequeueAfter}, nil
+ return reconcile.Result{RequeueAfter: statusUpdateRequeueAfter}, err
}
return reconcile.Result{}, errThis ensures:
- The deployment error is not lost
- The reconcile is properly marked as failed
- Status update will be retried after 30s
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := r.StatusWriter.Update(ctx, instance, conditions); err != nil { | |
| klog.Errorf("failed to update performance profile %q status: %v", instance.GetName(), err) | |
| return reconcile.Result{}, err | |
| return reconcile.Result{RequeueAfter: statusUpdateRequeueAfter}, nil | |
| } | |
| return reconcile.Result{}, err | |
| if err := r.StatusWriter.Update(ctx, instance, conditions); err != nil { | |
| klog.Errorf("failed to update performance profile %q status: %v", instance.GetName(), err) | |
| return reconcile.Result{RequeueAfter: statusUpdateRequeueAfter}, err | |
| } | |
| return reconcile.Result{}, err |
🤖 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 `@pkg/performanceprofile/controller/performanceprofile_controller.go` around
lines 619 - 623, The status-update branch currently discards the original
deployment error by returning nil; capture the original deployment error (e.g.,
store the error returned from the deployment step before calling
r.StatusWriter.Update), call r.StatusWriter.Update(ctx, instance, conditions) as
now, and if that update fails return reconcile.Result{RequeueAfter:
statusUpdateRequeueAfter}, <original deployment error> so the reconcile reports
the real failure; specifically, preserve the error produced before the
r.StatusWriter.Update call and return it alongside the RequeueAfter instead of
returning nil.
There was a problem hiding this comment.
You cannot combine reconcile.Result{RequeueAfter: statusUpdateRequeueAfter} with non-nil error. See controller-runtime code controller-runtime/pkg/internal/controller/controller.go line 467
There was a problem hiding this comment.
Let me look up the controller-runtime source code to verify this behavior.
@jmencak, you are absolutely correct, and I apologize for the incorrect suggestion.
The controller-runtime documentation explicitly states: "If the returned error is non-nil, the Result is ignored and the request will be requeued using exponential backoff. The only exception is if the error is a TerminalError in which case no requeuing happens." And conversely, "If the error is nil and the returned Result has a non-zero result.RequeueAfter, the request will be requeued after the specified duration."
The controller-runtime internal controller code confirms this — when both RequeueAfter/Requeue are set and a non-nil error is returned, it logs: "Warning: Reconciler returned both a result with either RequeueAfter or Requeue set and a non-nil error. RequeueAfter and Requeue will always be ignored if the error is non-nil."
So the PR's current pattern of return reconcile.Result{RequeueAfter: statusUpdateRequeueAfter}, nil is the correct approach — the nil error paired with RequeueAfter achieves the intended fixed-delay retry without triggering exponential backoff. My suggestion to return a non-nil error alongside RequeueAfter would have been silently ignored by the framework. The existing code in the PR is correct.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
|
Should we address this in all 5 places where Edit: Ignore, that last one is |
|
/lgtm |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/lgtm LGTM to me as well. It seems all correct |
|
/retest |
|
/verified by @mrniranjan |
openshift-ci-robot
added
the
verified
|
@mrniranjan: This PR has been marked as verified by DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/lgtm /hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
e2e-aws-nto-ext is optional. It will pass after #1512 merges. |
|
@MarSik: 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. |
7 participants
The old code logged an error when not able to save status, but it never retried. This caused stale conditions to be reported until the next reconcile (possibly long time in the future).
Summary by CodeRabbit