Merge pull request #8247 from sdminonne/CNTRLPLANE-2204

CNTRLPLANE-2204: improve AllNodesHealthy and AllMachinesReady conditions

Author: openshift-merge-bot[bot]

hypershift-operator/controllers/nodepool/conditions.go

@@ -98,11 +98,24 @@ func FindStatusCondition(conditions []hyperv1.NodePoolCondition, conditionType s
 	return nil
 }
 
-// FindStatusCondition finds the conditionType in conditions.
-func findCAPIStatusCondition(conditions []capiv1.Condition, conditionType capiv1.ConditionType) *capiv1.Condition {
-	for i := range conditions {
-		if conditions[i].Type == conditionType {
-			return &conditions[i]
+// machineConditionResult normalizes a CAPI Machine condition into a common struct.
+type machineConditionResult struct {
+	Status  corev1.ConditionStatus
+	Reason  string
+	Message string
+}
+
+// findMachineStatusCondition looks up a condition on a CAPI Machine from
+// Machine.Status.Conditions ([]capiv1.Condition).
+// Returns nil if the condition is not found.
+func findMachineStatusCondition(machine *capiv1.Machine, conditionType string) *machineConditionResult {
+	for i := range machine.Status.Conditions {
+		if string(machine.Status.Conditions[i].Type) == conditionType {
+			return &machineConditionResult{
+				Status:  machine.Status.Conditions[i].Status,
+				Reason:  machine.Status.Conditions[i].Reason,
+				Message: machine.Status.Conditions[i].Message,
+			}
 		}
 	}
 
@@ -604,29 +617,45 @@ func (r *NodePoolReconciler) setMachineAndNodeConditions(ctx context.Context, no
 func (r *NodePoolReconciler) setAllNodesHealthyCondition(nodePool *hyperv1.NodePool, machines []*capiv1.Machine) {
 	status := corev1.ConditionTrue
 	reason := hyperv1.AsExpectedReason
-	var message string
+	message := hyperv1.AllIsWellMessage
 
-	if len(machines) < 1 {
+	if numMachines := len(machines); numMachines == 0 {
 		status = corev1.ConditionFalse
 		reason = hyperv1.NodePoolNotFoundReason
 		message = "No Machines are created"
 		if nodePool.Spec.Replicas != nil && *nodePool.Spec.Replicas == 0 {
 			reason = hyperv1.AsExpectedReason
 			message = "NodePool set to no replicas"
 		}
-	}
+	} else {
+		numNotHealthy := 0
+		messageMap := make(map[string][]string)
 
-	for _, machine := range machines {
-		condition := findCAPIStatusCondition(machine.Status.Conditions, capiv1.MachineNodeHealthyCondition)
-		if condition != nil && condition.Status != corev1.ConditionTrue {
-			status = corev1.ConditionFalse
-			reason = condition.Reason
-			message = message + fmt.Sprintf("Machine %s: %s\n", machine.Name, condition.Reason)
+		for _, machine := range machines {
+			condition := findMachineStatusCondition(machine, string(capiv1.MachineNodeHealthyCondition))
+			if condition == nil {
+				// NodeHealthy condition not yet reported; treat as not healthy.
+				status = corev1.ConditionFalse
+				numNotHealthy++
+				mapReason := capiv1.WaitingForNodeRefReason
+				mapMessage := fmt.Sprintf("Machine %s: %s\n", machine.Name, mapReason)
+				messageMap[mapReason] = append(messageMap[mapReason], mapMessage)
+			} else if condition.Status != corev1.ConditionTrue {
+				status = corev1.ConditionFalse
+				numNotHealthy++
+				mapReason := condition.Reason
+				var mapMessage string
+				if condition.Message != "" {
+					mapMessage = fmt.Sprintf("Machine %s: %s: %s\n", machine.Name, condition.Reason, condition.Message)
+				} else {
+					mapMessage = fmt.Sprintf("Machine %s: %s\n", machine.Name, condition.Reason)
+				}
+				messageMap[mapReason] = append(messageMap[mapReason], mapMessage)
+			}
+		}
+		if numNotHealthy > 0 {
+			reason, message = aggregateMachineReasonsAndMessages(messageMap, numMachines, numNotHealthy, aggregatorMachineStateHealthy)
 		}
-	}
-
-	if status == corev1.ConditionTrue {
-		message = hyperv1.AllIsWellMessage
 	}
 
 	allMachinesHealthyCondition := &hyperv1.NodePoolCondition{
@@ -666,11 +695,18 @@ func (r *NodePoolReconciler) setAllMachinesReadyCondition(nodePool *hyperv1.Node
 		messageMap := make(map[string][]string)
 
 		for _, machine := range machines {
-			readyCond := findCAPIStatusCondition(machine.Status.Conditions, capiv1.ReadyCondition)
-			if readyCond != nil && readyCond.Status != corev1.ConditionTrue {
+			readyCond := findMachineStatusCondition(machine, string(capiv1.ReadyCondition))
+			if readyCond == nil {
+				// Ready condition not yet reported; treat as not ready.
+				status = corev1.ConditionFalse
+				numNotReady++
+				mapReason := capiv1.WaitingForInfrastructureFallbackReason
+				mapMessage := fmt.Sprintf("Machine %s: %s\n", machine.Name, mapReason)
+				messageMap[mapReason] = append(messageMap[mapReason], mapMessage)
+			} else if readyCond.Status != corev1.ConditionTrue {
 				status = corev1.ConditionFalse
 				numNotReady++
-				infraReadyCond := findCAPIStatusCondition(machine.Status.Conditions, capiv1.InfrastructureReadyCondition)
+				infraReadyCond := findMachineStatusCondition(machine, string(capiv1.InfrastructureReadyCondition))
 				// We append the reason as part of the higher Message, since the message is meaningless.
 				// This is how a CAPI condition looks like in AWS for an instance deleted out of band failure.
 				//	- lastTransitionTime: "2022-11-28T15:14:28Z"
@@ -685,7 +721,11 @@ func (r *NodePoolReconciler) setAllMachinesReadyCondition(nodePool *hyperv1.Node
 					mapMessage = fmt.Sprintf("Machine %s: %s: %s\n", machine.Name, infraReadyCond.Reason, infraReadyCond.Message)
 				} else {
 					mapReason = readyCond.Reason
-					mapMessage = fmt.Sprintf("Machine %s: %s\n", machine.Name, readyCond.Reason)
+					if readyCond.Message != "" && !isSetupCounterCondMessage.MatchString(readyCond.Message) {
+						mapMessage = fmt.Sprintf("Machine %s: %s: %s\n", machine.Name, readyCond.Reason, readyCond.Message)
+					} else {
+						mapMessage = fmt.Sprintf("Machine %s: %s\n", machine.Name, readyCond.Reason)
+					}
 				}
 
 				messageMap[mapReason] = append(messageMap[mapReason], mapMessage)

hypershift-operator/controllers/nodepool/nodepool_controller.go

@@ -1095,8 +1095,13 @@ func sortedByCreationTimestamp(machines []*capiv1.Machine) []*capiv1.Machine {
 
 const (
 	endOfMessage                         = "... too many similar errors\n"
+	endOfGlobalMessage                   = "... message truncated\n"
+	endOfReasons                         = ",ReasonsTruncated"
 	maxMessageLength                     = 1000
+	maxGlobalMessageLength               = 3000
+	maxReasonLength                      = 1024 // +kubebuilder:validation:MaxLength on NodePoolCondition.Reason
 	aggregatorMachineStateReady          = "ready"
+	aggregatorMachineStateHealthy        = "healthy"
 	aggregatorMachineStateLiveMigratable = "live migratable"
 )
 
@@ -1117,10 +1122,46 @@ func aggregateMachineReasonsAndMessages(messageMap map[string][]string, numMachi
 	sort.Strings(reasons)
 
 	for _, reason := range reasons {
-		msgBuilder.WriteString(aggregateMachineMessages(messageMap[reason]))
+		// Sort messages within each reason bucket to ensure deterministic output
+		// regardless of Kubernetes list order, avoiding unnecessary status updates.
+		sort.Strings(messageMap[reason])
+		reasonBlock := aggregateMachineMessages(messageMap[reason])
+		if msgBuilder.Len()+len(reasonBlock)+len(endOfGlobalMessage) > maxGlobalMessageLength {
+			msgBuilder.WriteString(endOfGlobalMessage)
+			break
+		}
+		msgBuilder.WriteString(reasonBlock)
 	}
 
-	return strings.Join(reasons, ","), msgBuilder.String()
+	return truncateReasons(reasons), msgBuilder.String()
+}
+
+// truncateReasons joins reasons with commas and truncates the result to fit
+// within the NodePoolCondition.Reason MaxLength=1024 validation limit.
+// When truncation occurs, the suffix ",ReasonsTruncated" is appended.
+func truncateReasons(reasons []string) string {
+	joined := strings.Join(reasons, ",")
+	if len(joined) <= maxReasonLength {
+		return joined
+	}
+
+	// Build the truncated reason string by adding reasons one at a time,
+	// reserving space for the endOfReasons suffix.
+	builder := strings.Builder{}
+	for i, reason := range reasons {
+		separator := ""
+		if i > 0 {
+			separator = ","
+		}
+		if builder.Len()+len(separator)+len(reason)+len(endOfReasons) > maxReasonLength {
+			builder.WriteString(endOfReasons)
+			break
+		}
+		builder.WriteString(separator)
+		builder.WriteString(reason)
+	}
+
+	return builder.String()
 }
 
 func aggregateMachineMessages(msgs []string) string {