From 60e4c9b3774540000cb3473a500e9b2617cc60c7 Mon Sep 17 00:00:00 2001
From: fabriziopandini <fpandini@vmware.com>
Date: Fri, 14 Jun 2024 19:50:48 +0200
Subject: [PATCH] Deprioritize unknown NodeHealthy conditions for deletion

---
 .../machineset/machineset_delete_policy.go    |  3 +-
 .../machineset_delete_policy_test.go          | 41 +++++++++++++++----
 2 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/internal/controllers/machineset/machineset_delete_policy.go b/internal/controllers/machineset/machineset_delete_policy.go
index 9ad2beb99890..cc6abf6417bb 100644
--- a/internal/controllers/machineset/machineset_delete_policy.go
+++ b/internal/controllers/machineset/machineset_delete_policy.go
@@ -146,8 +146,9 @@ func isMachineHealthy(machine *clusterv1.Machine) bool {
 	if machine.Status.FailureReason != nil || machine.Status.FailureMessage != nil {
 		return false
 	}
+	// Note: for the sake of prioritization, we are not making any assumption about Health when ConditionUnknown.
 	nodeHealthyCondition := conditions.Get(machine, clusterv1.MachineNodeHealthyCondition)
-	if nodeHealthyCondition != nil && nodeHealthyCondition.Status != corev1.ConditionTrue {
+	if nodeHealthyCondition != nil && nodeHealthyCondition.Status == corev1.ConditionFalse {
 		return false
 	}
 	healthCheckCondition := conditions.Get(machine, clusterv1.MachineHealthCheckSucceededCondition)
diff --git a/internal/controllers/machineset/machineset_delete_policy_test.go b/internal/controllers/machineset/machineset_delete_policy_test.go
index 024599c75123..775937c327bd 100644
--- a/internal/controllers/machineset/machineset_delete_policy_test.go
+++ b/internal/controllers/machineset/machineset_delete_policy_test.go
@@ -68,7 +68,7 @@ func TestMachineToDelete(t *testing.T) {
 			},
 		},
 	}
-	healthyCheckConditionFalseMachine := &clusterv1.Machine{
+	healthCheckSucceededConditionFalseMachine := &clusterv1.Machine{
 		Status: clusterv1.MachineStatus{
 			NodeRef: nodeRef,
 			Conditions: clusterv1.Conditions{
@@ -79,6 +79,17 @@ func TestMachineToDelete(t *testing.T) {
 			},
 		},
 	}
+	healthCheckSucceededConditionUnknownMachine := &clusterv1.Machine{
+		Status: clusterv1.MachineStatus{
+			NodeRef: nodeRef,
+			Conditions: clusterv1.Conditions{
+				{
+					Type:   clusterv1.MachineHealthCheckSucceededCondition,
+					Status: corev1.ConditionUnknown,
+				},
+			},
+		},
+	}
 
 	tests := []struct {
 		desc     string
@@ -230,19 +241,31 @@ func TestMachineToDelete(t *testing.T) {
 				healthyMachine,
 			},
 			expect: []*clusterv1.Machine{
-				nodeHealthyConditionUnknownMachine,
+				healthyMachine,
 			},
 		},
 		{
-			desc: "func=randomDeletePolicy, NodeHealthyConditionFalseMachine, diff=1",
+			desc: "func=randomDeletePolicy, HealthCheckSucceededConditionFalseMachine, diff=1",
 			diff: 1,
 			machines: []*clusterv1.Machine{
 				healthyMachine,
-				healthyCheckConditionFalseMachine,
+				healthCheckSucceededConditionFalseMachine,
 				healthyMachine,
 			},
 			expect: []*clusterv1.Machine{
-				healthyCheckConditionFalseMachine,
+				healthCheckSucceededConditionFalseMachine,
+			},
+		},
+		{
+			desc: "func=randomDeletePolicy, HealthCheckSucceededConditionUnknownMachine, diff=1",
+			diff: 1,
+			machines: []*clusterv1.Machine{
+				healthyMachine,
+				healthCheckSucceededConditionUnknownMachine,
+				healthyMachine,
+			},
+			expect: []*clusterv1.Machine{
+				healthyMachine,
 			},
 		},
 	}
@@ -383,9 +406,10 @@ func TestMachineNewestDelete(t *testing.T) {
 			desc: "func=newestDeletePriority, diff=1 (nodeHealthyConditionUnknownMachine)",
 			diff: 1,
 			machines: []*clusterv1.Machine{
+				// nodeHealthyConditionUnknownMachine is not considered unhealthy with unknown condition.
 				secondNewest, oldest, secondOldest, newest, nodeHealthyConditionUnknownMachine,
 			},
-			expect: []*clusterv1.Machine{nodeHealthyConditionUnknownMachine},
+			expect: []*clusterv1.Machine{newest},
 		},
 	}
 
@@ -546,7 +570,8 @@ func TestMachineOldestDelete(t *testing.T) {
 			machines: []*clusterv1.Machine{
 				empty, secondNewest, oldest, secondOldest, newest, nodeHealthyConditionUnknownMachine,
 			},
-			expect: []*clusterv1.Machine{nodeHealthyConditionUnknownMachine},
+			// nodeHealthyConditionUnknownMachine is not considered unhealthy with unknown condition.
+			expect: []*clusterv1.Machine{oldest},
 		},
 		// these two cases ensures the mustDeleteMachine is always picked regardless of the machine names.
 		{
@@ -726,7 +751,7 @@ func TestIsMachineHealthy(t *testing.T) {
 					},
 				},
 			},
-			expect: false,
+			expect: true,
 		},
 		{
 			desc: "when all requirements are met for node to be healthy",