diff --git a/pkg/controller/elasticsearch/driver/upgrade_pods_deletion.go b/pkg/controller/elasticsearch/driver/upgrade_pods_deletion.go index a2871561a6..2c49757771 100644 --- a/pkg/controller/elasticsearch/driver/upgrade_pods_deletion.go +++ b/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 }) } diff --git a/pkg/controller/elasticsearch/driver/upgrade_predicates_test.go b/pkg/controller/elasticsearch/driver/upgrade_predicates_test.go index c0c9b323a3..5374beda62 100644 --- a/pkg/controller/elasticsearch/driver/upgrade_predicates_test.go +++ b/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) }) } }