From 67f640dfc1f8a6cfefe53d80e320a4427f5e8215 Mon Sep 17 00:00:00 2001 From: sebgl Date: Mon, 2 Mar 2020 11:54:27 +0100 Subject: [PATCH 1/5] Avoid calling voting config exclusions API over and over again This commit sets up an annotation containing the last applied value for voting config exclusions. The annotation can either be: - nil: it has not been set yet - empty: the last setting applied was to reset voting config exclusions - not empty: it contains the last applied voting config exclusions Before making the ES API call, which check the value of that annotation. Users can remove the annotation at anytime, it will be set again after the next API call. Note we ignore cases where users would manipulate voting config exclusions behind our back here. --- .../version/zen2/voting_exclusions.go | 49 ++++++++++++++++++- 1 file changed, 47 insertions(+), 2 deletions(-) diff --git a/pkg/controller/elasticsearch/version/zen2/voting_exclusions.go b/pkg/controller/elasticsearch/version/zen2/voting_exclusions.go index 15da6a3f5f..dca18d07c6 100644 --- a/pkg/controller/elasticsearch/version/zen2/voting_exclusions.go +++ b/pkg/controller/elasticsearch/version/zen2/voting_exclusions.go @@ -6,6 +6,7 @@ package zen2 import ( "context" + "strings" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -19,6 +20,35 @@ 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 is set. + VotingConfigExclusionsAnnotationName = "elasticsearch.k8s.elastic.co/voting-config-exclusions" +) + +func serializeExcludedNodesForAnnotation(excludedNodes []string) string { + return strings.Join(excludedNodes, ",") +} + +// 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 +58,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 +100,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 +121,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) } From 126ff16180915e671b0fd705d7832bc689c835b8 Mon Sep 17 00:00:00 2001 From: sebgl Date: Mon, 2 Mar 2020 11:57:08 +0100 Subject: [PATCH 2/5] Add unit tests --- .../version/zen2/voting_exclusions_test.go | 161 ++++++++++++++++-- 1 file changed, 144 insertions(+), 17 deletions(-) diff --git a/pkg/controller/elasticsearch/version/zen2/voting_exclusions_test.go b/pkg/controller/elasticsearch/version/zen2/voting_exclusions_test.go index 89d5287d42..69428ca4fb 100644 --- a/pkg/controller/elasticsearch/version/zen2/voting_exclusions_test.go +++ b/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,96 @@ 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]) }) } } From be26bc3cccb262d7f111fb1b360caf114b7905b5 Mon Sep 17 00:00:00 2001 From: sebgl Date: Mon, 2 Mar 2020 11:57:27 +0100 Subject: [PATCH 3/5] Fix other unit tests requiring ES in the fake client --- .../elasticsearch/driver/downscale_test.go | 35 ++++++++----------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/pkg/controller/elasticsearch/driver/downscale_test.go b/pkg/controller/elasticsearch/driver/downscale_test.go index fcdbf595f5..93ca20b474 100644 --- a/pkg/controller/elasticsearch/driver/downscale_test.go +++ b/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,8 +803,8 @@ 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, @@ -811,8 +812,8 @@ func Test_doDownscale_zen2VotingConfigExclusions(t *testing.T) { }.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, From 3b8d01edd4032f4477652a95fe177d8546998260 Mon Sep 17 00:00:00 2001 From: sebgl Date: Mon, 2 Mar 2020 16:12:35 +0100 Subject: [PATCH 4/5] Grammatical fix --- pkg/controller/elasticsearch/version/zen2/voting_exclusions.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/elasticsearch/version/zen2/voting_exclusions.go b/pkg/controller/elasticsearch/version/zen2/voting_exclusions.go index dca18d07c6..218660d60c 100644 --- a/pkg/controller/elasticsearch/version/zen2/voting_exclusions.go +++ b/pkg/controller/elasticsearch/version/zen2/voting_exclusions.go @@ -22,7 +22,7 @@ var ( const ( // VotingConfigExclusionsAnnotationName is an annotation that stores the last applied voting config exclusions. - // An empty value means no voting config exclusions is set. + // An empty value means no voting config exclusions are set. VotingConfigExclusionsAnnotationName = "elasticsearch.k8s.elastic.co/voting-config-exclusions" ) From e084d0fea3f11082a621e23d51d4a65ffedc0171 Mon Sep 17 00:00:00 2001 From: sebgl Date: Mon, 2 Mar 2020 16:21:24 +0100 Subject: [PATCH 5/5] Sort nodes in the annotation --- .../elasticsearch/version/zen2/voting_exclusions.go | 8 +++++++- .../elasticsearch/version/zen2/voting_exclusions_test.go | 8 ++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/pkg/controller/elasticsearch/version/zen2/voting_exclusions.go b/pkg/controller/elasticsearch/version/zen2/voting_exclusions.go index 218660d60c..e5adfd48b5 100644 --- a/pkg/controller/elasticsearch/version/zen2/voting_exclusions.go +++ b/pkg/controller/elasticsearch/version/zen2/voting_exclusions.go @@ -6,6 +6,7 @@ package zen2 import ( "context" + "sort" "strings" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -26,8 +27,13 @@ const ( VotingConfigExclusionsAnnotationName = "elasticsearch.k8s.elastic.co/voting-config-exclusions" ) +// serializeExcludedNodesForAnnotation returns a sorted comma-separated representation of the given slice. func serializeExcludedNodesForAnnotation(excludedNodes []string) string { - return strings.Join(excludedNodes, ",") + // 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 diff --git a/pkg/controller/elasticsearch/version/zen2/voting_exclusions_test.go b/pkg/controller/elasticsearch/version/zen2/voting_exclusions_test.go index 69428ca4fb..b24821e104 100644 --- a/pkg/controller/elasticsearch/version/zen2/voting_exclusions_test.go +++ b/pkg/controller/elasticsearch/version/zen2/voting_exclusions_test.go @@ -219,3 +219,11 @@ func TestAddToVotingConfigExclusions(t *testing.T) { }) } } + +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) +}