Skip to content

OCPBUGS-62277: Requeue PerformanceStatus update when status write fails #1495

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 1 commit into from
May 18, 2026
+7 −3
Merged

OCPBUGS-62277: Requeue PerformanceStatus update when status write fails #1495

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions pkg/performanceprofile/controller/performanceprofile_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@ import (
const (
openshiftFinalizer = "foreground-deletion"
hypershiftFinalizer = "hypershift.openshift.io/foreground-deletion"

// statusUpdateRequeueAfter is the delay before retrying reconcile after a failure to update performance profile status.
statusUpdateRequeueAfter = 30 * time.Second
)

// PerformanceProfileReconciler reconciles a PerformanceProfile object
Expand Down Expand Up @@ -615,13 +618,14 @@ func (r *PerformanceProfileReconciler) Reconcile(ctx context.Context, req ctrl.R
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{}, err
return reconcile.Result{RequeueAfter: statusUpdateRequeueAfter}, nil
}
return reconcile.Result{}, err
Comment on lines 619 to 623
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

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{}, err

This 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.

Suggested change
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.

Copy link
Copy Markdown
Contributor

@jmencak jmencak May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cannot combine reconcile.Result{RequeueAfter: statusUpdateRequeueAfter} with non-nil error. See controller-runtime code controller-runtime/pkg/internal/controller/controller.go line 467

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
err = r.StatusWriter.UpdateOwnedConditions(ctx, instance)
if err != nil {
klog.Errorf("failed to update performance profile %q status: %v", instance.GetName(), err)
return ctrl.Result{RequeueAfter: statusUpdateRequeueAfter}, nil
}
return ctrl.Result{}, nil
}
Expand Down Expand Up @@ -651,7 +655,7 @@ func (r *PerformanceProfileReconciler) getAndValidateMCP(ctx context.Context, in
conditions := status.GetDegradedConditions(status.ConditionFailedToFindMachineConfigPool, err.Error())
if err := r.StatusWriter.Update(ctx, profile, conditions); err != nil {
klog.Errorf("failed to update performance profile %q status: %v", profile.GetName(), err)
return nil, &reconcile.Result{}, err
return nil, &reconcile.Result{RequeueAfter: statusUpdateRequeueAfter}, nil
}
return nil, &reconcile.Result{}, nil
}
Expand All @@ -660,7 +664,7 @@ func (r *PerformanceProfileReconciler) getAndValidateMCP(ctx context.Context, in
conditions := status.GetDegradedConditions(status.ConditionBadMachineConfigLabels, err.Error())
if err := r.StatusWriter.Update(ctx, profile, conditions); err != nil {
klog.Errorf("failed to update performance profile %q status: %v", profile.GetName(), err)
return nil, &reconcile.Result{}, err
return nil, &reconcile.Result{RequeueAfter: statusUpdateRequeueAfter}, nil
}
return nil, &reconcile.Result{}, nil
}
Expand Down