Skip to content

CNTRLPLANE-400: feat(remediationAllowed in NP): propagate MHC RemediationAllowed to NodePool Ready condition #9019

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

Merged
openshift-merge-bot merged 1 commit into from
Jul 28, 2026
Merged

CNTRLPLANE-400: feat(remediationAllowed in NP): propagate MHC RemediationAllowed to NodePool Ready condition #9019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions api/hypershift/v1beta1/nodepool_conditions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ const (
NodePoolUpdatingPlatformMachineTemplateConditionType = "UpdatingPlatformMachineTemplate"
// NodePoolReadyConditionType bubbles up CAPI MachineDeployment/MachineSet Ready condition.
// This is true when all replicas are ready Nodes.
// This may also be set to false when the MachineHealthCheck RemediationAllowed
// condition is false (reason TooManyUnhealthy), indicating that auto-repair is
// blocked because too many machines are unhealthy.
// When this is false for too long, NodePoolAllMachinesReadyConditionType and NodePoolAllNodesHealthyConditionType might provide more context.
NodePoolReadyConditionType = "Ready"
// NodePoolAllMachinesReadyConditionType bubbles up and aggregates CAPI Machine Ready condition.
Expand Down
73 changes: 73 additions & 0 deletions hypershift-operator/controllers/nodepool/capi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
"sigs.k8s.io/controller-runtime/pkg/event"
"sigs.k8s.io/controller-runtime/pkg/predicate"

"github.com/go-logr/logr"
)
Expand Down Expand Up @@ -185,6 +187,20 @@ func (c *CAPI) Reconcile(ctx context.Context) error {
Reason: hyperv1.AsExpectedReason,
ObservedGeneration: nodePool.Generation,
})

// 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 {

@jparrill jparrill Jul 22, 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.

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?

@mgencur mgencur Jul 22, 2026 •
edited
Loading

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.

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".

@sdminonne sdminonne Jul 23, 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.

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

if remediationAllowed.Status == corev1.ConditionFalse {
SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
Type: hyperv1.NodePoolReadyConditionType,
Status: corev1.ConditionFalse,
Reason: remediationAllowed.Reason,
Message: remediationAllowed.Message,
ObservedGeneration: nodePool.Generation,
})
}
}
} else {
err := c.Get(ctx, client.ObjectKeyFromObject(mhc), mhc)
if err != nil && !apierrors.IsNotFound(err) {
Expand Down Expand Up @@ -736,6 +752,13 @@ func (c *CAPI) reconcileMachineHealthCheck(ctx context.Context,
}
}

// Set the nodePoolAnnotation so the enqueueParentNodePool watch handler
// can map MHC changes back to the parent NodePool.
if mhc.Annotations == nil {
mhc.Annotations = map[string]string{}
}
mhc.Annotations[nodePoolAnnotation] = client.ObjectKeyFromObject(nodePool).String()

@jparrill jparrill Jul 22, 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.

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.


resourcesName := generateName(capiClusterName, nodePool.Spec.ClusterName, nodePool.GetName())
mhc.Spec = capiv1.MachineHealthCheckSpec{
ClusterName: capiClusterName,
Expand Down Expand Up @@ -1178,6 +1201,15 @@ func (c *CAPI) spotMachineHealthCheck() *capiv1.MachineHealthCheck {
// reconcileSpotMachineHealthCheck reconciles a MachineHealthCheck specifically for spot instances.
// This MHC selects machines with the interruptibleInstanceLabel.
func (c *CAPI) reconcileSpotMachineHealthCheck(_ context.Context, mhc *capiv1.MachineHealthCheck) error {
nodePool := c.nodePool

// Set the nodePoolAnnotation so the enqueueParentNodePool watch handler
// can map MHC changes back to the parent NodePool.
if mhc.Annotations == nil {
mhc.Annotations = map[string]string{}
}
mhc.Annotations[nodePoolAnnotation] = client.ObjectKeyFromObject(nodePool).String()

// Spot instances need shorter timeouts for faster response to interruption
maxUnhealthy := intstr.FromString("100%")
timeOut := 8 * time.Minute
Expand Down Expand Up @@ -1405,3 +1437,44 @@ func (r *NodePoolReconciler) getMachinesForNodePool(ctx context.Context, nodePoo

return sortedByCreationTimestamp(machinesForNodePool), nil
}

// findMHCRemediationAllowedCondition finds the RemediationAllowed condition in a CAPI Conditions slice.
func findMHCRemediationAllowedCondition(conditions capiv1.Conditions) *capiv1.Condition {
for i := range conditions {
if conditions[i].Type == capiv1.RemediationAllowedCondition {
return &conditions[i]
}
}
return nil
}

// mhcRemediationAllowedChangedPredicate returns a predicate that filters MHC events
// to only pass through when the RemediationAllowed condition has changed. Create and
// Delete events always pass through; Update events are filtered to avoid unnecessary
// 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{

@jparrill jparrill Jul 22, 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.

Nit: The comment says "Create and Delete events always pass through" but that's implicit (unset predicate.Funcs fields default to true). Two thoughts:

  1. Consider making it explicit with CreateFunc/DeleteFunc returning true for readability.
  2. The controller itself creates the MHC, so every Create event triggers an immediate redundant re-reconciliation. A CreateFunc returning false would avoid that wasted cycle.

@sdminonne sdminonne Jul 23, 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.

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

UpdateFunc: func(e event.UpdateEvent) bool {
oldMHC, ok := e.ObjectOld.(*capiv1.MachineHealthCheck)
if !ok {
return true
}
newMHC, ok := e.ObjectNew.(*capiv1.MachineHealthCheck)
if !ok {
return true
}
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
},
}
}
Loading