OCPBUGS-121388: Make console status flush cancellation-aware - #1228
redhat-chai-bot wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
/label reliability AI-generated. Review for accuracy. |
|
🗣️ We’d really appreciate your feedback here AI-generated. Review for accuracy. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (17)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details🧰 Additional context used📓 Path-based instructions (3)Injection prevention (prodsec-skills): SQL: parameterized queries only; no string concatenation Command: no shell=True, os.system, or backtick exec with user input LDAP/XPath: escape special characters in filters Path traversal: canonicaliz...⚙️ CodeRabbit configuration file Files:
Review test code for quality and patterns.⚙️ CodeRabbit configuration file Files:
Review Go code following OpenShift operator patterns.⚙️ CodeRabbit configuration file Files:
🔇 Additional comments (17)
Walkthrough
ChangesStatus context propagation
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested reviewers: Merge Risk: ⚪ Minimal · up to Canceled reconciliations now avoid persisting transient Degraded status while genuine active status-update failures retain their existing behavior. No current merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
reliability
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: redhat-chai-bot 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 |
Failure-job linksThese public Prow runs carry the console Degraded assertion and cancellation signature associated with this change:
The recurring discriminator is AI-generated. Review for accuracy. |
stbenjam
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@redhat-chai-bot: This pull request references Jira Issue OCPBUGS-121388, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
@redhat-chai-bot: This pull request references Jira Issue OCPBUGS-121388, which is valid. 3 validation(s) were run on this bug
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. |
| func (c *StatusHandler) FlushAndReturn(ctx context.Context, returnErr error) error { | ||
| if ctxErr := ctx.Err(); ctxErr != nil { | ||
| if returnErr != nil { | ||
| return returnErr |
There was a problem hiding this comment.
I think graceful-shutdown might be more complex, e.g. see the cluster-version operator, where we set up multiple Context instances, use postMainContext for the leader lease, runContext for the main controller, and have a 2m grace period to perform cleanup tasks like a final status sync before releasing the leader lease.
There was a problem hiding this comment.
Thanks — agreed that a full graceful-shutdown design with separate run, cleanup, and lease contexts plus a final status sync would be broader, similar to the CVO pattern.
This PR is intentionally narrower: it passes library-go’s worker reconciliation context into FlushAndReturn, so a reconciliation invalidated by controller shutdown cannot persist its cancellation as a real Degraded condition. It does not change cleanup or leader-lease ownership, and genuine status-update errors still persist while the reconciliation context is active.
Console-operator currently has no final-sync/cleanup context or delayed lease-release orchestration. Adding those semantics would require a separate startup/lifecycle change, so I’d treat that as follow-up work rather than a prerequisite for this cancellation-specific fix. If you consider the broader lifecycle behavior necessary for this PR, please let me know.
AI-generated. Review for accuracy.
There was a problem hiding this comment.
This PR is intentionally narrower: it passes library-go’s worker reconciliation context into
FlushAndReturn, so a reconciliation invalidated by controller shutdown cannot persist its cancellation as a realDegradedcondition.
So what happens instead if the context cancelation happens while the controllers have already found a real issue? Is that real-issue persisted through into the Kube API status property? Or is it silently discarded, because the new logic notices that the single context has been canceled, and because there's only a single context, no attempt is made to persist that real issue into status?
There was a problem hiding this comment.
Yes. If the reconciliation has found a genuine issue but its context is canceled before the status update commits, this code does not persist that issue. It returns the genuine reconciliation error, so it is logged, but the shutting-down worker will not retry it; the new leader must rediscover it. If the update completed before cancellation, the status remains persisted.
With an active context, genuine issues are persisted normally, and an ordinary status-update failure is returned. The tests cover both the canceled genuine-error path and the active-context Degraded path.
This is the PR’s explicit policy: do not initiate or continue status persistence from a canceled reconciliation. Guaranteeing a final status write during shutdown would require a separate bounded cleanup context and delayed lease release, like CVO, rather than bypassing cancellation in this helper.
AI-generated. Review for accuracy.
|
/pipeline-required |
|
/pipeline required |
|
Scheduling required tests: |
CI failure triageThe newly scheduled required jobs failed for infrastructure/test-environment reasons rather than the console status-flush change:
The focused unit, verification, dependency, and image checks for this change remain passing. No source change was made in response to these unrelated infrastructure failures. AI-generated. Review for accuracy. |
|
/retest |
|
@redhat-chai-bot: The following test 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. |
Follow-up CI triage: AWS console e2eThe latest AWS console e2e run failed again. The indexed prior run for this PR shows the same Playwright authentication setup failures ( Historical analysis shows this job has failed 11 consecutive times since September 2 across multiple unrelated PRs (#1217, #1218, #1227, and #1228), so this is a systemic test-infrastructure/job issue rather than a regression from commit AI-generated. Review for accuracy. |
4 participants
This pull request was generated by the Product Reliability Agent. To help us improve product stability and CI, please focus review on functional correctness and material concerns. The agent will automatically address feedback, but we’d appreciate keeping non-blocking nits from delaying an otherwise correct PR. Within two business days, please merge, provide blocking feedback, or close the PR with a reason.
Why
During an upgrade or leader handoff, shutdown can cancel reconciliation that is still running in the outgoing console-operator leader. Before this change, status flushing outlived that reconciliation context, so an expected
context canceledresult from an in-flight configuration read could be persisted as a realDegradedcondition. A replacement leader could then publish the stale condition until its first successful synchronization cleared it. This made a healthy upgrade appear to have a console degradation and caused upgrade verification to reject otherwise healthy runs.This change makes status flushing honor the active reconciliation context: cancellation is propagated and is not persisted as a product degradation, while genuine errors from an active context retain their normal handling.
Failure evidence
The following public Prow runs carry the console Degraded assertion and cancellation signature associated with this issue. Direct Prow links for these runs are posted in the PR discussion.
The Azure run from 2026-08-20 has the strongest direct causal audit trail. The other listed runs share the same test and event signature; the GCP run independently contains the same cancellation signature, while the exact leader-handoff sequence is inferred from the audited case.
The recurring discriminator is
[Monitor:legacy-cvo-invariants][bz-Management Console] clusteroperator/console should not change condition/Degraded, with reasonConfigMapSync_FailedGetOLMConfig, message identifierConfigMapSyncDegraded, and anolmconfigs/clusterread ending incontext canceled. One FIPS run also showed the corresponding custom-route cleanup variant.Changes
Validation
context.Canceled.GOFLAGS=-mod=vendor go test ./pkg/console/statusmake verifymake check(unit tests, formatting, and vet)git diff --checkReview
A dedicated adversarial review independently reproduced and then verified the cancellation edge case. The final review disposition was APPROVE with no actionable or blocking findings.