Merge pull request #6300 from thedadams/machine-sort-delete-policy

🐛 Consistent ordering for deletion priority
kubernetes-sigs · Mar 30, 2022 · 2099b30 · 2099b30
2 parents 6b52d87 + 0c0db0c
commit 2099b30
Show file tree

Hide file tree

Showing 2 changed files with 101 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,13 @@ 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 {
+		// In cases where the priority is identical, it should be ensured that the same machine order is returned each time.
+		// Ordering by name is a simple way to do this.
+		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!")