Delete old shutdowns before creating new ones (elastic#5629)

Moves the cleanup at the beginning of the upgrade function to make sure we don't end up with left over shutdowns and then don't get to clean them up in time because we exit early after node deletions.

pkg/controller/elasticsearch/driver/upgrade.go

@@ -67,6 +67,14 @@ func (d *defaultDriver) handleUpgrades(
 	logger := log.WithValues("namespace", d.ES.Namespace, "es_name", d.ES.Name)
 	nodeShutdown := shutdown.NewNodeShutdown(esClient, nodeNameToID, esclient.Restart, d.ES.ResourceVersion, logger)
 
+	// once expectations are satisfied we can already delete shutdowns that are complete and where the node
+	// is back in the cluster to avoid completed shutdowns from accumulating and affecting node availability calculations
+	// in Elasticsearch for example for indices with `auto_expand_replicas` setting.
+	if supportsNodeShutdown(esClient.Version()) {
+		// clear all shutdowns of type restart that have completed
+		results = results.WithError(nodeShutdown.Clear(ctx, esclient.ShutdownComplete.Applies, nodeShutdown.OnlyNodesInCluster))
+	}
+
 	// Get the list of pods currently existing in the StatefulSetList
 	currentPods, err := statefulSets.GetActualPods(d.Client)
 	if err != nil {
@@ -315,13 +323,6 @@ func (d *defaultDriver) maybeCompleteNodeUpgrades(
 		return results.WithReconciliationState(defaultRequeue.WithReason(reason))
 	}
 
-	// small optimisation: once expectations are satisfied we can already delete shutdowns that are complete and where the node
-	// is back in the cluster to avoid completed shutdowns from accumulating
-	if supportsNodeShutdown(esClient.Version()) {
-		// clear all shutdowns of type restart that have completed
-		results = results.WithError(nodeShutdown.Clear(ctx, esclient.ShutdownComplete.Applies, nodeShutdown.OnlyNodesInCluster))
-	}
-
 	statefulSets, err := sset.RetrieveActualStatefulSets(d.Client, k8s.ExtractNamespacedName(&d.ES))
 	if err != nil {
 		return results.WithError(err)

pkg/controller/elasticsearch/driver/upgrade_test.go

@@ -458,7 +458,8 @@ func Test_defaultDriver_maybeCompleteNodeUpgrades(t *testing.T) {
 			shutdowns:      shutdownFixture,
 			runtimeObjects: append(testSset.Pods(), testSset.BuildPtr()),
 			assertions: func(results *reconciler.Results, esClient *fakeESClient) {
-				require.True(t, esClient.DeleteShutdownCalled)
+				// shutdown will be cleaned up already outside of completeNodeUpgrades
+				require.False(t, esClient.DeleteShutdownCalled)
 				require.False(t, esClient.EnableShardAllocationCalled)
 				reconciled, _ := results.IsReconciled()
 				require.False(t, reconciled)