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
+345 −46
Open

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

Show file tree
Hide file tree
Changes from all 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
Loading