CNTRLPLANE-400: feat(remediationAlloowed in NP): propagate MHC RemediationAllowed to NodePool Ready condition#9019
CNTRLPLANE-400: feat(remediationAlloowed in NP): propagate MHC RemediationAllowed to NodePool Ready condition#9019sdminonne wants to merge 1 commit into
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
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sdminonne 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 |
📝 WalkthroughWalkthroughThe nodepool controller annotates MachineHealthChecks with their parent NodePool key and filters MHC updates to relevant Sequence Diagram(s)sequenceDiagram
participant MachineHealthCheck
participant NodePoolController
participant CAPIReconcile
participant NodePool
MachineHealthCheck->>NodePoolController: Change RemediationAllowed
NodePoolController->>CAPIReconcile: Enqueue associated NodePool
CAPIReconcile->>NodePool: Set Ready false with reason and message
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
| oldCond := findMHCRemediationAllowedCondition(oldMHC.Status.Conditions) | ||
| newCond := findMHCRemediationAllowedCondition(newMHC.Status.Conditions) | ||
| if oldCond == nil && newCond == nil { | ||
| return false | ||
| } | ||
| if oldCond == nil || newCond == nil { | ||
| return true | ||
| } | ||
| return oldCond.Status != newCond.Status |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reconcile when the blocked-remediation reason or message changes.
A RemediationAllowed=False update with unchanged status but a new reason/message is filtered out, leaving the NodePool Ready condition stale.
hypershift-operator/controllers/nodepool/capi.go#L1458-L1466: compareReasonandMessagein addition toStatus.hypershift-operator/controllers/nodepool/capi_test.go#L3895-L3898: add a same-status reason/message-change case that expects reconciliation.
Proposed fix
- return oldCond.Status != newCond.Status
+ return oldCond.Status != newCond.Status ||
+ oldCond.Reason != newCond.Reason ||
+ oldCond.Message != newCond.MessageThe PR objective explicitly requires preserving the MHC message. As per coding guidelines, unit test code changes and additions.
📝 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.
| oldCond := findMHCRemediationAllowedCondition(oldMHC.Status.Conditions) | |
| newCond := findMHCRemediationAllowedCondition(newMHC.Status.Conditions) | |
| if oldCond == nil && newCond == nil { | |
| return false | |
| } | |
| if oldCond == nil || newCond == nil { | |
| return true | |
| } | |
| return oldCond.Status != newCond.Status | |
| oldCond := findMHCRemediationAllowedCondition(oldMHC.Status.Conditions) | |
| newCond := findMHCRemediationAllowedCondition(newMHC.Status.Conditions) | |
| if oldCond == nil && newCond == nil { | |
| return false | |
| } | |
| if oldCond == nil || newCond == nil { | |
| return true | |
| } | |
| return oldCond.Status != newCond.Status || | |
| oldCond.Reason != newCond.Reason || | |
| oldCond.Message != newCond.Message |
📍 Affects 2 files
hypershift-operator/controllers/nodepool/capi.go#L1458-L1466(this comment)hypershift-operator/controllers/nodepool/capi_test.go#L3895-L3898
🤖 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.go` around lines 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.
Source: Coding guidelines
There was a problem hiding this comment.
Fixed in the latest force-push. The predicate now also compares Reason and Message:
return oldCond.Status != newCond.Status ||
oldCond.Reason != newCond.Reason ||
oldCond.Message != newCond.MessageAdded two new test cases covering Reason-only and Message-only changes (8 total).
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
…odePool status Bubble up the CAPI MachineHealthCheck RemediationAllowed condition into the NodePool Ready condition. When more machines are unhealthy than maxUnhealthy allows, CAPI sets RemediationAllowed=False with reason TooManyUnhealthy and stops remediating. This state was previously only visible by inspecting the MHC object directly in the control plane namespace. Now, when RemediationAllowed=False, the NodePool Ready condition is overridden to False with the MHC's original reason and message preserved. When remediation is allowed or the condition is absent, Ready is left as set by MachineDeployment/MachineSet reconciliation. - Set nodePoolAnnotation on the MHC so the existing enqueueParentNodePool watch handler can map it back to its parent NodePool. - After MHC create/update, read its RemediationAllowed condition and override Ready=False when remediation is blocked, preserving the MHC's reason and message. - Add a MachineHealthCheck watch in SetupWithManager so MHC status changes trigger NodePool reconciliation. - Add a predicate filter on the MHC watch so only RemediationAllowed condition changes (Status, Reason, or Message) trigger reconciliation, avoiding unnecessary reconciliations from unrelated MHC status field updates. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
5a73424 to
bb23dd6
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9019 +/- ##
==========================================
+ Coverage 44.15% 44.16% +0.01%
==========================================
Files 772 772
Lines 96260 96307 +47
==========================================
+ Hits 42503 42537 +34
- Misses 50816 50825 +9
- Partials 2941 2945 +4
... 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: 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. |
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. |
2 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
New Features
Bug Fixes