Merge pull request #10431 from k8s-infra-cherrypick-robot/cherry-pick…

…-10087-to-release-1.6 [release-1.6] 🐛 fix: deletion priority to avoid deleting too many machines
kubernetes-sigs · Apr 15, 2024 · 36c0e55 · 36c0e55
2 parents cfefa6a + e844fb8
commit 36c0e55
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 6 deletions.
diff --git a/internal/controllers/machineset/machineset_delete_policy.go b/internal/controllers/machineset/machineset_delete_policy.go
@@ -35,6 +35,7 @@ type (
 
 const (
  mustDelete deletePriority = 100.0
+ shouldDelete deletePriority = 75.0
  betterDelete deletePriority = 50.0
  couldDelete deletePriority = 20.0
  mustNotDelete deletePriority = 0.0
@@ -48,10 +49,10 @@ func oldestDeletePriority(machine *clusterv1.Machine) deletePriority {
  return mustDelete
  }
  if _, ok := machine.ObjectMeta.Annotations[clusterv1.DeleteMachineAnnotation]; ok {
- return mustDelete
+ return shouldDelete
  }
  if !isMachineHealthy(machine) {
- return mustDelete
+ return betterDelete
  }
  if machine.ObjectMeta.CreationTimestamp.Time.IsZero() {
  return mustNotDelete
@@ -60,20 +61,20 @@ func oldestDeletePriority(machine *clusterv1.Machine) deletePriority {
  if d.Seconds() < 0 {
  return mustNotDelete
  }
- return deletePriority(float64(mustDelete) * (1.0 - math.Exp(-d.Seconds()/secondsPerTenDays)))
+ return deletePriority(float64(betterDelete) * (1.0 - math.Exp(-d.Seconds()/secondsPerTenDays)))
 }
 
 func newestDeletePriority(machine *clusterv1.Machine) deletePriority {
  if !machine.DeletionTimestamp.IsZero() {
  return mustDelete
  }
  if _, ok := machine.ObjectMeta.Annotations[clusterv1.DeleteMachineAnnotation]; ok {
- return mustDelete
+ return shouldDelete
  }
  if !isMachineHealthy(machine) {
- return mustDelete
+ return betterDelete
  }
- return mustDelete - oldestDeletePriority(machine)
+ return betterDelete - oldestDeletePriority(machine)
 }
 
 func randomDeletePolicy(machine *clusterv1.Machine) deletePriority {

diff --git a/internal/controllers/machineset/machineset_delete_policy_test.go b/internal/controllers/machineset/machineset_delete_policy_test.go
@@ -407,6 +407,18 @@ func TestMachineOldestDelete(t *testing.T) {
  ObjectMeta: metav1.ObjectMeta{CreationTimestamp: metav1.NewTime(currentTime.Time.AddDate(0, 0, -10))},
  Status: clusterv1.MachineStatus{FailureReason: &statusError, NodeRef: nodeRef},
  }
+ mustDeleteMachine := &clusterv1.Machine{
+ ObjectMeta: metav1.ObjectMeta{Name: "b", DeletionTimestamp: &currentTime},
+ Status: clusterv1.MachineStatus{NodeRef: nodeRef},
+ }
+ unhealthyMachineA := &clusterv1.Machine{
+ ObjectMeta: metav1.ObjectMeta{Name: "a", CreationTimestamp: metav1.NewTime(currentTime.Time.AddDate(0, 0, -10))},
+ Status: clusterv1.MachineStatus{FailureReason: &statusError, NodeRef: nodeRef},
+ }
+ unhealthyMachineZ := &clusterv1.Machine{
+ ObjectMeta: metav1.ObjectMeta{Name: "z", CreationTimestamp: metav1.NewTime(currentTime.Time.AddDate(0, 0, -10))},
+ Status: clusterv1.MachineStatus{FailureReason: &statusError, NodeRef: nodeRef},
+ }
  deleteMachineWithoutNodeRef := &clusterv1.Machine{
  ObjectMeta: metav1.ObjectMeta{CreationTimestamp: metav1.NewTime(currentTime.Time.AddDate(0, 0, -10))},
  }
@@ -513,6 +525,23 @@ func TestMachineOldestDelete(t *testing.T) {
  },
  expect: []*clusterv1.Machine{nodeHealthyConditionUnknownMachine},
  },
+ // these two cases ensures the mustDeleteMachine is always picked regardless of the machine names.
+ {
+ desc: "func=oldestDeletePriority, diff=1 (unhealthyMachineA)",
+ diff: 1,
+ machines: []*clusterv1.Machine{
+ empty, secondNewest, oldest, secondOldest, newest, mustDeleteMachine, unhealthyMachineA,
+ },
+ expect: []*clusterv1.Machine{mustDeleteMachine},
+ },
+ {
+ desc: "func=oldestDeletePriority, diff=1 (unhealthyMachineZ)",
+ diff: 1,
+ machines: []*clusterv1.Machine{
+ empty, secondNewest, oldest, secondOldest, newest, mustDeleteMachine, unhealthyMachineZ,
+ },
+ expect: []*clusterv1.Machine{mustDeleteMachine},
+ },
  }
 
  for _, test := range tests {