Do not call the voting config exclusions API at every single reconciliation

pkg/controller/elasticsearch/driver/downscale_test.go

@@ -34,7 +34,13 @@ import (
 
 // Sample StatefulSets to use in tests
 var (
-	clusterName         = "cluster-name"
+	clusterName = "cluster-name"
+	es          = esv1.Elasticsearch{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      clusterName,
+			Namespace: "ns",
+		},
+	}
 	ssetMaster3Replicas = sset.TestSset{
 		Name:      "ssetMaster3Replicas",
 		Namespace: "ns",
@@ -118,7 +124,7 @@ var (
 			Ready:           true,
 		}.Build(),
 	}
-	runtimeObjs = []runtime.Object{&ssetMaster3Replicas, &ssetData4Replicas,
+	runtimeObjs = []runtime.Object{&es, &ssetMaster3Replicas, &ssetData4Replicas,
 		&podsSsetMaster3Replicas[0], &podsSsetMaster3Replicas[1], &podsSsetMaster3Replicas[2],
 		&podsSsetData4Replicas[0], &podsSsetData4Replicas[1], &podsSsetData4Replicas[2], &podsSsetData4Replicas[3],
 	}
@@ -147,13 +153,8 @@ func TestHandleDownscale(t *testing.T) {
 				{Index: "index-1", Shard: "0", State: esclient.STARTED, NodeName: "ssetData4Replicas-2"},
 			},
 		),
-		esClient: esClient,
-		es: esv1.Elasticsearch{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      clusterName,
-				Namespace: "ns",
-			},
-		},
+		esClient:  esClient,
+		es:        es,
 		parentCtx: context.Background(),
 	}
 
@@ -802,17 +803,17 @@ func Test_doDownscale_updateReplicasAndExpectations(t *testing.T) {
 func Test_doDownscale_zen2VotingConfigExclusions(t *testing.T) {
 	ssetMasters := sset.TestSset{
 		Name:        "masters",
-		Namespace:   "ns",
-		ClusterName: "es",
+		Namespace:   es.Namespace,
+		ClusterName: es.Name,
 		Version:     "7.1.0",
 		Replicas:    3,
 		Master:      true,
 		Data:        false,
 	}.Build()
 	ssetData := sset.TestSset{
 		Name:        "datas",
-		Namespace:   "ns",
-		ClusterName: "es",
+		Namespace:   es.Namespace,
+		ClusterName: es.Name,
 		Version:     "7.1.0",
 		Replicas:    3,
 		Master:      false,
@@ -847,12 +848,6 @@ func Test_doDownscale_zen2VotingConfigExclusions(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			es := esv1.Elasticsearch{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: ssetMasters.Namespace,
-					Name:      "es",
-				},
-			}
 			// simulate an existing v7 master for zen2 to be called
 			v7Pod := corev1.Pod{
 				ObjectMeta: metav1.ObjectMeta{
@@ -865,7 +860,7 @@ func Test_doDownscale_zen2VotingConfigExclusions(t *testing.T) {
 					},
 				},
 			}
-			k8sClient := k8s.WrappedFakeClient(&ssetMasters, &ssetData, &v7Pod)
+			k8sClient := k8s.WrappedFakeClient(es.DeepCopy(), &ssetMasters, &ssetData, &v7Pod)
 			esClient := &fakeESClient{}
 			downscaleCtx := downscaleContext{
 				k8sClient:      k8sClient,

pkg/controller/elasticsearch/version/zen2/voting_exclusions.go

@@ -6,6 +6,8 @@ package zen2
 
 import (
 	"context"
+	"sort"
+	"strings"
 
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 
@@ -19,6 +21,40 @@ var (
 	log = logf.Log.WithName("zen2")
 )
 
+const (
+	// VotingConfigExclusionsAnnotationName is an annotation that stores the last applied voting config exclusions.
+	// An empty value means no voting config exclusions are set.
+	VotingConfigExclusionsAnnotationName = "elasticsearch.k8s.elastic.co/voting-config-exclusions"
+)
+
+// serializeExcludedNodesForAnnotation returns a sorted comma-separated representation of the given slice.
+func serializeExcludedNodesForAnnotation(excludedNodes []string) string {
+	// sort a copy to not mutate the given slice
+	sliceCopy := make([]string, len(excludedNodes))
+	copy(sliceCopy, excludedNodes)
+	sort.Strings(sliceCopy)
+	return strings.Join(sliceCopy, ",")
+}
+
+// votingConfigAnnotationMatches returns true if the voting config exclusions annotation value
+// matches the given excluded nodes.
+func votingConfigAnnotationMatches(es esv1.Elasticsearch, excludedNodes []string) bool {
+	value, exists := es.Annotations[VotingConfigExclusionsAnnotationName]
+	if !exists {
+		return false
+	}
+	return value == serializeExcludedNodesForAnnotation(excludedNodes)
+}
+
+// setVotingConfigAnnotation sets the value of the voting config exclusions annotation to the given excluded nodes.
+func setVotingConfigAnnotation(c k8s.Client, es esv1.Elasticsearch, excludedNodes []string) error {
+	if es.Annotations == nil {
+		es.Annotations = map[string]string{}
+	}
+	es.Annotations[VotingConfigExclusionsAnnotationName] = serializeExcludedNodesForAnnotation(excludedNodes)
+	return c.Update(&es)
+}
+
 // AddToVotingConfigExclusions adds the given node names to exclude from voting config exclusions.
 func AddToVotingConfigExclusions(ctx context.Context, c k8s.Client, esClient client.Client, es esv1.Elasticsearch, excludeNodes []string) error {
 	compatible, err := AllMastersCompatibleWithZen2(c, es)
@@ -28,13 +64,20 @@ func AddToVotingConfigExclusions(ctx context.Context, c k8s.Client, esClient cli
 	if !compatible {
 		return nil
 	}
+
+	if votingConfigAnnotationMatches(es, excludeNodes) {
+		// nothing to do, we already applied that setting
+		return nil
+	}
+
 	log.Info("Setting voting config exclusions", "namespace", es.Namespace, "nodes", excludeNodes)
 	ctx, cancel := context.WithTimeout(ctx, client.DefaultReqTimeout)
 	defer cancel()
 	if err := esClient.AddVotingConfigExclusions(ctx, excludeNodes, ""); err != nil {
 		return err
 	}
-	return nil
+	// store the excluded nodes value in an annotation so we don't perform the same API call over and over again
+	return setVotingConfigAnnotation(c, es, excludeNodes)
 }
 
 // canClearVotingConfigExclusions returns true if it is safe to clear voting config exclusions.
@@ -63,6 +106,12 @@ func ClearVotingConfigExclusions(ctx context.Context, es esv1.Elasticsearch, c k
 		return false, nil
 	}
 
+	var noExcludedNodes []string = nil
+	if votingConfigAnnotationMatches(es, noExcludedNodes) {
+		// nothing to do, we already applied that setting
+		return false, nil
+	}
+
 	canClear, err := canClearVotingConfigExclusions(c, actualStatefulSets)
 	if err != nil {
 		return false, err
@@ -78,5 +127,7 @@ func ClearVotingConfigExclusions(ctx context.Context, es esv1.Elasticsearch, c k
 	if err := esClient.DeleteVotingConfigExclusions(ctx, false); err != nil {
 		return false, err
 	}
-	return false, nil
+
+	// store the excluded nodes value in an annotation so we don't perform the same API call over and over again
+	return false, setVotingConfigAnnotation(c, es, noExcludedNodes)
 }

pkg/controller/elasticsearch/version/zen2/voting_exclusions_test.go

@@ -18,7 +18,8 @@ import (
 )
 
 type fakeVotingConfigExclusionsESClient struct {
-	called bool
+	called        bool
+	excludedNodes []string
 	client.Client
 }
 
@@ -27,6 +28,20 @@ func (f *fakeVotingConfigExclusionsESClient) DeleteVotingConfigExclusions(ctx co
 	return nil
 }
 
+func (f *fakeVotingConfigExclusionsESClient) AddVotingConfigExclusions(ctx context.Context, nodeNames []string, timeout string) error {
+	f.called = true
+	f.excludedNodes = nodeNames
+	return nil
+}
+
+func withVotingConfigAnnotation(es esv1.Elasticsearch, value string) *esv1.Elasticsearch {
+	clone := es.DeepCopy()
+	clone.Annotations = map[string]string{
+		VotingConfigExclusionsAnnotationName: value,
+	}
+	return clone
+}
+
 func Test_ClearVotingConfigExclusions(t *testing.T) {
 	// dummy statefulset with 3 pods
 	statefulSet3rep := sset.TestSset{Name: "nodes", Version: "7.2.0", Replicas: 3, Master: true, Data: true}.Build()
@@ -45,38 +60,64 @@ func Test_ClearVotingConfigExclusions(t *testing.T) {
 	// simulate 2 pods out of the 3
 	statefulSet2rep := sset.TestSset{Name: "nodes", Version: "7.2.0", Replicas: 2, Master: true, Data: true}.Build()
 	tests := []struct {
-		name               string
-		c                  k8s.Client
-		actualStatefulSets sset.StatefulSetList
-		wantCall           bool
-		wantRequeue        bool
+		name                       string
+		c                          k8s.Client
+		es                         *esv1.Elasticsearch
+		actualStatefulSets         sset.StatefulSetList
+		wantCall                   bool
+		wantRequeue                bool
+		wantVotingConfigAnnotation string
 	}{
 		{
 			name: "no v7 nodes",
-			c:    k8s.WrappedFakeClient(),
+			c:    k8s.WrappedFakeClient(&es),
+			es:   &es,
 			actualStatefulSets: sset.StatefulSetList{
 				createStatefulSetWithESVersion("6.8.0"),
 			},
-			wantCall:    false,
-			wantRequeue: false,
+			wantCall:                   false,
+			wantRequeue:                false,
+			wantVotingConfigAnnotation: "",
 		},
 		{
-			name:               "3/3 nodes there: can clear",
-			c:                  k8s.WrappedFakeClient(&statefulSet3rep, &pods[0], &pods[1], &pods[2]),
-			actualStatefulSets: sset.StatefulSetList{statefulSet3rep},
-			wantCall:           true,
-			wantRequeue:        false,
+			name:                       "3/3 nodes there, no annotation set: should clear",
+			c:                          k8s.WrappedFakeClient(&es, &statefulSet3rep, &pods[0], &pods[1], &pods[2]),
+			es:                         &es,
+			actualStatefulSets:         sset.StatefulSetList{statefulSet3rep},
+			wantCall:                   true,
+			wantRequeue:                false,
+			wantVotingConfigAnnotation: "",
+		},
+		{
+			name:                       "3/3 nodes there, annotation already set, should do nothing",
+			c:                          k8s.WrappedFakeClient(withVotingConfigAnnotation(es, ""), &statefulSet3rep, &pods[0], &pods[1], &pods[2]),
+			es:                         withVotingConfigAnnotation(es, ""),
+			actualStatefulSets:         sset.StatefulSetList{statefulSet3rep},
+			wantCall:                   false,
+			wantRequeue:                false,
+			wantVotingConfigAnnotation: "",
+		},
+		{
+			name:                       "3/3 nodes there, annotation set to the wrong value, should clear",
+			c:                          k8s.WrappedFakeClient(withVotingConfigAnnotation(es, "node1"), &statefulSet3rep, &pods[0], &pods[1], &pods[2]),
+			es:                         withVotingConfigAnnotation(es, "node1"),
+			actualStatefulSets:         sset.StatefulSetList{statefulSet3rep},
+			wantCall:                   true,
+			wantRequeue:                false,
+			wantVotingConfigAnnotation: "",
 		},
 		{
 			name:               "2/3 nodes there: cannot clear, should requeue",
-			c:                  k8s.WrappedFakeClient(&statefulSet3rep, &pods[0], &pods[1]),
+			c:                  k8s.WrappedFakeClient(&es, &statefulSet3rep, &pods[0], &pods[1]),
+			es:                 &es,
 			actualStatefulSets: sset.StatefulSetList{statefulSet3rep},
 			wantCall:           false,
 			wantRequeue:        true,
 		},
 		{
 			name:               "3/2 nodes there: cannot clear, should requeue",
-			c:                  k8s.WrappedFakeClient(&statefulSet2rep, &pods[0], &pods[1], &pods[2]),
+			es:                 &es,
+			c:                  k8s.WrappedFakeClient(&es, &statefulSet2rep, &pods[0], &pods[1], &pods[2]),
 			actualStatefulSets: sset.StatefulSetList{statefulSet2rep},
 			wantCall:           false,
 			wantRequeue:        true,
@@ -85,10 +126,104 @@ func Test_ClearVotingConfigExclusions(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			clientMock := &fakeVotingConfigExclusionsESClient{}
-			requeue, err := ClearVotingConfigExclusions(context.Background(), es, tt.c, clientMock, tt.actualStatefulSets)
+			requeue, err := ClearVotingConfigExclusions(context.Background(), *tt.es, tt.c, clientMock, tt.actualStatefulSets)
 			require.NoError(t, err)
 			require.Equal(t, tt.wantRequeue, requeue)
 			require.Equal(t, tt.wantCall, clientMock.called)
+			var retrievedES esv1.Elasticsearch
+			err = tt.c.Get(k8s.ExtractNamespacedName(tt.es), &retrievedES)
+			require.NoError(t, err)
+			require.Equal(t, tt.wantVotingConfigAnnotation, retrievedES.Annotations[VotingConfigExclusionsAnnotationName])
 		})
 	}
 }
+
+func TestAddToVotingConfigExclusions(t *testing.T) {
+	es := esv1.Elasticsearch{ObjectMeta: metav1.ObjectMeta{Name: "es", Namespace: "ns"}}
+	masterPod := sset.TestPod{
+		Namespace:   "ns",
+		Name:        "pod-name",
+		ClusterName: "es",
+		Version:     "7.2.0",
+		Master:      true,
+	}.BuildPtr()
+	tests := []struct {
+		name                       string
+		es                         *esv1.Elasticsearch
+		c                          k8s.Client
+		excludeNodes               []string
+		wantAPICalled              bool
+		wantAPICalledWith          []string
+		wantVotingConfigAnnotation string
+	}{
+		{
+			name: "some zen1 masters: do nothing",
+			es:   &es,
+			c: k8s.WrappedFakeClient(&es, sset.TestPod{
+				Namespace:   "ns",
+				Name:        "pod-name",
+				ClusterName: "es",
+				Version:     "6.8.0",
+				Master:      true,
+			}.BuildPtr()),
+			excludeNodes:  []string{"node1"},
+			wantAPICalled: false,
+		},
+		{
+			name:                       "setting already applied based on annotation: do nothing",
+			es:                         withVotingConfigAnnotation(es, "node1,node2"),
+			c:                          k8s.WrappedFakeClient(withVotingConfigAnnotation(es, "node1,node2")),
+			excludeNodes:               []string{"node1", "node2"},
+			wantAPICalled:              false,
+			wantVotingConfigAnnotation: "node1,node2",
+		},
+		{
+			name:                       "no annotation: set voting config exclusions",
+			es:                         &es,
+			c:                          k8s.WrappedFakeClient(&es, masterPod),
+			excludeNodes:               []string{"node1", "node2"},
+			wantAPICalled:              true,
+			wantAPICalledWith:          []string{"node1", "node2"},
+			wantVotingConfigAnnotation: "node1,node2",
+		},
+		{
+			name:                       "empty annotation: set voting config exclusions",
+			es:                         withVotingConfigAnnotation(es, ""),
+			c:                          k8s.WrappedFakeClient(withVotingConfigAnnotation(es, ""), masterPod),
+			excludeNodes:               []string{"node1", "node2"},
+			wantAPICalled:              true,
+			wantAPICalledWith:          []string{"node1", "node2"},
+			wantVotingConfigAnnotation: "node1,node2",
+		},
+		{
+			name:                       "annotation mismatch: set voting config exclusions",
+			es:                         withVotingConfigAnnotation(es, "node1"),
+			c:                          k8s.WrappedFakeClient(withVotingConfigAnnotation(es, "node1"), masterPod),
+			excludeNodes:               []string{"node1", "node2"},
+			wantAPICalled:              true,
+			wantAPICalledWith:          []string{"node1", "node2"},
+			wantVotingConfigAnnotation: "node1,node2",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			clientMock := &fakeVotingConfigExclusionsESClient{}
+			err := AddToVotingConfigExclusions(context.Background(), tt.c, clientMock, *tt.es, tt.excludeNodes)
+			require.NoError(t, err)
+			require.Equal(t, tt.wantAPICalled, clientMock.called)
+			require.Equal(t, tt.wantAPICalledWith, clientMock.excludedNodes)
+			var retrievedES esv1.Elasticsearch
+			err = tt.c.Get(k8s.ExtractNamespacedName(tt.es), &retrievedES)
+			require.NoError(t, err)
+			require.Equal(t, tt.wantVotingConfigAnnotation, retrievedES.Annotations[VotingConfigExclusionsAnnotationName])
+		})
+	}
+}
+
+func Test_serializeExcludedNodesForAnnotation1(t *testing.T) {
+	nodes := []string{"nodeA", "nodeC", "nodeB"}
+	// should be sorted alphabetically in a single comma-separated string
+	require.Equal(t, "nodeA,nodeB,nodeC", serializeExcludedNodesForAnnotation(nodes))
+	// initial slice should not be mutated
+	require.Equal(t, []string{"nodeA", "nodeC", "nodeB"}, nodes)
+}