Ceph: fix the deduplication of tolerations collected for the drain-canary

- Implement exportable TolerationSet with the following differences:
  - Change map key to a string to ensure it is deterministic.
    The old key was a struct that included a pointer.
  - Sort the outputted list by this key to ensure the order is deterministic.
  - Get the tolerations from the deployment instead of the pod as pod tolerations
    are sometimes modified by the system. We only want user provided tolerations
    in our deployment.
- Modify tests to try duplicate keys from different structs. The old way tested duplicate
  keys using the same structs.
Signed-off-by: Rohan CJ <rohantmp@gmail.com>
(cherry picked from commit b16d55f4)

pkg/operator/ceph/disruption/controllerconfig/toleration.go

+/*
+Copyright 2019 The Rook Authors. All rights reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+	http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package controllerconfig
+
+import (
+	"fmt"
+	"sort"
+	"strings"
+
+	corev1 "k8s.io/api/core/v1"
+)
+
+// TolerationSet is a set of unique tolerations.
+type TolerationSet struct {
+	tolerations map[string]corev1.Toleration
+}
+
+// Add adds a toleration to the TolerationSet
+func (t *TolerationSet) Add(toleration corev1.Toleration) {
+	key := getKey(toleration)
+	if len(t.tolerations) == 0 {
+		t.tolerations = make(map[string]corev1.Toleration)
+	}
+	t.tolerations[key] = toleration
+}
+
+func getKey(toleration corev1.Toleration) string {
+	return fmt.Sprintf("%s-%s-%s-%s", toleration.Key, toleration.Operator, toleration.Effect, toleration.Value)
+}
+
+// ToList returns a list of all tolerations in the set. The order will always be the same for the same set.
+func (t *TolerationSet) ToList() []corev1.Toleration {
+	tolerationList := make([]corev1.Toleration, 0)
+	for _, toleration := range t.tolerations {
+		tolerationList = append(tolerationList, toleration)
+	}
+	sort.SliceStable(tolerationList, func(i, j int) bool {
+		a := getKey(tolerationList[i])
+		b := getKey(tolerationList[j])
+		return strings.Compare(a, b) == -1
+	})
+	return tolerationList
+}

pkg/operator/ceph/disruption/nodedrain/reconcile_test.go → pkg/operator/ceph/disruption/controllerconfig/toleration_test.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package nodedrain
+package controllerconfig
 
 import (
 	"testing"
@@ -24,8 +24,68 @@ import (
 	corev1 "k8s.io/api/core/v1"
 )
 
-func TestCanaryTolerations(t *testing.T) {
-	uniqueTolerationsManual := []corev1.Toleration{
+func TestTolerationSet(t *testing.T) {
+	uniqueTolerationsManualA := []corev1.Toleration{
+		// key1
+		//   exists
+		{
+			Key:      "key1",
+			Operator: corev1.TolerationOpExists,
+			Effect:   corev1.TaintEffectNoSchedule,
+		},
+		{
+			Key:      "key1",
+			Operator: corev1.TolerationOpExists,
+			Effect:   corev1.TaintEffectPreferNoSchedule,
+		},
+		//   equals with different values
+		{
+			Key:      "key1",
+			Operator: corev1.TolerationOpEqual,
+			Value:    "value1",
+			Effect:   corev1.TaintEffectNoSchedule,
+		},
+		{
+			Key:      "key1",
+			Operator: corev1.TolerationOpEqual,
+			Value:    "value2",
+			Effect:   corev1.TaintEffectNoSchedule,
+		},
+
+		//   with different effects
+		{
+			Key:      "key1",
+			Operator: corev1.TolerationOpEqual,
+			Value:    "value2",
+			Effect:   corev1.TaintEffectNoExecute,
+		},
+		// key2
+		{
+			Key:      "key2",
+			Operator: corev1.TolerationOpExists,
+			Effect:   corev1.TaintEffectNoSchedule,
+		},
+		{
+			Key:      "key2",
+			Operator: corev1.TolerationOpEqual,
+			Value:    "value1",
+			Effect:   corev1.TaintEffectNoSchedule,
+		},
+		{
+			Key:      "key2",
+			Operator: corev1.TolerationOpEqual,
+			Value:    "value2",
+			Effect:   corev1.TaintEffectNoSchedule,
+		},
+		{
+			Key:      "key2",
+			Operator: corev1.TolerationOpEqual,
+			Value:    "value2",
+			Effect:   corev1.TaintEffectNoExecute,
+		},
+	}
+	//identical to uniqueTolerationsManualA
+	uniqueTolerationsManualB := []corev1.Toleration{
 		// key1
 		//   exists
 		{
@@ -86,25 +146,25 @@ func TestCanaryTolerations(t *testing.T) {
 	}
 
 	tolerationsWithDuplicates := make([]corev1.Toleration, 0)
-	for i := range uniqueTolerationsManual {
-		tolerationsWithDuplicates = append(tolerationsWithDuplicates, uniqueTolerationsManual[i])
+	for i := range uniqueTolerationsManualA {
+		tolerationsWithDuplicates = append(tolerationsWithDuplicates, uniqueTolerationsManualA[i])
 
 		//append the previous one again if it's within range, else append the last one
 		if i > 0 {
-			tolerationsWithDuplicates = append(tolerationsWithDuplicates, uniqueTolerationsManual[i-1])
+			tolerationsWithDuplicates = append(tolerationsWithDuplicates, uniqueTolerationsManualB[i-1])
 		} else {
-			tolerationsWithDuplicates = append(tolerationsWithDuplicates, uniqueTolerationsManual[len(uniqueTolerationsManual)-1])
+			tolerationsWithDuplicates = append(tolerationsWithDuplicates, uniqueTolerationsManualB[len(uniqueTolerationsManualB)-1])
 		}
 	}
-	uniqueTolerationsMap := make(map[corev1.Toleration]struct{})
+	uniqueTolerationsMap := &TolerationSet{}
 	for _, toleration := range tolerationsWithDuplicates {
-		uniqueTolerationsMap[toleration] = struct{}{}
+		uniqueTolerationsMap.Add(toleration)
 	}
 
-	uniqueTolerations := tolerationMapToList(uniqueTolerationsMap)
+	uniqueTolerations := uniqueTolerationsMap.ToList()
 
-	assert.Equal(t, len(uniqueTolerationsManual), len(uniqueTolerations))
-	for _, tolerationI := range uniqueTolerationsManual {
+	assert.Equal(t, len(uniqueTolerationsManualA), len(uniqueTolerations))
+	for _, tolerationI := range uniqueTolerationsManualA {
 		found := false
 		for _, tolerationJ := range uniqueTolerations {
 			if tolerationI == tolerationJ {

pkg/operator/ceph/disruption/nodedrain/reconcile.go

@@ -90,22 +90,39 @@ func (r *ReconcileNode) reconcile(request reconcile.Request) (reconcile.Result,
 	}
 
 	// map with tolerations as keys and empty struct as values for uniqueness
-	uniqueTolerations := make(map[corev1.Toleration]struct{})
+	uniqueTolerations := controllerconfig.TolerationSet{}
+
 	occupiedByOSD := false
 	for _, osdPod := range osdPodList.Items {
 		if osdPod.Spec.NodeName == request.Name {
+			labels := osdPod.GetLabels()
+			deploymentList := &appsv1.DeploymentList{}
+			labelSelector := map[string]string{
+				osd.OsdIdLabelKey: labels[osd.OsdIdLabelKey],
+				k8sutil.AppAttr:   osd.AppName,
+			}
+			var deployment appsv1.Deployment
+			err := r.client.List(context.TODO(), deploymentList, client.MatchingLabels(labelSelector), client.InNamespace(osdPod.GetNamespace()))
+			if err != nil || len(deploymentList.Items) < 1 {
+				logger.Errorf("Cannot find deployment for osd id %s in namespace %s", labels[osd.OsdIdLabelKey], osdPod.GetNamespace())
+			} else {
+				deployment = deploymentList.Items[0]
+			}
+			if len(deploymentList.Items) > 1 {
+				logger.Errorf("Found multiple deployments for osd id %s in namespace %s: %+v", labels[osd.OsdIdLabelKey], osdPod.GetNamespace(), deploymentList)
+			}
+
 			occupiedByOSD = true
 			// get the osd tolerations
-			for _, osdToleration := range osdPod.Spec.Tolerations {
+			for _, osdToleration := range deployment.Spec.Template.Spec.Tolerations {
 				if osdToleration.Key == "node.kubernetes.io/unschedulable" {
-					labels := osdPod.GetLabels()
 					logger.Errorf(
 						"osd %s in namespace %s tolerates the drain taint, but the drain canary will not.",
 						labels[osd.OsdIdLabelKey],
 						labels[k8sutil.ClusterAttr],
 					)
 				} else {
-					uniqueTolerations[osdToleration] = struct{}{}
+					uniqueTolerations.Add(osdToleration)
 				}
 			}
 		}
@@ -147,7 +164,7 @@ func (r *ReconcileNode) reconcile(request reconcile.Request) (reconcile.Result,
 			Spec: corev1.PodSpec{
 				NodeSelector: nodeSelector,
 				Containers:   newDoNothingContainers(r.context.RookImage),
-				Tolerations:  tolerationMapToList(uniqueTolerations),
+				Tolerations:  uniqueTolerations.ToList(),
 			},
 		}
 
@@ -165,14 +182,6 @@ func (r *ReconcileNode) reconcile(request reconcile.Request) (reconcile.Result,
 	return reconcile.Result{}, nil
 }
 
-func tolerationMapToList(tolerationMap map[corev1.Toleration]struct{}) []corev1.Toleration {
-	tolerationList := make([]corev1.Toleration, 0)
-	for toleration := range tolerationMap {
-		tolerationList = append(tolerationList, toleration)
-	}
-	return tolerationList
-}
-
 // returns a container that does nothing
 func newDoNothingContainers(rookImage string) []corev1.Container {
 	return []corev1.Container{{