Skip to content

CNTRLPLANE-3691: Decouple NodePool condition handling from CAPI reconciliation #9032

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hypershift-jira-solve-ci wants to merge 3 commits into
base: main
Choose a base branch
from
Open

CNTRLPLANE-3691: Decouple NodePool condition handling from CAPI reconciliation #9032

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
61f76ab
refactor(nodepool): decouple condition handling from capi.go
Jul 16, 2026
159b233
fix(nodepool): validate CAPIResult conditions in tests
Jul 17, 2026
9e1ffa7
test(nodepool): improve CAPIResult condition coverage
Jul 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 35 additions & 29 deletions hypershift-operator/controllers/nodepool/capi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ const (
globalPSNodeLabel = "hypershift.openshift.io/nodepool-globalps-enabled"
)

// CAPIResult holds condition information computed during CAPI reconciliation.
// The caller is responsible for setting these on the NodePool status.
type CAPIResult struct {
Conditions []hyperv1.NodePoolCondition
}

// CAPI Knows how to reconcile all the CAPI resources for a unique token.
// TODO(alberto): consider stronger decoupling from Token by making it an interface
// and let nodepool, hostedcluster, and client be fields of CAPI / interface methods.
Expand All @@ -54,6 +60,7 @@ type CAPI struct {
capiClusterName string
scaleFromZeroPlatform hyperv1.PlatformType
upsert.ApplyProvider
conditions []hyperv1.NodePoolCondition

@bryan-cox bryan-cox Jul 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hypershift-jira-solve-ci hypershift-jira-solve-ci Bot Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// hasStatusCapacity checks if a machine template has Status.Capacity populated
Expand Down Expand Up @@ -84,37 +91,36 @@ func newCAPI(token *Token, capiClusterName string) (*CAPI, error) {
}, nil
}

func (c *CAPI) Reconcile(ctx context.Context) error {
func (c *CAPI) Reconcile(ctx context.Context) (*CAPIResult, error) {
log := ctrl.LoggerFrom(ctx)

nodePool := c.nodePool
if err := c.cleanupMachineTemplates(ctx, log, nodePool, c.controlplaneNamespace); err != nil {
return err
return nil, err
}

if c.nodePool.Spec.Platform.Type == hyperv1.AWSPlatform {
if err := c.reconcileAWSMachines(ctx); err != nil {
return err
return nil, err
}
}

// Reconcile (Platform)MachineTemplate.
template, err := c.machineTemplateBuilders(ctx)
if err != nil {
return err
return nil, err
}

if result, err := c.ApplyManifest(ctx, c.Client, template); err != nil {
return err
return nil, err
} else {
log.Info("Reconciled Machine template", "result", result)
}

// Check if platform machine template needs to be updated.
targetMachineTemplate := template.GetName()
if isUpdatingMachineTemplate(nodePool, targetMachineTemplate) {
// TODO (alberto): deocuple all conditions handling from this file into nodepool_controller.go dedicated function.
SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
c.conditions = append(c.conditions, hyperv1.NodePoolCondition{
Type: hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType,
Status: corev1.ConditionTrue,
Reason: hyperv1.AsExpectedReason,
Expand All @@ -125,7 +131,7 @@ func (c *CAPI) Reconcile(ctx context.Context) error {
"current", nodePool.GetAnnotations()[nodePoolAnnotationPlatformMachineTemplate],
"target", targetMachineTemplate)
} else {
SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
c.conditions = append(c.conditions, hyperv1.NodePoolCondition{
Type: hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType,
Status: corev1.ConditionFalse,
Reason: hyperv1.AsExpectedReason,
Expand All @@ -141,7 +147,7 @@ func (c *CAPI) Reconcile(ctx context.Context) error {
ms,
template)
}); err != nil {
return fmt.Errorf("failed to reconcile MachineSet %q: %w",
return nil, fmt.Errorf("failed to reconcile MachineSet %q: %w",
client.ObjectKeyFromObject(ms).String(), err)
Comment on lines +150 to 151

@coderabbitai coderabbitai Bot Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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 accumulated CAPIResult on 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.

} else {
log.Info("Reconciled MachineSet", "result", result)
Expand All @@ -157,7 +163,7 @@ func (c *CAPI) Reconcile(ctx context.Context) error {
md,
template)
}); err != nil {
return fmt.Errorf("failed to reconcile MachineDeployment %q: %w",
return nil, fmt.Errorf("failed to reconcile MachineDeployment %q: %w",
client.ObjectKeyFromObject(md).String(), err)
} else {
log.Info("Reconciled MachineDeployment", "result", result)
Expand All @@ -166,20 +172,20 @@ func (c *CAPI) Reconcile(ctx context.Context) error {

mhc := c.machineHealthCheck()
if nodePool.Spec.Management.AutoRepair {
if c := FindStatusCondition(nodePool.Status.Conditions, hyperv1.NodePoolReachedIgnitionEndpoint); c == nil || c.Status != corev1.ConditionTrue {
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

@bryan-cox bryan-cox Jul 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hypershift-jira-solve-ci hypershift-jira-solve-ci Bot Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

if result, err := ctrl.CreateOrUpdate(ctx, c.Client, mhc, func() error {
return c.reconcileMachineHealthCheck(ctx, mhc)
}); err != nil {
return fmt.Errorf("failed to reconcile MachineHealthCheck %q: %w",
return nil, fmt.Errorf("failed to reconcile MachineHealthCheck %q: %w",
client.ObjectKeyFromObject(mhc).String(), err)
} else {
log.Info("Reconciled MachineHealthCheck", "result", result)
}
SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
c.conditions = append(c.conditions, hyperv1.NodePoolCondition{
Type: hyperv1.NodePoolAutorepairEnabledConditionType,
Status: corev1.ConditionTrue,
Reason: hyperv1.AsExpectedReason,
Expand All @@ -188,14 +194,14 @@ func (c *CAPI) Reconcile(ctx context.Context) error {
} else {
err := c.Get(ctx, client.ObjectKeyFromObject(mhc), mhc)
if err != nil && !apierrors.IsNotFound(err) {
return err
return nil, err
}
if err == nil {
if err := c.Delete(ctx, mhc); err != nil && !apierrors.IsNotFound(err) {
return err
return nil, err
}
}
SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
c.conditions = append(c.conditions, hyperv1.NodePoolCondition{
Type: hyperv1.NodePoolAutorepairEnabledConditionType,
Status: corev1.ConditionFalse,
Reason: hyperv1.AsExpectedReason,
Expand All @@ -209,7 +215,7 @@ func (c *CAPI) Reconcile(ctx context.Context) error {
if result, err := ctrl.CreateOrUpdate(ctx, c.Client, spotMHC, func() error {
return c.reconcileSpotMachineHealthCheck(ctx, spotMHC)
}); err != nil {
return fmt.Errorf("failed to reconcile spot MachineHealthCheck %q: %w",
return nil, fmt.Errorf("failed to reconcile spot MachineHealthCheck %q: %w",
client.ObjectKeyFromObject(spotMHC).String(), err)
} else {
log.Info("Reconciled spot MachineHealthCheck", "result", result)
Expand All @@ -218,16 +224,16 @@ func (c *CAPI) Reconcile(ctx context.Context) error {
// Delete spot MHC if spot is not enabled
err := c.Get(ctx, client.ObjectKeyFromObject(spotMHC), spotMHC)
if err != nil && !apierrors.IsNotFound(err) {
return err
return nil, err
}
if err == nil {
if err := c.Delete(ctx, spotMHC); err != nil && !apierrors.IsNotFound(err) {
return err
return nil, err
}
}
}

return nil
return &CAPIResult{Conditions: c.conditions}, nil
}

func (c *CAPI) cleanupMachineTemplates(ctx context.Context, log logr.Logger, nodePool *hyperv1.NodePool, controlPlaneNamespace string) error {
Expand Down Expand Up @@ -653,7 +659,7 @@ func (c *CAPI) reconcileMachineDeploymentStatus(log logr.Logger, machineDeployme
if cond.Reason != "" {
reason = cond.Reason
}
SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
c.conditions = append(c.conditions, hyperv1.NodePoolCondition{
Type: hyperv1.NodePoolReadyConditionType,
Status: cond.Status,
ObservedGeneration: nodePool.Generation,
Expand Down Expand Up @@ -1047,22 +1053,22 @@ func (c *CAPI) reconcileMachineSet(ctx context.Context,

// Bubble up AvailableReplicas and Ready condition from MachineSet.
nodePool.Status.Replicas = machineSet.Status.AvailableReplicas
for _, c := range machineSet.Status.Conditions {
for _, cond := range machineSet.Status.Conditions {
// This condition should aggregate and summarize readiness from underlying MachineSets and Machines
// https://github.com/kubernetes-sigs/cluster-api/issues/3486.
if c.Type == capiv1.ReadyCondition {
if cond.Type == capiv1.ReadyCondition {
// this is so api server does not complain
// invalid value: \"\": status.conditions.reason in body should be at least 1 chars long"
reason := hyperv1.AsExpectedReason
if c.Reason != "" {
reason = c.Reason
if cond.Reason != "" {
reason = cond.Reason
}

SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
c.conditions = append(c.conditions, hyperv1.NodePoolCondition{
Type: hyperv1.NodePoolReadyConditionType,
Status: c.Status,
Status: cond.Status,
ObservedGeneration: nodePool.Generation,
Message: c.Message,
Message: cond.Message,
Reason: reason,
})
break
Expand Down
34 changes: 22 additions & 12 deletions hypershift-operator/controllers/nodepool/capi_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1881,11 +1881,13 @@ func TestCAPIReconcile(t *testing.T) {
g.Expect(err).NotTo(HaveOccurred())
g.Expect(templateList.Items).To(HaveLen(2))

err = capi.Reconcile(t.Context())
capiResult, err := capi.Reconcile(t.Context())
if tt.expectedError {
g.Expect(err).To(HaveOccurred())
} else {
g.Expect(err).NotTo(HaveOccurred())
g.Expect(capiResult).NotTo(BeNil())
g.Expect(capiResult.Conditions).NotTo(BeEmpty())

// Check that old machine templates are deleted.
templateList := &capiaws.AWSMachineTemplateList{}
Expand Down Expand Up @@ -1995,8 +1997,10 @@ func TestCAPIReconcile(t *testing.T) {
g.Expect(err).NotTo(HaveOccurred())

// Re-run reconcile.
err = capi.Reconcile(t.Context())
capiResult, err = capi.Reconcile(t.Context())
g.Expect(err).NotTo(HaveOccurred())
g.Expect(capiResult).NotTo(BeNil())
g.Expect(capiResult.Conditions).NotTo(BeEmpty())

// Check for the expected annotations.
// TODO(alberto): reconcileMachineDeployment mutate the NodePool with this annotations and status.version.
Expand Down Expand Up @@ -2165,14 +2169,17 @@ func TestCAPIReconcile_machineset(t *testing.T) {
ApplyProvider: upsert.NewApplyProvider(false),
}

err := capi.Reconcile(t.Context())
capiResult, err := capi.Reconcile(t.Context())
g.Expect(err).NotTo(HaveOccurred())

ms := &capiv1.MachineSet{}
err = capi.Client.Get(t.Context(), client.ObjectKey{Namespace: controlplaneNamespace, Name: tt.nodePool.GetName()}, ms)
g.Expect(err).NotTo(HaveOccurred())
g.Expect(ms.Spec.Template.Spec.NodeDrainTimeout).To(Equal(tt.nodePool.Spec.NodeDrainTimeout))
g.Expect(ms.Spec.Template.Spec.NodeVolumeDetachTimeout).To(Equal(tt.nodePool.Spec.NodeVolumeDetachTimeout))

g.Expect(capiResult).NotTo(BeNil())
g.Expect(capiResult.Conditions).NotTo(BeEmpty())
})
}
}
Expand All @@ -2190,7 +2197,7 @@ func TestGlobalPSManagedLabelOnMachines(t *testing.T) {
nodePool *hyperv1.NodePool
hostedCluster *hyperv1.HostedCluster
objects []client.Object
reconcile func(t *testing.T, capi *CAPI) error
reconcile func(t *testing.T, capi *CAPI) (*CAPIResult, error)
expectLabel bool
}{
{
Expand Down Expand Up @@ -2332,10 +2339,10 @@ func TestGlobalPSManagedLabelOnMachines(t *testing.T) {
},
},
},
reconcile: func(t *testing.T, capi *CAPI) error {
reconcile: func(t *testing.T, capi *CAPI) (*CAPIResult, error) {
md := &capiv1.MachineDeployment{}
if err := capi.Client.Get(t.Context(), client.ObjectKey{Namespace: controlPlaneNamespace, Name: "test-nodepool"}, md); err != nil {
return err
return nil, err
}
template := &capiazure.AzureMachineTemplate{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -2344,7 +2351,7 @@ func TestGlobalPSManagedLabelOnMachines(t *testing.T) {
},

@bryan-cox bryan-cox Jul 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hypershift-jira-solve-ci hypershift-jira-solve-ci Bot Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
log := ctrl.LoggerFrom(t.Context())
return capi.reconcileMachineDeployment(t.Context(), log, md, template)
return &CAPIResult{Conditions: capi.conditions}, capi.reconcileMachineDeployment(t.Context(), log, md, template)
},
expectLabel: true,
},
Expand Down Expand Up @@ -2481,10 +2488,10 @@ func TestGlobalPSManagedLabelOnMachines(t *testing.T) {
},
// Call reconcileMachineDeployment directly to test the label logic
// without requiring full KubeVirt machine template setup.
reconcile: func(t *testing.T, capi *CAPI) error {
reconcile: func(t *testing.T, capi *CAPI) (*CAPIResult, error) {
md := &capiv1.MachineDeployment{}
if err := capi.Client.Get(t.Context(), client.ObjectKey{Namespace: controlPlaneNamespace, Name: "test-nodepool"}, md); err != nil {
return err
return nil, err
}
kvTemplate := &capikubevirt.KubevirtMachineTemplate{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -2493,7 +2500,7 @@ func TestGlobalPSManagedLabelOnMachines(t *testing.T) {
},
}
log := ctrl.LoggerFrom(t.Context())
return capi.reconcileMachineDeployment(t.Context(), log, md, kvTemplate)
return &CAPIResult{Conditions: capi.conditions}, capi.reconcileMachineDeployment(t.Context(), log, md, kvTemplate)
},
},
}
Expand Down Expand Up @@ -2535,11 +2542,11 @@ func TestGlobalPSManagedLabelOnMachines(t *testing.T) {

reconcile := tt.reconcile
if reconcile == nil {
reconcile = func(t *testing.T, capi *CAPI) error {
reconcile = func(t *testing.T, capi *CAPI) (*CAPIResult, error) {
return capi.Reconcile(t.Context())
}
}
err := reconcile(t, capi)
_, err := reconcile(t, capi)
g.Expect(err).NotTo(HaveOccurred())

globalPSManagedLabelKey := fmt.Sprintf("%s.%s", labelManagedPrefix, globalPSNodeLabel)
Expand Down Expand Up @@ -3063,6 +3070,9 @@ func TestReconcileMachineDeploymentStatus(t *testing.T) {
}

capi.reconcileMachineDeploymentStatus(logr.Discard(), tc.machineDeployment, templateCR)
for _, cond := range capi.conditions {
SetStatusCondition(&nodePool.Status.Conditions, cond)

@bryan-cox bryan-cox Jul 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hypershift-jira-solve-ci hypershift-jira-solve-ci Bot Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — the test was indeed asserting against stale state before this fix.

AI-assisted response via Claude Code

}

g.Expect(nodePool.Status.Replicas).To(Equal(tc.expectedReplicas))
g.Expect(nodePool.Status.Version).To(Equal(tc.expectedVersion))
Expand Down
9 changes: 5 additions & 4 deletions hypershift-operator/controllers/nodepool/nodepool_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,9 +315,6 @@ func (r *NodePoolReconciler) reconcile(ctx context.Context, hcluster *hyperv1.Ho
r.reachedIgnitionEndpointCondition,
r.machineAndNodeConditions,
r.validPlatformConfigCondition,
// TODO(alberto): consider moving here:
// NodePoolUpdatingPlatformMachineTemplateConditionType,
// NodePoolAutorepairEnabledConditionType.
}
for _, f := range signalConditions {
result, err := f(ctx, nodePool, hcluster)
Expand Down Expand Up @@ -429,14 +426,18 @@ func (r *NodePoolReconciler) reconcile(ctx context.Context, hcluster *hyperv1.Ho
return ctrl.Result{}, nil
}

if err := capi.Reconcile(ctx); err != nil {
capiResult, err := capi.Reconcile(ctx)
if err != nil {
var notReadyErr *NotReadyError
if coreerrors.As(err, &notReadyErr) {
log.Info("Waiting to create machine template", "message", err.Error())
return ctrl.Result{RequeueAfter: 5 * time.Second}, nil
}
return ctrl.Result{}, err
}
for _, condition := range capiResult.Conditions {
SetStatusCondition(&nodePool.Status.Conditions, condition)
}

@bryan-cox bryan-cox Jul 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hypershift-jira-solve-ci hypershift-jira-solve-ci Bot Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Set scale-from-zero annotations if provider is configured and platform is supported
// This works for both Replace (MachineDeployment) and InPlace (MachineSet) upgrade types
Expand Down