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 <[email protected]>
kubernetes-sigs · Mar 11, 2022 · aad2560 · aad2560
1 parent 4afdea8
commit aad2560
Show file tree

Hide file tree

Showing 2 changed files with 99 additions and 1 deletion.
diff --git 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
@@ -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!")