Skip to content

Commit

Permalink
Merge pull request #10770 from k8s-infra-cherrypick-robot/cherry-pick…
Browse files Browse the repository at this point in the history
…-10763-to-release-1.7

[release-1.7] 🌱 Deprioritize unknown NodeHealthy conditions for deletion
  • Loading branch information
k8s-ci-robot authored Jun 17, 2024
2 parents 56c2676 + 60e4c9b commit bffacde
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 9 deletions.
3 changes: 2 additions & 1 deletion internal/controllers/machineset/machineset_delete_policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
41 changes: 33 additions & 8 deletions internal/controllers/machineset/machineset_delete_policy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func TestMachineToDelete(t *testing.T) {
},
},
}
healthyCheckConditionFalseMachine := &clusterv1.Machine{
healthCheckSucceededConditionFalseMachine := &clusterv1.Machine{
Status: clusterv1.MachineStatus{
NodeRef: nodeRef,
Conditions: clusterv1.Conditions{
Expand All @@ -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
Expand Down Expand Up @@ -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,
},
},
}
Expand Down Expand Up @@ -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},
},
}

Expand Down Expand Up @@ -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.
{
Expand Down Expand Up @@ -726,7 +751,7 @@ func TestIsMachineHealthy(t *testing.T) {
},
},
},
expect: false,
expect: true,
},
{
desc: "when all requirements are met for node to be healthy",
Expand Down

0 comments on commit bffacde

Please sign in to comment.