Merge pull request #10763 from fabriziopandini/deprioritize-unknown-nodehalthy-for-deletion

🌱 Deprioritize unknown NodeHealthy conditions for deletion

Author: k8s-ci-robot

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)

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