Update deletion order sort test (#2394)

pkg/controller/elasticsearch/driver/upgrade_pods_deletion.go

@@ -99,34 +99,34 @@ func (ctx *rollingUpgradeCtx) getAllowedDeletions() (int, bool) {
 }
 
 // sortCandidates is the default sort function, masters have lower priority as
-// we want to update the data nodes first.
-// If 2 Pods are of the same type then use the reverse ordinal order.
+// we want to update the data nodes first. After that pods are sorted by stateful set name
+// then reverse ordinal order
 // TODO: Add some priority to unhealthy (bootlooping) Pods
 func sortCandidates(allPods []corev1.Pod) {
 	sort.Slice(allPods, func(i, j int) bool {
 		pod1 := allPods[i]
 		pod2 := allPods[j]
-		if (label.IsMasterNode(pod1) && label.IsMasterNode(pod2)) ||
-			(!label.IsMasterNode(pod1) && !label.IsMasterNode(pod2)) { // same type, use the reverse name function
-			ssetName1, ord1, err := sset.StatefulSetName(pod1.Name)
-			if err != nil {
-				return false
-			}
-			ssetName2, ord2, err := sset.StatefulSetName(pod2.Name)
-			if err != nil {
-				return false
-			}
-			if ssetName1 == ssetName2 {
-				// same name, compare ordinal, higher first
-				return ord1 > ord2
-			}
-			return ssetName1 < ssetName2
-		}
+		// check if either is a master node. masters come after all other roles
 		if label.IsMasterNode(pod1) && !label.IsMasterNode(pod2) {
-			// pod2 has higher priority since it is a data node
 			return false
 		}
-		return true
+		if !label.IsMasterNode(pod1) && label.IsMasterNode(pod2) {
+			return true
+		}
+		// neither or both are masters, use the reverse name function
+		ssetName1, ord1, err := sset.StatefulSetName(pod1.Name)
+		if err != nil {
+			return false
+		}
+		ssetName2, ord2, err := sset.StatefulSetName(pod2.Name)
+		if err != nil {
+			return false
+		}
+		if ssetName1 == ssetName2 {
+			// same name, compare ordinal, higher first
+			return ord1 > ord2
+		}
+		return ssetName1 < ssetName2
 	})
 }
 

pkg/controller/elasticsearch/driver/upgrade_predicates_test.go

@@ -15,6 +15,7 @@ import (
 	"github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/reconcile"
 	"github.com/elastic/cloud-on-k8s/pkg/utils/k8s"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 // These tests are focused on "type changes", i.e. when the type of a nodeSet is changed.
@@ -675,48 +676,51 @@ func TestDeletionStrategy_SortFunction(t *testing.T) {
 			name: "Mixed nodes",
 			fields: fields{
 				upgradeTestPods: newUpgradeTestPods(
-					newTestPod("masters-0").needsUpgrade(true),
-					newTestPod("data-0").needsUpgrade(true),
-					newTestPod("masters-1").needsUpgrade(true),
-					newTestPod("data-1").needsUpgrade(true),
-					newTestPod("masters-2").needsUpgrade(true),
+					// use "amasters" rather than "masters" to ensure we are not relying on the name sort accidentally
+					newTestPod("amasters-0").isMaster(true).needsUpgrade(true),
+					newTestPod("data-0").isData(true).needsUpgrade(true),
+					newTestPod("masters-0").isMaster(true).needsUpgrade(true),
+					newTestPod("amasters-1").isMaster(true).needsUpgrade(true),
+					newTestPod("data-1").isData(true).needsUpgrade(true),
+					newTestPod("amasters-2").isMaster(true).needsUpgrade(true),
 				),
 				esState: &testESState{
-					inCluster: []string{"data-1", "data-0", "masters-2", "masters-1", "masters-0"},
+					inCluster: []string{"data-1", "data-0", "amasters-2", "amasters-1", "amasters-0", "masters-0"},
 					health:    esv1.ElasticsearchUnknownHealth,
 				},
 			},
-			want: []string{"data-1", "data-0", "masters-2", "masters-1", "masters-0"},
+			want: []string{"data-1", "data-0", "amasters-2", "amasters-1", "amasters-0", "masters-0"},
 		},
 		{
 			name: "Masters first",
 			fields: fields{
 				upgradeTestPods: newUpgradeTestPods(
-					newTestPod("masters-0").needsUpgrade(true),
-					newTestPod("masters-1").needsUpgrade(true),
-					newTestPod("masters-2").needsUpgrade(true),
-					newTestPod("data-0").needsUpgrade(true),
-					newTestPod("data-1").needsUpgrade(true),
-					newTestPod("data-2").needsUpgrade(true),
+					// use "amasters" rather than "masters" to ensure we are not relying on the name sort accidentally
+					newTestPod("amasters-0").isMaster(true).needsUpgrade(true),
+					newTestPod("amasters-1").isMaster(true).needsUpgrade(true),
+					newTestPod("amasters-2").isMaster(true).needsUpgrade(true),
+					newTestPod("data-0").isData(true).needsUpgrade(true),
+					newTestPod("data-1").isData(true).needsUpgrade(true),
+					newTestPod("data-2").isData(true).needsUpgrade(true),
 				),
 				esState: &testESState{
-					inCluster: []string{"data-2", "data-1", "data-0", "masters-2", "masters-1", "masters-0"},
+					inCluster: []string{"data-2", "data-1", "data-0", "amasters-2", "amasters-1", "amasters-0"},
 					health:    esv1.ElasticsearchUnknownHealth,
 				},
 			},
-			want: []string{"data-2", "data-1", "data-0", "masters-2", "masters-1", "masters-0"},
+			want: []string{"data-2", "data-1", "data-0", "amasters-2", "amasters-1", "amasters-0"},
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			toUpgrade := tt.fields.upgradeTestPods.toUpgrade()
 			sortCandidates(toUpgrade)
-			assert.Equal(t, len(tt.want), len(toUpgrade))
-			for i := range tt.want {
-				if tt.want[i] != toUpgrade[i].Name {
-					t.Errorf("DeletionStrategyContext.SortFunction() = %v, want %v", names(toUpgrade), tt.want)
-				}
+			require.Equal(t, len(tt.want), len(toUpgrade))
+			var actualNames []string
+			for i := range toUpgrade {
+				actualNames = append(actualNames, toUpgrade[i].Name)
 			}
+			assert.Equal(t, tt.want, actualNames)
 		})
 	}
 }