CLOUDP-193669: Make overriden tolerations take precedence

docs/RELEASE_NOTES.md

@@ -1,21 +1,13 @@
-# MongoDB Kubernetes Operator 0.8.1
+# MongoDB Kubernetes Operator 0.8.2
+
+## Kubernetes Operator
 
-## MongoDBCommunity Resource
 - Changes
-  - Connection string options
-    - The MongoDBCommunity Resource now contains a new field ```additionalConnectionStringConfig``` where connection string options can be set, and they will apply to the connection string of every user.
-    - Each user in the resource contains the same field ```additionalConnectionStringConfig``` and these options apply only for this user and will override any existing options in the resource.
-    - The following options will be ignored `replicaSet`, `tls`, `ssl`, as they are set through other means.
-    - [Sample](../config/samples/mongodb.com_v1_mongodbcommunity_additional_connection_string_options.yaml)
-  - Support for Label and Annotations Wrapper
-    - Additionally to the `specWrapper` for `statefulsets` we now support overriding `metadata.Labels` and `metadata.Annotations` via the `MetadataWrapper`.
-    - [Sample](../config/samples/arbitrary_statefulset_configuration/mongodb.com_v1_metadata.yaml)
+  - Fix a bug when overriding tolerations causing an endless reconciliation loop ([1344](https://github.com/mongodb/mongodb-kubernetes-operator/issues/1344)).
 
 ## Updated Image Tags
 
-- mongodb-kubernetes-operator:0.8.1
-- mongodb-agent:12.0.24.7719-1
-- mongodb-kubernetes-readinessprobe:1.0.15
+- mongodb-kubernetes-operator:0.8.2
 
 _All the images can be found in:_
 

pkg/kube/podtemplatespec/podspec_template_test.go

@@ -247,6 +247,140 @@ func TestMergeEnvironmentVariables(t *testing.T) {
 	assert.Equal(t, mergedContainer.Env[1].Value, "val2")
 }
 
+func TestMergeTolerations(t *testing.T) {
+	tests := []struct {
+		name                string
+		defaultTolerations  []corev1.Toleration
+		overrideTolerations []corev1.Toleration
+		expectedTolerations []corev1.Toleration
+	}{
+		{
+			// In case the calling code specifies default tolerations,
+			// they should be kept when there are no overrides.
+			name: "Overriding with nil tolerations",
+			defaultTolerations: []corev1.Toleration{
+				{
+					Key:      "key1",
+					Value:    "value1",
+					Operator: corev1.TolerationOpEqual,
+				},
+				{
+					Key:      "key1",
+					Value:    "value2",
+					Operator: corev1.TolerationOpExists,
+				},
+			},
+			overrideTolerations: nil,
+			expectedTolerations: []corev1.Toleration{
+				{
+					Key:      "key1",
+					Value:    "value1",
+					Operator: corev1.TolerationOpEqual,
+				},
+				{
+					Key:      "key1",
+					Value:    "value2",
+					Operator: corev1.TolerationOpExists,
+				},
+			},
+		},
+		{
+			// If the override is specifying an empty list of tolerations,
+			// they should replace default tolerations.
+			name: "Overriding with empty tolerations",
+			defaultTolerations: []corev1.Toleration{
+				{
+					Key:      "key1",
+					Value:    "value1",
+					Operator: corev1.TolerationOpEqual,
+				},
+			},
+			overrideTolerations: []corev1.Toleration{},
+			expectedTolerations: []corev1.Toleration{},
+		},
+		{
+			// Overriding toleration should replace a nil original toleration.
+			name:               "Overriding when default toleration is nil",
+			defaultTolerations: nil,
+			overrideTolerations: []corev1.Toleration{
+				{
+					Key:      "key1",
+					Value:    "value1",
+					Operator: corev1.TolerationOpEqual,
+				},
+				{
+					Key:      "key1",
+					Value:    "value2",
+					Operator: corev1.TolerationOpExists,
+				},
+			},
+			expectedTolerations: []corev1.Toleration{
+				{
+					Key:      "key1",
+					Value:    "value1",
+					Operator: corev1.TolerationOpEqual,
+				},
+				{
+					Key:      "key1",
+					Value:    "value2",
+					Operator: corev1.TolerationOpExists,
+				},
+			},
+		},
+		{
+			// Overriding toleration should replace any original toleration.
+			name: "Overriding when original toleration is not nil",
+			defaultTolerations: []corev1.Toleration{
+				{
+					Key:      "key1",
+					Value:    "value3",
+					Operator: corev1.TolerationOpEqual,
+				},
+				{
+					Key:      "key1",
+					Value:    "value4",
+					Operator: corev1.TolerationOpExists,
+				},
+			},
+			overrideTolerations: []corev1.Toleration{
+				{
+					Key:      "key1",
+					Value:    "value1",
+					Operator: corev1.TolerationOpEqual,
+				},
+				{
+					Key:      "key1",
+					Value:    "value2",
+					Operator: corev1.TolerationOpExists,
+				},
+			},
+			expectedTolerations: []corev1.Toleration{
+				{
+					Key:      "key1",
+					Value:    "value1",
+					Operator: corev1.TolerationOpEqual,
+				},
+				{
+					Key:      "key1",
+					Value:    "value2",
+					Operator: corev1.TolerationOpExists,
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			defaultSpec := getDefaultPodSpec()
+			defaultSpec.Spec.Tolerations = tt.defaultTolerations
+			overrideSpec := getDefaultPodSpec()
+			overrideSpec.Spec.Tolerations = tt.overrideTolerations
+
+			mergedSpec := merge.PodTemplateSpecs(defaultSpec, overrideSpec)
+			assert.Equal(t, tt.expectedTolerations, mergedSpec.Spec.Tolerations)
+		})
+	}
+}
+
 func TestMergeContainer(t *testing.T) {
 	vol0 := corev1.VolumeMount{Name: "container-0.volume-mount-0"}
 	sideCarVol := corev1.VolumeMount{Name: "container-1.volume-mount-0"}

pkg/util/merge/merge.go

@@ -503,44 +503,6 @@ func VolumeMount(original, override corev1.VolumeMount) corev1.VolumeMount {
 	return merged
 }
 
-func Tolerations(defaultTolerations, overrideTolerations []corev1.Toleration) []corev1.Toleration {
-	mergedTolerations := make([]corev1.Toleration, 0)
-	defaultMap := createTolerationsMap(defaultTolerations)
-	for _, v := range overrideTolerations {
-		if _, ok := defaultMap[v.Key]; ok {
-			defaultMap[v.Key] = append(defaultMap[v.Key], v)
-		} else {
-			defaultMap[v.Key] = []corev1.Toleration{v}
-		}
-	}
-
-	for _, v := range defaultMap {
-		mergedTolerations = append(mergedTolerations, v...)
-	}
-
-	if len(mergedTolerations) == 0 {
-		return nil
-	}
-
-	sort.SliceStable(mergedTolerations, func(i, j int) bool {
-		return mergedTolerations[i].Key < mergedTolerations[j].Key
-	})
-
-	return mergedTolerations
-}
-
-func createTolerationsMap(tolerations []corev1.Toleration) map[string][]corev1.Toleration {
-	tolerationsMap := make(map[string][]corev1.Toleration)
-	for _, t := range tolerations {
-		if _, ok := tolerationsMap[t.Key]; ok {
-			tolerationsMap[t.Key] = append(tolerationsMap[t.Key], t)
-		} else {
-			tolerationsMap[t.Key] = []corev1.Toleration{t}
-		}
-	}
-	return tolerationsMap
-}
-
 func Volumes(defaultVolumes []corev1.Volume, overrideVolumes []corev1.Volume) []corev1.Volume {
 	defaultVolumesMap := createVolumesMap(defaultVolumes)
 	overrideVolumesMap := createVolumesMap(overrideVolumes)

pkg/util/merge/merge_podtemplate_spec.go

@@ -91,7 +91,9 @@ func PodTemplateSpecs(original, override corev1.PodTemplateSpec) corev1.PodTempl
 		merged.Spec.SchedulerName = override.Spec.SchedulerName
 	}
 
-	merged.Spec.Tolerations = Tolerations(original.Spec.Tolerations, override.Spec.Tolerations)
+	if override.Spec.Tolerations != nil {
+		merged.Spec.Tolerations = override.Spec.Tolerations
+	}
 
 	merged.Spec.HostAliases = HostAliases(original.Spec.HostAliases, override.Spec.HostAliases)
 

pkg/util/merge/merge_test.go

@@ -670,82 +670,3 @@ func TestMergeHostAliases(t *testing.T) {
 	assert.Equal(t, "1.2.3.5", merged[1].IP)
 	assert.Equal(t, []string{"abc"}, merged[1].Hostnames)
 }
-
-func TestTolerations(t *testing.T) {
-	type args struct {
-		defaultTolerations  []corev1.Toleration
-		overrideTolerations []corev1.Toleration
-	}
-	tests := []struct {
-		name string
-		args args
-		want []corev1.Toleration
-	}{
-		{
-			name: "override tolerations is nil",
-			args: args{
-				defaultTolerations: []corev1.Toleration{
-					{
-						Key:      "key1",
-						Value:    "value1",
-						Operator: corev1.TolerationOpEqual,
-					},
-					{
-						Key:      "key1",
-						Value:    "value2",
-						Operator: corev1.TolerationOpExists,
-					},
-				},
-				overrideTolerations: nil,
-			},
-			want: []corev1.Toleration{
-				{
-					Key:      "key1",
-					Value:    "value1",
-					Operator: corev1.TolerationOpEqual,
-				},
-				{
-					Key:      "key1",
-					Value:    "value2",
-					Operator: corev1.TolerationOpExists,
-				},
-			},
-		},
-
-		{
-			name: "default tolerations is nil",
-			args: args{
-				defaultTolerations: nil,
-				overrideTolerations: []corev1.Toleration{
-					{
-						Key:      "key1",
-						Value:    "value1",
-						Operator: corev1.TolerationOpEqual,
-					},
-					{
-						Key:      "key1",
-						Value:    "value2",
-						Operator: corev1.TolerationOpExists,
-					},
-				},
-			},
-			want: []corev1.Toleration{
-				{
-					Key:      "key1",
-					Value:    "value1",
-					Operator: corev1.TolerationOpEqual,
-				},
-				{
-					Key:      "key1",
-					Value:    "value2",
-					Operator: corev1.TolerationOpExists,
-				},
-			},
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			assert.Equalf(t, tt.want, Tolerations(tt.args.defaultTolerations, tt.args.overrideTolerations), "Tolerations(%v, %v)", tt.args.defaultTolerations, tt.args.overrideTolerations)
-		})
-	}
-}

test/e2e/mongodbtests/mongodbtests.go

@@ -4,11 +4,12 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"sigs.k8s.io/controller-runtime/pkg/client"
 	"strings"
 	"testing"
 	"time"
 
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
 	"github.com/mongodb/mongodb-kubernetes-operator/pkg/kube/container"
 	"github.com/mongodb/mongodb-kubernetes-operator/test/e2e/util/wait"
 
@@ -643,6 +644,20 @@ func StatefulSetContainerConditionIsTrue(mdb *mdbv1.MongoDBCommunity, containerN
 	}
 }
 
+func StatefulSetConditionIsTrue(mdb *mdbv1.MongoDBCommunity, condition func(s appsv1.StatefulSet) bool) func(*testing.T) {
+	return func(t *testing.T) {
+		sts := appsv1.StatefulSet{}
+		err := e2eutil.TestClient.Get(context.TODO(), types.NamespacedName{Name: mdb.Name, Namespace: mdb.Namespace}, &sts)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if !condition(sts) {
+			t.Fatalf(`StatefulSet "%s" does not satisfy condition`, mdb.Name)
+		}
+	}
+}
+
 // PodContainerBecomesNotReady waits until the container with 'containerName' in the pod #podNum becomes not ready.
 func PodContainerBecomesNotReady(mdb *mdbv1.MongoDBCommunity, podNum int, containerName string) func(*testing.T) {
 	return func(t *testing.T) {