Improve update and contenthash logic for ToplogoSpreadConstraints

Signed-off-by: Aurel Canciu <[email protected]>

Author: relu

pkg/operator/contenthash/topology_spread_constraints.go

@@ -20,6 +20,8 @@ package contenthash
 import (
 	"crypto/md5"
 	"encoding/hex"
+	"fmt"
+	"strings"
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -42,7 +44,18 @@ func TopologySpreadConstraints(in []corev1.TopologySpreadConstraint) string {
 			labelSelectors = map[string]string{}
 		}
 		writeStringHash(h, StringMap(labelSelectors))
-
+		writeStringHash(h, strings.Join(tsc.MatchLabelKeys, ""))
+		minDomainsSeconds := "nil"
+		if tsc.MinDomains != nil {
+			minDomainsSeconds = fmt.Sprint(*tsc.MinDomains)
+		}
+		writeStringHash(h, minDomainsSeconds)
+		if tsc.NodeAffinityPolicy != nil {
+			writeStringHash(h, string(*tsc.NodeAffinityPolicy))
+		}
+		if tsc.NodeTaintsPolicy != nil {
+			writeStringHash(h, string(*tsc.NodeTaintsPolicy))
+		}
 	}
 
 	sum := h.Sum(nil)

pkg/operator/contenthash/topology_spread_constraints_test.go

@@ -0,0 +1,64 @@
+/*
+Copyright 2020 PlanetScale Inc.
+
+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 contenthash
+
+import (
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/utils/ptr"
+)
+
+func TestTopologySpreadConstraintMinDomainsSecondsNil(t *testing.T) {
+	// Make sure nil is distinguishable from 0.
+	nilHash := TopologySpreadConstraints([]corev1.TopologySpreadConstraint{
+		{MinDomains: nil},
+	})
+	zeroHash := TopologySpreadConstraints([]corev1.TopologySpreadConstraint{
+
+		{MinDomains: ptr.To(int32(0))},
+	})
+	if nilHash == zeroHash {
+		t.Errorf("nilHash = zeroHash = %v; expected different values", nilHash)
+	}
+}
+
+func TestTopologySpreadConstraintNodeAffinityPolicysNil(t *testing.T) {
+	// Make sure nil is distinguishable from 0.
+	nilHash := TopologySpreadConstraints([]corev1.TopologySpreadConstraint{
+		{NodeAffinityPolicy: nil},
+	})
+	honorHash := TopologySpreadConstraints([]corev1.TopologySpreadConstraint{
+		{NodeAffinityPolicy: ptr.To(corev1.NodeInclusionPolicyHonor)},
+	})
+	if nilHash == honorHash {
+		t.Errorf("nilHash = zeroHash = %v; expected different values", nilHash)
+	}
+}
+
+func TestTopologySpreadConstraintNodeTaintsPolicysNil(t *testing.T) {
+	// Make sure nil is distinguishable from 0.
+	nilHash := TopologySpreadConstraints([]corev1.TopologySpreadConstraint{
+		{NodeTaintsPolicy: nil},
+	})
+	honorHash := TopologySpreadConstraints([]corev1.TopologySpreadConstraint{
+		{NodeTaintsPolicy: ptr.To(corev1.NodeInclusionPolicyHonor)},
+	})
+	if nilHash == honorHash {
+		t.Errorf("nilHash = zeroHash = %v; expected different values", nilHash)
+	}
+}

pkg/operator/desiredstatehash/builder_test.go

@@ -46,6 +46,8 @@ func TestEmptyValues(t *testing.T) {
 	b.AddContainersUpdates("empty ContainersUpdates", []corev1.Container{})
 	b.AddTolerations("nil Tolerations", nil)
 	b.AddTolerations("empty Tolerations", []corev1.Toleration{})
+	b.AddTopologySpreadConstraints("nil TopologySpreadConstraints", nil)
+	b.AddTopologySpreadConstraints("empty TopologySpreadConstraints", []corev1.TopologySpreadConstraint{})
 	b.AddVolumeNames("nil VolumeNames", nil)
 	b.AddVolumeNames("empty VolumeNames", []corev1.Volume{})
 

pkg/operator/update/podspec.go

@@ -20,7 +20,6 @@ import (
 	"reflect"
 
 	corev1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 // Volumes updates entries in 'dst' based on the values in 'src'.
@@ -302,20 +301,10 @@ func TopologySpreadConstraints(dst *[]corev1.TopologySpreadConstraint, src []cor
 srcLoop:
 	for srcIndex := range src {
 		srcObj := &src[srcIndex]
-		srcMap, srcErr := metav1.LabelSelectorAsMap(srcObj.LabelSelector)
-		if srcErr != nil {
-			// There is no return of error for this function
-			// or its callers, which makes this a bit of a hack
-			srcMap = map[string]string{}
-		}
 		// If this item is already there, update it.
 		for dstIndex := range *dst {
 			dstObj := &(*dst)[dstIndex]
-			dstMap, dstErr := metav1.LabelSelectorAsMap(dstObj.LabelSelector)
-			if dstErr != nil {
-				dstMap = map[string]string{}
-			}
-			if reflect.DeepEqual(dstMap, srcMap) {
+			if reflect.DeepEqual(srcObj, dstObj) {
 				*dstObj = *srcObj
 				continue srcLoop
 			}

pkg/operator/update/podspec_test.go

@@ -41,3 +41,38 @@ func TestTolerations(t *testing.T) {
 		t.Errorf("val = %#v; want %#v", val, want)
 	}
 }
+
+func TestTopologySpreadConstraints(t *testing.T) {
+	// Make sure we don't touch tolerations that were already there.
+	val := []corev1.TopologySpreadConstraint{
+		{
+			MaxSkew:           1,
+			TopologyKey:       "alreadyExists",
+			WhenUnsatisfiable: corev1.DoNotSchedule,
+		},
+	}
+	want := []corev1.TopologySpreadConstraint{
+		{
+			MaxSkew:           1,
+			TopologyKey:       "alreadyExists",
+			WhenUnsatisfiable: corev1.DoNotSchedule,
+		},
+		{
+			MaxSkew:           2,
+			TopologyKey:       "newKey",
+			WhenUnsatisfiable: corev1.ScheduleAnyway,
+		},
+	}
+
+	TopologySpreadConstraints(&val, []corev1.TopologySpreadConstraint{
+		{
+			MaxSkew:           2,
+			TopologyKey:       "newKey",
+			WhenUnsatisfiable: corev1.ScheduleAnyway,
+		},
+	})
+
+	if !equality.Semantic.DeepEqual(val, want) {
+		t.Errorf("val = %#v; want %#v", val, want)
+	}
+}