From 7dae429e8c7f6d3516b4bd7f84da5d916a84fe8c Mon Sep 17 00:00:00 2001 From: Donnie Adams Date: Thu, 3 Mar 2022 10:44:33 -0700 Subject: [PATCH] Consistent ordering for deletion priority If two machines had the same deletion priority, then on subsequent syncReplicas calls different machines could be deleted. For example, when using Oldest or Newest deletion priorities, then machines that are already deleting and machines without a node ref have the same priority. Then this could lead to multiple machines getting deleted on different calls to syncReplicas. That is, if a MachineSet is scaled down by 1, then one machine would get deleted, but then a second call to syncReplicas could delete a machine with no node ref instead of the machine that is already deleting. Signed-off-by: Donnie Adams --- .../machineset/machineset_delete_policy.go | 6 +- .../machineset_delete_policy_test.go | 94 +++++++++++++++++++ 2 files changed, 99 insertions(+), 1 deletion(-) diff --git a/internal/controllers/machineset/machineset_delete_policy.go b/internal/controllers/machineset/machineset_delete_policy.go index 4c35595948a3..fb33b9daf4b4 100644 --- a/internal/controllers/machineset/machineset_delete_policy.go +++ b/internal/controllers/machineset/machineset_delete_policy.go @@ -97,7 +97,11 @@ type sortableMachines struct { func (m sortableMachines) Len() int { return len(m.machines) } func (m sortableMachines) Swap(i, j int) { m.machines[i], m.machines[j] = m.machines[j], m.machines[i] } func (m sortableMachines) Less(i, j int) bool { - return m.priority(m.machines[j]) < m.priority(m.machines[i]) // high to low + priorityI, priorityJ := m.priority(m.machines[i]), m.priority(m.machines[j]) + if priorityI == priorityJ { + return m.machines[i].Name < m.machines[j].Name + } + return priorityJ < priorityI // high to low } func getMachinesToDeletePrioritized(filteredMachines []*clusterv1.Machine, diff int, fun deletePriorityFunc) []*clusterv1.Machine { diff --git a/internal/controllers/machineset/machineset_delete_policy_test.go b/internal/controllers/machineset/machineset_delete_policy_test.go index 8448f7a34f99..eb550a218bfc 100644 --- a/internal/controllers/machineset/machineset_delete_policy_test.go +++ b/internal/controllers/machineset/machineset_delete_policy_test.go @@ -17,11 +17,13 @@ limitations under the License. package machineset import ( + "fmt" "testing" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/rand" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" capierrors "sigs.k8s.io/cluster-api/errors" @@ -523,6 +525,98 @@ func TestMachineOldestDelete(t *testing.T) { } } +func TestMachineDeleteMultipleSamePriority(t *testing.T) { + machines := make([]*clusterv1.Machine, 0, 10) + // All of these machines will have the same delete priority because they all have the "must delete" annotation. + for i := 0; i < 10; i++ { + machines = append(machines, &clusterv1.Machine{ + ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("machine-%d", i), Annotations: map[string]string{clusterv1.DeleteMachineAnnotation: "true"}}, + }) + } + + tests := []struct { + desc string + diff int + deletePriority deletePriorityFunc + }{ + { + desc: "multiple with same priority, func=oldestDeletePriority, diff=1", + diff: 1, + deletePriority: oldestDeletePriority, + }, + { + desc: "multiple with same priority, func=oldestDeletePriority, diff=5", + diff: 5, + deletePriority: oldestDeletePriority, + }, + { + desc: "multiple with same priority, func=oldestDeletePriority, diff=rand", + diff: rand.Intn(len(machines)), + deletePriority: oldestDeletePriority, + }, + { + desc: "multiple with same priority, func=randomDeletePolicy, diff=1", + diff: 1, + deletePriority: randomDeletePolicy, + }, + { + desc: "multiple with same priority, func=randomDeletePolicy, diff=5", + diff: 5, + deletePriority: randomDeletePolicy, + }, + { + desc: "multiple with same priority, func=randomDeletePolicy, diff=rand", + diff: rand.Intn(len(machines)), + deletePriority: randomDeletePolicy, + }, + { + desc: "multiple with same priority, func=newestDeletePriority, diff=1", + diff: 1, + deletePriority: newestDeletePriority, + }, + { + desc: "multiple with same priority, func=newestDeletePriority, diff=5", + diff: 5, + deletePriority: newestDeletePriority, + }, + { + desc: "multiple with same priority, func=newestDeletePriority, diff=rand", + diff: rand.Intn(len(machines)), + deletePriority: newestDeletePriority, + }, + { + desc: "multiple with same priority, func=randomDeletePolicy, diff=1", + diff: 1, + deletePriority: randomDeletePolicy, + }, + { + desc: "multiple with same priority, func=randomDeletePolicy, diff=5", + diff: 5, + deletePriority: randomDeletePolicy, + }, + { + desc: "multiple with same priority, func=randomDeletePolicy, diff=rand", + diff: rand.Intn(len(machines)), + deletePriority: randomDeletePolicy, + }, + } + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + g := NewWithT(t) + + order := rand.Perm(len(machines)) + shuffledMachines := make([]*clusterv1.Machine, len(machines)) + for i, j := range order { + shuffledMachines[i] = machines[j] + } + + result := getMachinesToDeletePrioritized(shuffledMachines, test.diff, test.deletePriority) + g.Expect(result).To(Equal(machines[:test.diff])) + }) + } +} + func TestIsMachineHealthy(t *testing.T) { nodeRef := &corev1.ObjectReference{Name: "some-node"} statusError := capierrors.MachineStatusError("I'm unhealthy!")