CNTRLPLANE-400: feat(remediationAllowed in NP): propagate MHC RemediationAllowed to NodePool Ready condition - #9019
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
do-not-merge/needs-area
area/hypershift-operator
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMachineHealthCheck reconciliation now annotates standard and spot MHCs with their parent NodePool. NodePool reconciliation maps a false Sequence Diagram(s)sequenceDiagram
participant MachineHealthCheck
participant NodePoolController
participant NodePool
MachineHealthCheck->>NodePoolController: update RemediationAllowed
NodePoolController->>MachineHealthCheck: inspect condition
MachineHealthCheck-->>NodePoolController: return status, reason, message
NodePoolController->>NodePool: set Ready=False with MHC details
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
🧹 Nitpick comments (1)
hypershift-operator/controllers/nodepool/capi_test.go (1)
3829-3846: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExercise
CAPI.Reconcileinstead of duplicating its new branch.This reproduces the production condition logic, so it can pass if
CAPI.Reconcilestops invoking that logic. Drive the MHC status through the existing reconciliation fixture and assert the resulting NodePool condition.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/nodepool/capi_test.go` around lines 3829 - 3846, Replace the duplicated condition-handling block in the test with an invocation of CAPI.Reconcile using the existing reconciliation fixture, configuring the fixture’s MachineHealthCheck status with tt.mhcConditions. Assert the resulting NodePool condition produced by reconciliation, preserving coverage of the remediation-allowed behavior without directly calling findMHCRemediationAllowedCondition or SetStatusCondition. Add or update unit assertions for the reconciled outcomes.Source: Coding guidelines
🤖 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/nodepool/capi.go`:
- Around line 1458-1466: The MHC condition change detection only compares
Status, so same-status Reason or Message updates do not trigger reconciliation.
In hypershift-operator/controllers/nodepool/capi.go lines 1458-1466, update the
comparison in the findMHCRemediationAllowedCondition flow to also detect Reason
and Message changes. In hypershift-operator/controllers/nodepool/capi_test.go
lines 3895-3898, add a test case with unchanged Status but changed Reason and/or
Message that expects reconciliation.
---
Nitpick comments:
In `@hypershift-operator/controllers/nodepool/capi_test.go`:
- Around line 3829-3846: Replace the duplicated condition-handling block in the
test with an invocation of CAPI.Reconcile using the existing reconciliation
fixture, configuring the fixture’s MachineHealthCheck status with
tt.mhcConditions. Assert the resulting NodePool condition produced by
reconciliation, preserving coverage of the remediation-allowed behavior without
directly calling findMHCRemediationAllowedCondition or SetStatusCondition. Add
or update unit assertions for the reconciled outcomes.
🪄 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: fbc73ca4-b40a-4d1d-8510-f6bd6114cd38
📒 Files selected for processing (3)
hypershift-operator/controllers/nodepool/capi.gohypershift-operator/controllers/nodepool/capi_test.gohypershift-operator/controllers/nodepool/nodepool_controller.go
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
5a73424 to
bb23dd6
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9019 +/- ##
==========================================
+ Coverage 44.51% 44.54% +0.02%
==========================================
Files 774 774
Lines 96997 97053 +56
==========================================
+ Hits 43179 43232 +53
Misses 50830 50830
- Partials 2988 2991 +3
... and 1 file 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
sdminonne
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@sdminonne: This pull request references CNTRLPLANE-400 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set. 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. |
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped some comments. Thanks!
|
|
||
| // When MHC RemediationAllowed is False, override the Ready condition to signal | ||
| // that auto-repair is blocked because too many machines are unhealthy. | ||
| if remediationAllowed := findMHCRemediationAllowedCondition(mhc.Status.Conditions); remediationAllowed != nil { |
There was a problem hiding this comment.
Question: Should this be a dedicated condition type instead of overriding Ready?
Today Ready is documented as "bubbles up CAPI MachineDeployment/MachineSet Ready condition — true when all replicas are ready Nodes" (nodepool_conditions.go:50-53). After this change, Ready=False could also mean "nodes are fine but the MHC circuit breaker tripped." An operator seeing Ready=False, Reason=TooManyUnhealthy can't easily distinguish an infrastructure failure from a remediation threshold breach.
The existing pattern is one condition per signal: AutorepairEnabled, AllMachinesReady, AllNodesHealthy, UpdatingVersion. Something like RemediationPaused would follow that pattern and keep Ready's contract intact.
If overriding Ready is the intended approach, the doc comment at nodepool_conditions.go:50 should be updated to reflect this new source.
Wdyt @sdminonne?
There was a problem hiding this comment.
If we go for a separate condition, please not that there are two separate conditions on MachineHealthCheck:
RemediationAllowed and Paused. I would probably choose MachineRemediationAllowed because it's clear it's related to machines and reflects the right condition RemediationAllowed. Using RemediationPaused could confuse this with the other condition "Paused".
There was a problem hiding this comment.
I think that I took the override-readyCondition path 'cause @enxebre pushed for that path in some slack chat (but I cannot find it anymore so let's take it's my choice (and fault).
Overriding 'Ready' is intentional: When the MHC circuit breaker fires, the NodePool is notReady. Not ready nodes won't be replaced and the pool is degraded so I would say notReady it's the right semantic.
I knot that we're adding other signals to Ready but this is already the case since it already aggregates multiple signals: it bubbles up MachineDeployment/MachineSet readiness, which itself reflects infrastructure provider status, node readiness, and replica counts. Adding one more signal source (MHC circuit breaker) is just another one: same pattern.
The existing per-signal conditions (AutorepairEnabled, AllMachinesReady, AllNodesHealthy) exist to provide drill-down detail when Ready is false — they don't replace Ready. An operator seeing Ready=F alse, Reason=TooManyUnhealthy gets an immediately actionable signal; they can then inspect AllMachinesReady and AllNodesHealthy for details.
But the doc in nodepool_conditions.go is updated as requested by @jparrill
| // When MHC RemediationAllowed is False, override the Ready condition to signal | ||
| // that auto-repair is blocked because too many machines are unhealthy. | ||
| if remediationAllowed := findMHCRemediationAllowedCondition(mhc.Status.Conditions); remediationAllowed != nil { | ||
| if remediationAllowed.Status != corev1.ConditionTrue { |
There was a problem hiding this comment.
Nit: ConditionUnknown also satisfies != ConditionTrue and would override Ready to False. Is that intentional? If so, worth adding a test case for it. If Unknown should be treated as "we don't know yet, leave Ready alone," you'd need == ConditionFalse instead.
| if mhc.Annotations == nil { | ||
| mhc.Annotations = map[string]string{} | ||
| } | ||
| mhc.Annotations[nodePoolAnnotation] = client.ObjectKeyFromObject(nodePool).String() |
There was a problem hiding this comment.
Bug: reconcileSpotMachineHealthCheck (line ~1180) does not set nodePoolAnnotation. The new MHC watch uses enqueueParentNodePool which depends on this annotation to map events back to the parent NodePool — spot MHC events that pass the predicate will be silently dropped.
Even if MaxUnhealthy=100% makes RemediationAllowed=False unlikely for spot, every other CAPI child resource (MachineDeployment, MachineSet, Machine, MachineTemplate) carries this annotation. The spot MHC should too.
| // NodePool reconciliations from unrelated MHC status field changes (e.g. CurrentHealthy, | ||
| // Targets) that are already covered by MachineDeployment/MachineSet/Machine watches. | ||
| func mhcRemediationAllowedChangedPredicate() predicate.Funcs { | ||
| return predicate.Funcs{ |
There was a problem hiding this comment.
Nit: The comment says "Create and Delete events always pass through" but that's implicit (unset predicate.Funcs fields default to true). Two thoughts:
- Consider making it explicit with
CreateFunc/DeleteFuncreturningtruefor readability. - The controller itself creates the MHC, so every Create event triggers an immediate redundant re-reconciliation. A
CreateFuncreturningfalsewould avoid that wasted cycle.
There was a problem hiding this comment.
I"m not totally positive about this but here is my claude answer:
Suppressing Create events is premature optimization. The controller itself creates the MHC, so the "wasted" Create reconciliation is a single no-op pass through an already-idempotent reconciler. The cost is negligible — one extra reconcile per MHC creation (which happens once per NodePool lifecycle). Suppressing it would add code to save microseconds, and would make the predicate less predictable if the watch is ever reused in a context where Create events matter.
I'm not fixing this
| }, | ||
| } | ||
|
|
||
| if remediationAllowed := findMHCRemediationAllowedCondition(mhc.Status.Conditions); remediationAllowed != nil { |
There was a problem hiding this comment.
Important: This test copies the production logic from Reconcile() verbatim instead of calling the actual method. If someone refactors Reconcile() and breaks the ordering or conditional branches, this test still passes on its own copy.
Consider calling CAPI.Reconcile() with a fake client containing an MHC with RemediationAllowed=False and asserting on the resulting NodePool conditions — same pattern as TestCAPIReconcile at line 1317.
| } | ||
| } | ||
|
|
||
| func TestMHCRemediationAllowedChangedPredicate(t *testing.T) { |
There was a problem hiding this comment.
Nit: The 8 Update cases are thorough — nice coverage. Two optional additions:
- A test confirming Create events pass through (the predicate comment says they do, but it's implicit behavior from
predicate.Funcsdefaults). - A test with a non-MHC object type hitting the
!okguard clause.
There was a problem hiding this comment.
The 8 existing Update test cases cover the actual filtering logic where bugs can live. Testing that predicate.Funcs defaults to true for Create/Delete events is testing the controller-runtime framework, not our code. The !ok type assertion guard clause is defensive code that cannot be reached with a properly typed watch registration — controller-runtime guarantees the object type matches the watch source. Testing unreachable code paths adds maintenance cost without catching real bugs.
Pushing back on this
bb23dd6 to
8ebd740
Compare
openshift-ci
Bot
added
the
area/api
8ebd740 to
bbb77de
Compare
openshift-ci
Bot
added
the
area/control-plane-operator
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
`@control-plane-operator/controllers/hostedcontrolplane/v2/assets/assets_test.go`:
- Around line 56-58: Wrap the body of TestKonnectivityServerAuthenticatesAgents
in a subtest whose name follows the required “When ... it should ...” format,
specifically describing the configured cluster CA flag and agent authentication
behavior. Preserve the existing test setup and assertions inside that subtest.
🪄 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: d7cb6007-c541-4242-8e83-547ca46818d5
⛔ Files ignored due to path filters (7)
control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/AROSwift/zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/GCP/zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/IBMCloud/zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/ModernTLS/zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yamlis excluded by!**/testdata/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_conditions.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (6)
api/hypershift/v1beta1/nodepool_conditions.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/assets_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/kube-apiserver/deployment.yamlhypershift-operator/controllers/nodepool/capi.gohypershift-operator/controllers/nodepool/capi_test.gohypershift-operator/controllers/nodepool/nodepool_controller.go
🚧 Files skipped from review as they are similar to previous changes (3)
- hypershift-operator/controllers/nodepool/nodepool_controller.go
- hypershift-operator/controllers/nodepool/capi.go
- hypershift-operator/controllers/nodepool/capi_test.go
| func TestKonnectivityServerAuthenticatesAgents(t *testing.T) { | ||
| t.Parallel() | ||
| g := NewWithT(t) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a guideline-compliant test description.
Wrap the test body in a subtest named like “When the cluster CA flag is configured, it should authenticate agents” to follow the repository’s required test-case format.
As per coding guidelines, unit-test cases must use the “When ... it should ...” format.
🤖 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
`@control-plane-operator/controllers/hostedcontrolplane/v2/assets/assets_test.go`
around lines 56 - 58, Wrap the body of TestKonnectivityServerAuthenticatesAgents
in a subtest whose name follows the required “When ... it should ...” format,
specifically describing the configured cluster CA flag and agent authentication
behavior. Preserve the existing test setup and assertions inside that subtest.
Source: Coding guidelines
|
/rebase |
|
🤖 Rebasing PR onto main: workflow run |
0ee7e5a to
6cfaad0
Compare
openshift-ci-robot
removed
the
verified
|
/verified by @sdminonne in dev cluster |
openshift-ci-robot
added
the
verified
|
@sdminonne: 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. |
|
Simply rebased. Now #9095 got merged and unit tests shouldn't race anymore |
|
/pipeline required |
|
Scheduling tests matching the |
|
/retest-required |
2 similar comments
|
/retest-required |
|
/retest-required |
sdminonne
changed the title
|
/retest-required |
|
/lgtm |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/retest-require |
|
The gke failure depends on #9099 because the the osImageStream changed for 5.0 There is a related slack thread about this known issue. Some testing is ongoing |
|
/retest-required |
|
/approve |
|
/label acknowledge-critical-fixes-only |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: csrwng, jparrill, sdminonne 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
|
/test images |
|
@sdminonne: 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. |
6 participants
Summary
RemediationAllowedcondition into the NodePoolReadycondition. WhenRemediationAllowed=False(too many unhealthy machines exceedmaxUnhealthy), the NodePoolReadycondition is overridden toFalsewith reasonTooManyUnhealthyand the MHC's original message preserved.RemediationAllowedcondition changes (Status, Reason, or Message) trigger NodePool reconciliation, avoiding unnecessary reconciliations from unrelated MHC status field updates.nodePoolAnnotationon the MHC so the existingenqueueParentNodePoolwatch handler can map MHC events back to the parent NodePool.Test plan
TestMHCRemediationAllowedBubbledUpToReady— verifies Ready is overridden when RemediationAllowed=False, left untouched when True or absentTestMHCRemediationAllowedChangedPredicate— verifies predicate filters Update events correctly (8 cases: Status changes, Reason-only change, Message-only change, condition appearing/disappearing, no-change cases)TestReconcileMachineHealthCheckAnnotation— verifies nodePoolAnnotation is set on MHCmake verifypasses (lint, gitlint, CRD schema check)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation