Fix toleration comparison & merging logic

Author: tallclair

hack/.golint_failures

@@ -244,7 +244,6 @@ pkg/util/rlimit
 pkg/util/selinux
 pkg/util/sysctl/testing
 pkg/util/taints
-pkg/util/tolerations
 pkg/version/verflag
 pkg/volume
 pkg/volume/azure_dd

pkg/util/tolerations/BUILD

@@ -13,7 +13,11 @@ go_library(
         "tolerations.go",
     ],
     importpath = "k8s.io/kubernetes/pkg/util/tolerations",
-    deps = ["//pkg/apis/core:go_default_library"],
+    deps = [
+        "//pkg/apis/core:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/api/equality:go_default_library",
+        "//vendor/k8s.io/klog:go_default_library",
+    ],
 )
 
 filegroup(
@@ -33,5 +37,12 @@ go_test(
     name = "go_default_test",
     srcs = ["tolerations_test.go"],
     embed = [":go_default_library"],
-    deps = ["//pkg/apis/core:go_default_library"],
+    deps = [
+        "//pkg/apis/core:go_default_library",
+        "//pkg/apis/core/validation:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
+        "//vendor/github.com/stretchr/testify/assert:go_default_library",
+        "//vendor/github.com/stretchr/testify/require:go_default_library",
+        "//vendor/k8s.io/utils/pointer:go_default_library",
+    ],
 )

pkg/util/tolerations/tolerations.go

@@ -17,114 +17,91 @@ limitations under the License.
 package tolerations
 
 import (
+	apiequality "k8s.io/apimachinery/pkg/api/equality"
+	"k8s.io/klog"
 	api "k8s.io/kubernetes/pkg/apis/core"
 )
 
-type key struct {
-	tolerationKey string
-	effect        api.TaintEffect
-}
-
-func convertTolerationToKey(in api.Toleration) key {
-	return key{in.Key, in.Effect}
-}
-
 // VerifyAgainstWhitelist checks if the provided tolerations
 // satisfy the provided whitelist and returns true, otherwise returns false
-func VerifyAgainstWhitelist(tolerations []api.Toleration, whitelist []api.Toleration) bool {
-	if len(whitelist) == 0 {
+func VerifyAgainstWhitelist(tolerations, whitelist []api.Toleration) bool {
+	if len(whitelist) == 0 || len(tolerations) == 0 {
 		return true
 	}
 
-	t := ConvertTolerationToAMap(tolerations)
-	w := ConvertTolerationToAMap(whitelist)
-
-	for k1, v1 := range t {
-		if v2, ok := w[k1]; !ok || !AreEqual(v1, v2) {
-			return false
+next:
+	for _, t := range tolerations {
+		for _, w := range whitelist {
+			if isSuperset(w, t) {
+				continue next
+			}
 		}
+		return false
 	}
+
 	return true
 }
 
-// IsConflict returns true if the key of two tolerations match
-// but one or more other fields differ, otherwise returns false
-func IsConflict(first []api.Toleration, second []api.Toleration) bool {
-	firstMap := ConvertTolerationToAMap(first)
-	secondMap := ConvertTolerationToAMap(second)
-
-	for k1, v1 := range firstMap {
-		if v2, ok := secondMap[k1]; ok && !AreEqual(v1, v2) {
-			return true
+// MergeTolerations merges two sets of tolerations into one. If one toleration is a superset of
+// another, only the superset is kept.
+func MergeTolerations(first, second []api.Toleration) []api.Toleration {
+	all := append(first, second...)
+	merged := []api.Toleration{}
+
+next:
+	for i, t := range all {
+		for _, t2 := range merged {
+			if isSuperset(t2, t) {
+				continue next // t is redundant; ignore it
+			}
 		}
-	}
-	return false
-}
-
-// MergeTolerations merges two sets of tolerations into one
-// it does not check for conflicts
-func MergeTolerations(first []api.Toleration, second []api.Toleration) []api.Toleration {
-	var mergedTolerations []api.Toleration
-	mergedTolerations = append(mergedTolerations, second...)
-	firstMap := ConvertTolerationToAMap(first)
-	secondMap := ConvertTolerationToAMap(second)
-	// preserve order of first when merging
-	for _, v := range first {
-		k := convertTolerationToKey(v)
-		// if first contains key conflicts, the last one takes precedence
-		if _, ok := secondMap[k]; !ok && firstMap[k] == v {
-			mergedTolerations = append(mergedTolerations, v)
+		if i+1 < len(all) {
+			for _, t2 := range all[i+1:] {
+				// If the tolerations are equal, prefer the first.
+				if isSuperset(t2, t) && !apiequality.Semantic.DeepEqual(&t, &t2) {
+					continue next // t is redundant; ignore it
+				}
+			}
 		}
+		merged = append(merged, t)
 	}
-	return mergedTolerations
+
+	return merged
 }
 
-// EqualTolerations returns true if two sets of tolerations are equal, otherwise false
-// it assumes no duplicates in individual set of tolerations
-func EqualTolerations(first []api.Toleration, second []api.Toleration) bool {
-	if len(first) != len(second) {
-		return false
+// isSuperset checks whether ss tolerates a superset of t.
+func isSuperset(ss, t api.Toleration) bool {
+	if apiequality.Semantic.DeepEqual(&t, &ss) {
+		return true
 	}
 
-	firstMap := ConvertTolerationToAMap(first)
-	secondMap := ConvertTolerationToAMap(second)
-
-	for k1, v1 := range firstMap {
-		if v2, ok := secondMap[k1]; !ok || !AreEqual(v1, v2) {
-			return false
-		}
+	if t.Key != ss.Key &&
+		// An empty key with Exists operator means match all keys & values.
+		(ss.Key != "" || ss.Operator != api.TolerationOpExists) {
+		return false
 	}
-	return true
-}
 
-// ConvertTolerationToAMap converts toleration list into a map[string]api.Toleration
-func ConvertTolerationToAMap(in []api.Toleration) map[key]api.Toleration {
-	out := map[key]api.Toleration{}
-	for _, v := range in {
-		out[convertTolerationToKey(v)] = v
+	// An empty effect means match all effects.
+	if t.Effect != ss.Effect && ss.Effect != "" {
+		return false
 	}
-	return out
-}
 
-// AreEqual checks if two provided tolerations are equal or not.
-func AreEqual(first, second api.Toleration) bool {
-	if first.Key == second.Key &&
-		first.Operator == second.Operator &&
-		first.Value == second.Value &&
-		first.Effect == second.Effect &&
-		AreTolerationSecondsEqual(first.TolerationSeconds, second.TolerationSeconds) {
-		return true
+	if ss.Effect == api.TaintEffectNoExecute {
+		if ss.TolerationSeconds != nil {
+			if t.TolerationSeconds == nil ||
+				*t.TolerationSeconds > *ss.TolerationSeconds {
+				return false
+			}
+		}
 	}
-	return false
-}
 
-// AreTolerationSecondsEqual checks if two provided TolerationSeconds are equal or not.
-func AreTolerationSecondsEqual(ts1, ts2 *int64) bool {
-	if ts1 == ts2 {
-		return true
-	}
-	if ts1 != nil && ts2 != nil && *ts1 == *ts2 {
+	switch ss.Operator {
+	case api.TolerationOpEqual, "": // empty operator means Equal
+		return t.Operator == api.TolerationOpEqual && t.Value == ss.Value
+	case api.TolerationOpExists:
 		return true
+	default:
+		klog.Errorf("Unknown toleration operator: %s", ss.Operator)
+		return false
 	}
-	return false
 }