OCPBUGS-100301: fix(hostedcluster): handle Unknown status in ClusterVersionFailing inversion - #9186
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@reedcort: This pull request references Jira Issue OCPBUGS-100301, 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. |
openshift-ci-robot
added
the
jira/invalid-bug
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughHostedCluster condition inversion now explicitly maps unrecognized CVO statuses to Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/hypershift-operator
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@reedcort: This pull request references Jira Issue OCPBUGS-100301, which is valid. The bug has been moved to the POST state. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go`:
- Around line 4046-4077: Replace the duplicated condition-map and
status-inversion logic in the test with coverage of the production
reconciliation behavior: either extract a shared helper used by both reconcile
and reconcileLegacy and test that helper, or invoke each reconciler with an
appropriate fake client. Ensure the tests exercise missing-condition fallbacks
and inversion outcomes, and add or update unit tests for the production code
changes.
- Around line 4016-4020: Add a table-driven test case alongside the existing
ClusterVersionFailing status cases using metav1.ConditionStatus("Unexpected") as
hcpStatus and metav1.ConditionUnknown as expectedStatus, covering fallback
statuses beyond the named ConditionUnknown constant.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ee96441a-2b5f-40c1-994d-05a9bcaa2130
📒 Files selected for processing (3)
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.gohypershift-operator/controllers/hostedcluster/reconcile_legacy.go
| { | ||
| name: "When ClusterVersionFailing is Unknown it should produce ClusterVersionSucceeding Unknown", | ||
| hcpStatus: metav1.ConditionUnknown, | ||
| expectedStatus: metav1.ConditionUnknown, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover unexpected statuses beyond ConditionUnknown.
The new fallback handles every status other than True and False, but the table only tests the named Unknown constant. Add a case such as metav1.ConditionStatus("Unexpected") and expect metav1.ConditionUnknown.
As per coding guidelines, unit test any code changes and additions.
🤖 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
`@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go`
around lines 4016 - 4020, Add a table-driven test case alongside the existing
ClusterVersionFailing status cases using metav1.ConditionStatus("Unexpected") as
hcpStatus and metav1.ConditionUnknown as expectedStatus, covering fallback
statuses beyond the named ConditionUnknown constant.
Source: Coding guidelines
| hcpCVOConditions := map[hyperv1.ConditionType]*metav1.Condition{ | ||
| hyperv1.ClusterVersionSucceeding: meta.FindStatusCondition(hcp.Status.Conditions, string(hyperv1.ClusterVersionFailing)), | ||
| } | ||
|
|
||
| for conditionType := range hcpCVOConditions { | ||
| hcCVOCondition := &metav1.Condition{ | ||
| Type: string(conditionType), | ||
| Status: metav1.ConditionUnknown, | ||
| Reason: hyperv1.StatusUnknownReason, | ||
| ObservedGeneration: hcluster.Generation, | ||
| } | ||
|
|
||
| if hcpCVOConditions[conditionType] != nil { | ||
| hcCVOCondition = hcpCVOConditions[conditionType] | ||
| hcCVOCondition.ObservedGeneration = hcluster.Generation | ||
|
|
||
| if conditionType == hyperv1.ClusterVersionSucceeding { | ||
| hcCVOCondition.Type = string(hyperv1.ClusterVersionSucceeding) | ||
| var status metav1.ConditionStatus | ||
| switch hcpCVOConditions[conditionType].Status { | ||
| case metav1.ConditionTrue: | ||
| status = metav1.ConditionFalse | ||
| case metav1.ConditionFalse: | ||
| status = metav1.ConditionTrue | ||
| default: | ||
| status = metav1.ConditionUnknown | ||
| } | ||
| hcCVOCondition.Status = status | ||
| } | ||
| } | ||
|
|
||
| meta.SetStatusCondition(&hcluster.Status.Conditions, *hcCVOCondition) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Exercise the production reconciliation code instead of copying its implementation.
This test reconstructs the same map and switch as production, so it can pass even if reconcile or reconcileLegacy omits or mishandles the fallback. Extract a shared inversion helper used by both paths and test that helper, or invoke each reconciler with an appropriate fake client.
As per coding guidelines, unit test any code changes and additions.
🤖 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
`@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go`
around lines 4046 - 4077, Replace the duplicated condition-map and
status-inversion logic in the test with coverage of the production
reconciliation behavior: either extract a shared helper used by both reconcile
and reconcileLegacy and test that helper, or invoke each reconciler with an
appropriate fake client. Ensure the tests exercise missing-condition fallbacks
and inversion outcomes, and add or update unit tests for the production code
changes.
Source: Coding guidelines
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9186 +/- ##
==========================================
+ Coverage 44.81% 44.95% +0.13%
==========================================
Files 775 778 +3
Lines 97219 97430 +211
==========================================
+ Hits 43572 43798 +226
+ Misses 50644 50611 -33
- Partials 3003 3021 +18
... and 26 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/lgtm |
|
Scheduling tests matching the |
Test Resultse2e-aws
e2e-aks
|
|
@reedcort: This pull request references Jira Issue OCPBUGS-100301, 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. |
| default: | ||
| status = metav1.ConditionUnknown |
There was a problem hiding this comment.
Suggestion: The identical inversion switch exists in reconcile_legacy.go:441. Consider extracting a shared helper to eliminate the duplication:
func invertConditionStatus(s metav1.ConditionStatus) metav1.ConditionStatus {
switch s {
case metav1.ConditionTrue:
return metav1.ConditionFalse
case metav1.ConditionFalse:
return metav1.ConditionTrue
default:
return metav1.ConditionUnknown
}
}The project already does this with computeClusterVersionStatus() which is shared between both paths. This would also let the test exercise the actual production function instead of re-implementing the logic inline.
| t.Run(tc.name, func(t *testing.T) { | ||
| g := NewWithT(t) |
There was a problem hiding this comment.
Suggestion: The subtests should call t.Parallel() — other subtests in this file do (e.g., TestComputeAWSDefaultSGDeletedCondition). Since these cases are independent:
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
g := NewWithT(t)| case metav1.ConditionTrue: | ||
| status = metav1.ConditionFalse | ||
| case metav1.ConditionFalse: | ||
| status = metav1.ConditionTrue | ||
| default: | ||
| status = metav1.ConditionUnknown | ||
| } | ||
| hcCVOCondition.Status = status | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: This test re-implements the inversion switch inline rather than calling reconcile() or reconcileLegacy(). If the production code is changed but this test isn't updated (or vice versa), the test will still pass while the production path is broken.
If extracting invertConditionStatus() is too much for a bug-fix PR, at minimum the test name should clarify it's testing the logic in isolation, not the code path.
| name: "When ClusterVersionFailing is Unknown it should produce ClusterVersionSucceeding Unknown", | ||
| hcpStatus: metav1.ConditionUnknown, | ||
| expectedStatus: metav1.ConditionUnknown, | ||
| }, |
There was a problem hiding this comment.
Suggestion: Consider adding a fourth test case for hcpStatus: "" (empty string). That's the exact zero-value that triggered the original production bug, and it would also hit the default branch:
{
name: "When ClusterVersionFailing has empty status it should produce ClusterVersionSucceeding Unknown",
hcpStatus: metav1.ConditionStatus(""),
expectedStatus: metav1.ConditionUnknown,
},| } | ||
| } | ||
|
|
||
| func TestCVOConditionInversion(t *testing.T) { |
There was a problem hiding this comment.
Per TESTING.md, test function names should map 1:1 to the function under test. Since this doesn't call a production function directly, the name TestCVOConditionInversion is fine as-is. But if you extract the helper, rename to TestInvertConditionStatus to match.
openshift-ci
Bot
added
the
approved
|
/approve cancel |
openshift-ci
Bot
removed
the
approved
…version The ClusterVersionFailing → ClusterVersionSucceeding condition inversion switch only handled True and False. When ClusterVersionFailing had Status: Unknown (common during control plane disruptions), the switch fell through and set ClusterVersionSucceeding.Status to "" (zero value), which the API server permanently rejects on Status().Update. This blocked the entire reconcile loop, preventing HC-to-HCP annotation sync and causing a scheduling deadlock where request-serving pods remained Pending indefinitely. Extract invertConditionStatus() helper shared by both reconcile and reconcileLegacy paths, with a default case mapping Unknown and any unexpected status to ConditionUnknown. Fixes: OCPBUGS-100301 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
be6e52b to
d9edcc4
Compare
|
Scheduling tests matching the |
csrwng
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, reedcort 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 |
|
/verified by @reedcort via unit tests |
openshift-ci-robot
added
the
verified
|
@reedcort: 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. |
|
/retest-required |
|
/test e2e-kubevirt-aws-ovn-reduced |
2 similar comments
|
/test e2e-kubevirt-aws-ovn-reduced |
|
/test e2e-kubevirt-aws-ovn-reduced |
|
@reedcort: all tests passed! 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. |
|
@reedcort: Jira Issue Verification Checks: Jira Issue OCPBUGS-100301 Jira Issue OCPBUGS-100301 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
|
Fix included in release 5.0.0-0.nightly-2026-08-04-172547 |
8 participants
What this PR does / why we need it:
The ClusterVersionFailing → ClusterVersionSucceeding condition inversion switch (hostedcluster_controller.go:755-762) only handled True and False. When ClusterVersionFailing had Status: Unknown (common during control plane disruptions like cluster-size-override), the switch fell through and set ClusterVersionSucceeding.Status to "" (zero value). The API server permanently rejects this on Status().Update, blocking the entire reconcile loop before Phase 7 (CoreHCPChain). This prevented HC-to-HCP annotation sync, causing request-serving pods to schedule with stale node affinity and remain Pending indefinitely.
This fix adds a default case that maps Unknown (and any other unexpected status) to ConditionUnknown. Applied to both reconcile and reconcileLegacy paths.
Which issue(s) this PR fixes:
Fixes OCPBUGS-100301
Special notes for your reviewer:
Two production incidents: 105 min and 44 hours downtime (ROSAENG-62684).
Root cause confirmed by operator logs showing
status.conditions[4].status: Unsupported value: ""and HCP showingClusterVersionFailing: Unknown.Bug reproduced on staging cluster by stripping ClusterVersion conditions from the guest cluster:
This caused the HCCO to set
ClusterVersionFailing: Unknownon the HCP, which immediately triggered the same error loop observed in production:Checklist: