CNTRLPLANE-3691: Decouple NodePool condition handling from CAPI reconciliation#9032
Conversation
CAPI.Reconcile() was setting NodePool conditions inline, mixing CAPI resource reconciliation with NodePool status management. This made capi.go harder to reason about and violated separation of concerns. Introduce a CAPIResult struct returned by CAPI.Reconcile() that carries condition information back to the caller. The caller in nodepool_controller.go now sets conditions on the NodePool after the CAPI reconciliation completes, centralizing condition management in the controller layer. Conditions moved: - NodePoolUpdatingPlatformMachineTemplateConditionType - NodePoolAutorepairEnabledConditionType - NodePoolReadyConditionType (from both MachineDeployment and MachineSet) No behavioral change — the same conditions are set under the same circumstances. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Capture and validate the CAPIResult returned by capi.Reconcile() in TestCAPIReconcile, TestCAPIReconcile_machineset, and fix a pre-existing test bug in TestReconcileMachineDeploymentStatus where conditions accumulated on capi.conditions were not applied to the NodePool. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-3691 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 task 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. |
📝 WalkthroughWalkthrough
Sequence Diagram(s)sequenceDiagram
participant NodePoolReconciler
participant CAPI
participant MachineResources
NodePoolReconciler->>CAPI: Reconcile(ctx)
CAPI->>MachineResources: Reconcile machines and health checks
MachineResources-->>CAPI: Status and errors
CAPI-->>NodePoolReconciler: CAPIResult with conditions
NodePoolReconciler->>NodePoolReconciler: Apply conditions to NodePool status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/hypershift-operator
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hypershift-jira-solve-ci[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 |
![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)
1884-1890: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the expected condition contract, not merely a non-empty result.
These assertions pass if only an unrelated condition is returned. Add expected condition type/status assertions per fixture, particularly for autorepair and ready-condition paths.
hypershift-operator/controllers/nodepool/capi_test.go#L1884-L1890: assert the conditions expected from the initial reconciliation.hypershift-operator/controllers/nodepool/capi_test.go#L2000-L2003: assert the expected condition after rollout completion.hypershift-operator/controllers/nodepool/capi_test.go#L2172-L2182: assert the in-place reconciliation condition set.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 1884 - 1890, Strengthen the condition assertions in hypershift-operator/controllers/nodepool/capi_test.go at lines 1884-1890, 2000-2003, and 2172-2182: use each fixture’s expected condition type and status, including autorepair, ready-condition, rollout-completion, and in-place reconciliation paths, rather than only checking that conditions are non-empty. Update the tests around the capi.Reconcile calls and add or adjust unit-test expectations for every affected scenario.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 150-151: Preserve accumulated conditions after CAPI reconciliation
begins: in CAPI.Reconcile, return the accumulated CAPIResult alongside errors on
every subsequent error path, including the MachineSet failure near
hypershift-operator/controllers/nodepool/capi.go:150-151. In
hypershift-operator/controllers/nodepool/nodepool_controller.go:429-440, apply
any non-nil CAPIResult before handling and returning the reconciliation error.
---
Nitpick comments:
In `@hypershift-operator/controllers/nodepool/capi_test.go`:
- Around line 1884-1890: Strengthen the condition assertions in
hypershift-operator/controllers/nodepool/capi_test.go at lines 1884-1890,
2000-2003, and 2172-2182: use each fixture’s expected condition type and status,
including autorepair, ready-condition, rollout-completion, and in-place
reconciliation paths, rather than only checking that conditions are non-empty.
Update the tests around the capi.Reconcile calls and add or adjust unit-test
expectations for every affected scenario.
🪄 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: 1b60cd96-8142-43b2-8179-b7be4e59fd53
📒 Files selected for processing (3)
hypershift-operator/controllers/nodepool/capi.gohypershift-operator/controllers/nodepool/capi_test.gohypershift-operator/controllers/nodepool/nodepool_controller.go
| return nil, fmt.Errorf("failed to reconcile MachineSet %q: %w", | ||
| client.ObjectKeyFromObject(ms).String(), err) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve accumulated conditions when CAPI reconciliation fails.
A condition is appended before these later failures, but CAPI.Reconcile returns nil, err; the controller therefore returns before applying it. Previously, the inline status mutation was patched even on reconcile errors, so this loses status updates such as the platform-template condition.
hypershift-operator/controllers/nodepool/capi.go#L150-L151: return the accumulatedCAPIResulton every error path reached after condition collection begins.hypershift-operator/controllers/nodepool/nodepool_controller.go#L429-L440: apply a non-nil result before handling and returning the error.
📍 Affects 2 files
hypershift-operator/controllers/nodepool/capi.go#L150-L151(this comment)hypershift-operator/controllers/nodepool/nodepool_controller.go#L429-L440
🤖 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 150 - 151,
Preserve accumulated conditions after CAPI reconciliation begins: in
CAPI.Reconcile, return the accumulated CAPIResult alongside errors on every
subsequent error path, including the MachineSet failure near
hypershift-operator/controllers/nodepool/capi.go:150-151. In
hypershift-operator/controllers/nodepool/nodepool_controller.go:429-440, apply
any non-nil CAPIResult before handling and returning the reconciliation error.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9032 +/- ##
==========================================
+ Coverage 44.11% 44.27% +0.16%
==========================================
Files 772 781 +9
Lines 96227 98553 +2326
==========================================
+ Hits 42449 43636 +1187
- Misses 50832 51867 +1035
- Partials 2946 3050 +104
... and 44 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:
|
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
Overall this is a clean, well-scoped refactor. The CAPIResult struct approach is solid and the test bug fix in TestReconcileMachineDeploymentStatus is a genuine improvement. A few observations inline.
| capiClusterName string | ||
| scaleFromZeroPlatform hyperv1.PlatformType | ||
| upsert.ApplyProvider | ||
| conditions []hyperv1.NodePoolCondition |
There was a problem hiding this comment.
nit (judgement call): The conditions field on CAPI acts as a builder for CAPIResult.Conditions — Reconcile() could build the slice locally and return it directly, avoiding the mutable struct field. Not blocking, but something to consider if this area gets touched again.
Also worth noting for a follow-up: capi.go still directly mutates nodePool.Status.Replicas (lines 655, 1055), nodePool.Status.Version (lines 635, 1033), and nodePool.Annotations (lines 641-651). The Jira spec says "capi.go focuses purely on CAPI resource reconciliation" — conditions are now decoupled, but these other status mutations remain. A follow-up issue to decouple those would complete the separation of concerns.
There was a problem hiding this comment.
Agreed on both points. Building the slice locally in Reconcile() and returning it directly would be cleaner — noted for a future pass. And yes, the remaining Status.Replicas, Status.Version, and Annotations mutations in capi.go are the next candidates for decoupling per the Jira scope.
AI-assisted response via Claude Code
| if cond := FindStatusCondition(nodePool.Status.Conditions, hyperv1.NodePoolReachedIgnitionEndpoint); cond == nil || cond.Status != corev1.ConditionTrue { | ||
| log.Info("ReachedIgnitionEndpoint is false, MachineHealthCheck won't be created until this is true") | ||
| return nil | ||
| return &CAPIResult{Conditions: c.conditions}, nil |
There was a problem hiding this comment.
Good: this early return now correctly preserves conditions accumulated before this point (specifically NodePoolUpdatingPlatformMachineTemplateConditionType). In the old code, SetStatusCondition was called inline before this return so they were already applied — the new code maintains that behavior by returning c.conditions here. Verified this is behavior-preserving.
There was a problem hiding this comment.
Thanks for verifying — that was the key invariant: conditions accumulated before the early return must still be returned to the caller.
AI-assisted response via Claude Code
| } | ||
| for _, condition := range capiResult.Conditions { | ||
| SetStatusCondition(&nodePool.Status.Conditions, condition) | ||
| } |
There was a problem hiding this comment.
The Jira spec suggested a "dedicated function" for condition-setting. This inline loop is simpler and arguably better for a 3-line operation. If the number of condition sources grows (e.g., decoupling Status.Replicas and Status.Version from capi.go too), this could graduate to a applyCAPIConditions(nodePool, capiResult) helper.
There was a problem hiding this comment.
Agreed — the inline loop is proportional to the current scope. If additional condition sources land (e.g., decoupled Replicas/Version), graduating to an applyCAPIConditions helper makes sense.
AI-assisted response via Claude Code
|
|
||
| capi.reconcileMachineDeploymentStatus(logr.Discard(), tc.machineDeployment, templateCR) | ||
| for _, cond := range capi.conditions { | ||
| SetStatusCondition(&nodePool.Status.Conditions, cond) |
There was a problem hiding this comment.
Good catch — this fixes a pre-existing test bug. Before this change, reconcileMachineDeploymentStatus accumulated conditions on capi.conditions but nothing applied them to nodePool.Status.Conditions, so the Ready condition assertions below were passing vacuously against stale state.
There was a problem hiding this comment.
Thanks — the test was indeed asserting against stale state before this fix.
AI-assisted response via Claude Code
| @@ -2344,7 +2351,7 @@ func TestGlobalPSManagedLabelOnMachines(t *testing.T) { | |||
| }, | |||
There was a problem hiding this comment.
nit: This &CAPIResult{Conditions: capi.conditions} construction is duplicated in the KubeVirt test case below (~line 2500). Both test helpers reach into capi.conditions directly, coupling the test to the internal accumulation mechanism. Minor — this is test scaffolding and the duplication is tolerable.
There was a problem hiding this comment.
Acknowledged — the duplication is intentional scaffolding for now. If this area evolves, extracting a buildCAPIResult(capi) test helper would clean it up.
AI-assisted response via Claude Code
|
Unit test coverage is not passing codecov/patch either. |
Add test cases for previously uncovered code paths: - ReachedIgnitionEndpoint not set with AutoRepair enabled (early return) - MachineSet ReadyCondition propagation to CAPIResult - Validate specific condition types (UpdatingPlatformMachineTemplate, AutorepairEnabled) in existing test assertions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Done. Added test coverage in 9e1ffa7 for previously uncovered code paths:
Note: the AI-assisted response via Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
hypershift-operator/controllers/nodepool/capi_test.go (1)
2038-2046: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider asserting the actual condition Status, not just presence.
This check only confirms
NodePoolUpdatingPlatformMachineTemplateConditionTypeexists incapiResult.Conditions, unlike the sibling Autorepair checks in the same block which assert.Statusexplicitly. Strengthening this to also verify the expected True/False value (based on whether the template actually changed) would make the test meaningfully stronger against regressions in that branch's logic.🤖 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 2038 - 2046, The CAPIResult test only checks for the UpdatingPlatformMachineTemplate condition’s presence. In the assertion around foundUpdatingCondition, locate the matching condition and assert its Status matches the expected True/False value for whether the platform machine template changed, following the sibling Autorepair checks while preserving the existing presence validation.
🤖 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_test.go`:
- Around line 2071-2086: Update the MHC lookup in the autorepair-disabled
assertion to use the existing controlpaneNamespace variable instead of the
hardcoded "test-cp-namespace" value. Keep the name and IsNotFound validation
unchanged so the test verifies deletion in the actual control-plane namespace.
---
Nitpick comments:
In `@hypershift-operator/controllers/nodepool/capi_test.go`:
- Around line 2038-2046: The CAPIResult test only checks for the
UpdatingPlatformMachineTemplate condition’s presence. In the assertion around
foundUpdatingCondition, locate the matching condition and assert its Status
matches the expected True/False value for whether the platform machine template
changed, following the sibling Autorepair checks while preserving the existing
presence validation.
🪄 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: 7fe1bbcf-57f6-4c26-97b0-64f3fade88b3
📒 Files selected for processing (1)
hypershift-operator/controllers/nodepool/capi_test.go
| } else { | ||
| mhc := &capiv1.MachineHealthCheck{} | ||
| err = capi.Client.Get(t.Context(), client.ObjectKey{Namespace: "test-cp-namespace", Name: tt.nodePool.GetName()}, mhc) | ||
| g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) | ||
|
|
||
| // Validate autorepair disabled condition is present. | ||
| foundAutorepair := false | ||
| for _, cond := range capiResult.Conditions { | ||
| if cond.Type == hyperv1.NodePoolAutorepairEnabledConditionType { | ||
| g.Expect(cond.Status).To(Equal(corev1.ConditionFalse)) | ||
| foundAutorepair = true | ||
| break | ||
| } | ||
| } | ||
| g.Expect(foundAutorepair).To(BeTrue(), "expected AutorepairEnabled=False condition in CAPIResult") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Hardcoded namespace makes this assertion vacuous.
Line 2073 checks MHC absence in the literal namespace "test-cp-namespace" instead of controlpaneNamespace (used consistently elsewhere, e.g. lines 2052, 2069). Since no object is ever created under that literal string in this fake client, IsNotFound is trivially true regardless of whether the AutoRepair=false MHC-deletion logic actually works against the real control-plane namespace — a regression here wouldn't be caught.
🐛 Proposed fix
} else {
mhc := &capiv1.MachineHealthCheck{}
- err = capi.Client.Get(t.Context(), client.ObjectKey{Namespace: "test-cp-namespace", Name: tt.nodePool.GetName()}, mhc)
+ err = capi.Client.Get(t.Context(), client.ObjectKey{Namespace: controlpaneNamespace, Name: tt.nodePool.GetName()}, mhc)
g.Expect(apierrors.IsNotFound(err)).To(BeTrue())📝 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.
| } else { | |
| mhc := &capiv1.MachineHealthCheck{} | |
| err = capi.Client.Get(t.Context(), client.ObjectKey{Namespace: "test-cp-namespace", Name: tt.nodePool.GetName()}, mhc) | |
| g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) | |
| // Validate autorepair disabled condition is present. | |
| foundAutorepair := false | |
| for _, cond := range capiResult.Conditions { | |
| if cond.Type == hyperv1.NodePoolAutorepairEnabledConditionType { | |
| g.Expect(cond.Status).To(Equal(corev1.ConditionFalse)) | |
| foundAutorepair = true | |
| break | |
| } | |
| } | |
| g.Expect(foundAutorepair).To(BeTrue(), "expected AutorepairEnabled=False condition in CAPIResult") | |
| } | |
| } else { | |
| mhc := &capiv1.MachineHealthCheck{} | |
| err = capi.Client.Get(t.Context(), client.ObjectKey{Namespace: controlpaneNamespace, Name: tt.nodePool.GetName()}, mhc) | |
| g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) | |
| // Validate autorepair disabled condition is present. | |
| foundAutorepair := false | |
| for _, cond := range capiResult.Conditions { | |
| if cond.Type == hyperv1.NodePoolAutorepairEnabledConditionType { | |
| g.Expect(cond.Status).To(Equal(corev1.ConditionFalse)) | |
| foundAutorepair = true | |
| break | |
| } | |
| } | |
| g.Expect(foundAutorepair).To(BeTrue(), "expected AutorepairEnabled=False condition in CAPIResult") | |
| } |
🤖 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 2071 -
2086, Update the MHC lookup in the autorepair-disabled assertion to use the
existing controlpaneNamespace variable instead of the hardcoded
"test-cp-namespace" value. Keep the name and IsNotFound validation unchanged so
the test verifies deletion in the actual control-plane namespace.
|
/test address-review-comments |
|
Review agent triggered. View job |
|
@hypershift-jira-solve-ci[bot]: 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. |
2 participants
What this PR does / why we need it:
Decouples NodePool condition management from
CAPI.Reconcile()by introducing aCAPIResultstruct that carries condition information back to the caller. Previously,CAPI.Reconcile()was setting NodePool conditions inline, mixing CAPI resource reconciliation with NodePool status management. This madecapi.goharder to reason about and violated separation of concerns.The caller in
nodepool_controller.gonow sets conditions on the NodePool after the CAPI reconciliation completes, centralizing condition management in the controller layer.Conditions moved:
NodePoolUpdatingPlatformMachineTemplateConditionTypeNodePoolAutorepairEnabledConditionTypeNodePoolReadyConditionType(from both MachineDeployment and MachineSet)No behavioral change — the same conditions are set under the same circumstances.
Tests are updated to capture and validate the
CAPIResultreturned bycapi.Reconcile(), and a pre-existing test bug is fixed where conditions accumulated oncapi.conditionswere not applied to the NodePool.Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/CNTRLPLANE-3691
Special notes for your reviewer:
This is a pure refactor with no behavioral change. The diff is large because condition-setting logic moves from
capi.gotonodepool_controller.go, but the conditions themselves and when they are set remain identical.Checklist:
Always review AI generated responses prior to use.
Generated with Claude Code via openshift-developer plugin
Summary by CodeRabbit