Merge pull request #577 from Miciah/BZ1900819-specify-topology-spread-constraints

Bug 1900819: Specify topology spread constraints

pkg/operator/controller/ingress/deployment.go

@@ -209,7 +209,7 @@ func desiredRouterDeployment(ci *operatorv1.IngressController, ingressController
 	}
 	deployment.Spec.Replicas = &desiredReplicas
 
-	needDeploymentHash := false
+	configureAffinity := false
 	switch ci.Status.EndpointPublishingStrategy.Type {
 	case operatorv1.HostNetworkStrategyType:
 		// Typically, an ingress controller will be scaled with replicas
@@ -272,7 +272,7 @@ func desiredRouterDeployment(ci *operatorv1.IngressController, ingressController
 		// that a node that had local endpoints at the start of a
 		// rolling update continues to have local endpoints for the
 		// duration of and at the completion of the update.
-		needDeploymentHash = true
+		configureAffinity = true
 		deployment.Spec.Template.Spec.Affinity = &corev1.Affinity{
 			PodAffinity: &corev1.PodAffinity{
 				PreferredDuringSchedulingIgnoredDuringExecution: []corev1.WeightedPodAffinityTerm{
@@ -298,6 +298,10 @@ func desiredRouterDeployment(ci *operatorv1.IngressController, ingressController
 					},
 				},
 			},
+			// TODO: Once https://issues.redhat.com/browse/RFE-1759
+			// is implemented, replace
+			// "RequiredDuringSchedulingIgnoredDuringExecution" with
+			// "PreferredDuringSchedulingIgnoredDuringExecution".
 			PodAntiAffinity: &corev1.PodAntiAffinity{
 				RequiredDuringSchedulingIgnoredDuringExecution: []corev1.PodAffinityTerm{
 					{
@@ -322,6 +326,27 @@ func desiredRouterDeployment(ci *operatorv1.IngressController, ingressController
 		}
 	}
 
+	// Configure topology constraints to spread replicas across availability
+	// zones.  We want to allow scheduling more replicas than there are AZs,
+	// so we specify "ScheduleAnyway".  We want to allow scheduling a
+	// newer-generation replica on the same node as an older-generation
+	// replica where the deployment strategy allows and depends on doing so,
+	// so we specify a label selector with the deployment's hash.
+	deployment.Spec.Template.Spec.TopologySpreadConstraints = []corev1.TopologySpreadConstraint{{
+		MaxSkew:           int32(1),
+		TopologyKey:       corev1.LabelTopologyZone,
+		WhenUnsatisfiable: corev1.ScheduleAnyway,
+		LabelSelector: &metav1.LabelSelector{
+			MatchExpressions: []metav1.LabelSelectorRequirement{
+				{
+					Key:      controller.ControllerDeploymentHashLabel,
+					Operator: metav1.LabelSelectorOpIn,
+					// Values is set at the end of this function.
+				},
+			},
+		},
+	}}
+
 	statsSecretName := fmt.Sprintf("router-stats-%s", ci.Name)
 	env := []corev1.EnvVar{
 		{Name: "ROUTER_SERVICE_NAME", Value: ci.Name},
@@ -710,13 +735,14 @@ func desiredRouterDeployment(ci *operatorv1.IngressController, ingressController
 	deployment.Spec.Template.Spec.Containers[0].VolumeMounts = routerVolumeMounts
 	deployment.Spec.Template.Spec.Containers[0].Env = append(deployment.Spec.Template.Spec.Containers[0].Env, env...)
 
-	// If the deployment needs a hash for the affinity policy, we must
-	// compute it now, after all the other fields have been computed, and
-	// inject it into the appropriate fields.
-	if needDeploymentHash {
-		hash := deploymentTemplateHash(deployment)
-		deployment.Spec.Template.Labels[controller.ControllerDeploymentHashLabel] = hash
-		values := []string{hash}
+	// Compute the hash for topology spread constraints and possibly
+	// affinity policy now, after all the other fields have been computed,
+	// and inject it into the appropriate fields.
+	hash := deploymentTemplateHash(deployment)
+	deployment.Spec.Template.Labels[controller.ControllerDeploymentHashLabel] = hash
+	values := []string{hash}
+	deployment.Spec.Template.Spec.TopologySpreadConstraints[0].LabelSelector.MatchExpressions[0].Values = values
+	if configureAffinity {
 		deployment.Spec.Template.Spec.Affinity.PodAffinity.PreferredDuringSchedulingIgnoredDuringExecution[0].PodAffinityTerm.LabelSelector.MatchExpressions[1].Values = values
 		deployment.Spec.Template.Spec.Affinity.PodAntiAffinity.RequiredDuringSchedulingIgnoredDuringExecution[0].LabelSelector.MatchExpressions[1].Values = values
 	}
@@ -866,37 +892,11 @@ func hashableDeployment(deployment *appsv1.Deployment, onlyTemplate bool) *appsv
 	// trigger a rolling update of the deployment.
 	affinity := deployment.Spec.Template.Spec.Affinity.DeepCopy()
 	if affinity != nil {
-		cmpMatchExpressions := func(a, b metav1.LabelSelectorRequirement) bool {
-			if a.Key != b.Key {
-				return a.Key < b.Key
-			}
-			if a.Operator != b.Operator {
-				return a.Operator < b.Operator
-			}
-			for i := range b.Values {
-				if i == len(a.Values) {
-					return true
-				}
-				if a.Values[i] != b.Values[i] {
-					return a.Values[i] < b.Values[i]
-				}
-			}
-			return false
-		}
-
 		if affinity.PodAffinity != nil {
 			terms := affinity.PodAffinity.PreferredDuringSchedulingIgnoredDuringExecution
 			for _, term := range terms {
 				labelSelector := term.PodAffinityTerm.LabelSelector
-				if labelSelector != nil {
-					for i, expr := range labelSelector.MatchExpressions {
-						if expr.Key == controller.ControllerDeploymentHashLabel {
-							// Hash value should be ignored.
-							labelSelector.MatchExpressions[i].Values = nil
-						}
-					}
-
-				}
+				zeroOutDeploymentHash(labelSelector)
 				exprs := labelSelector.MatchExpressions
 				sort.Slice(exprs, func(i, j int) bool {
 					return cmpMatchExpressions(exprs[i], exprs[j])
@@ -906,15 +906,7 @@ func hashableDeployment(deployment *appsv1.Deployment, onlyTemplate bool) *appsv
 		if affinity.PodAntiAffinity != nil {
 			terms := affinity.PodAntiAffinity.RequiredDuringSchedulingIgnoredDuringExecution
 			for _, term := range terms {
-				if term.LabelSelector != nil {
-					for i, expr := range term.LabelSelector.MatchExpressions {
-						if expr.Key == controller.ControllerDeploymentHashLabel {
-							// Hash value should be ignored.
-							term.LabelSelector.MatchExpressions[i].Values = nil
-						}
-					}
-
-				}
+				zeroOutDeploymentHash(term.LabelSelector)
 				exprs := term.LabelSelector.MatchExpressions
 				sort.Slice(exprs, func(i, j int) bool {
 					return cmpMatchExpressions(exprs[i], exprs[j])
@@ -936,6 +928,17 @@ func hashableDeployment(deployment *appsv1.Deployment, onlyTemplate bool) *appsv
 		return tolerations[i].Key < tolerations[j].Key || tolerations[i].Operator < tolerations[j].Operator || tolerations[i].Value < tolerations[j].Value || tolerations[i].Effect < tolerations[j].Effect
 	})
 	hashableDeployment.Spec.Template.Spec.Tolerations = tolerations
+	topologySpreadConstraints := make([]corev1.TopologySpreadConstraint, len(deployment.Spec.Template.Spec.TopologySpreadConstraints))
+	for i, constraint := range deployment.Spec.Template.Spec.TopologySpreadConstraints {
+		topologySpreadConstraints[i] = *constraint.DeepCopy()
+		labelSelector := topologySpreadConstraints[i].LabelSelector
+		zeroOutDeploymentHash(labelSelector)
+		exprs := labelSelector.MatchExpressions
+		sort.Slice(exprs, func(i, j int) bool {
+			return cmpMatchExpressions(exprs[i], exprs[j])
+		})
+	}
+	hashableDeployment.Spec.Template.Spec.TopologySpreadConstraints = topologySpreadConstraints
 	hashableDeployment.Spec.Template.Spec.NodeSelector = deployment.Spec.Template.Spec.NodeSelector
 	containers := make([]corev1.Container, len(deployment.Spec.Template.Spec.Containers))
 	for i, container := range deployment.Spec.Template.Spec.Containers {
@@ -1004,6 +1007,37 @@ func hashableDeployment(deployment *appsv1.Deployment, onlyTemplate bool) *appsv
 	return &hashableDeployment
 }
 
+// cmpMatchExpressions is a helper for hashableDeployment.
+func cmpMatchExpressions(a, b metav1.LabelSelectorRequirement) bool {
+	if a.Key != b.Key {
+		return a.Key < b.Key
+	}
+	if a.Operator != b.Operator {
+		return a.Operator < b.Operator
+	}
+	for i := range b.Values {
+		if i == len(a.Values) {
+			return true
+		}
+		if a.Values[i] != b.Values[i] {
+			return a.Values[i] < b.Values[i]
+		}
+	}
+	return false
+}
+
+// zeroOutDeploymentHash is a helper for hashableDeployment.
+func zeroOutDeploymentHash(labelSelector *metav1.LabelSelector) {
+	if labelSelector != nil {
+		for i, expr := range labelSelector.MatchExpressions {
+			if expr.Key == controller.ControllerDeploymentHashLabel {
+				// Hash value should be ignored.
+				labelSelector.MatchExpressions[i].Values = nil
+			}
+		}
+	}
+}
+
 // hashableProbe returns a copy of the given probe with exactly the fields that
 // should be used for computing a deployment's hash copied over.  Fields that
 // should be ignored, or that have explicit values that are equal to their
@@ -1137,6 +1171,7 @@ func deploymentConfigChanged(current, expected *appsv1.Deployment) (bool, *appsv
 	updated.Spec.Template.Spec.Containers[0].StartupProbe = expected.Spec.Template.Spec.Containers[0].StartupProbe
 	updated.Spec.Template.Spec.Containers[0].VolumeMounts = expected.Spec.Template.Spec.Containers[0].VolumeMounts
 	updated.Spec.Template.Spec.Tolerations = expected.Spec.Template.Spec.Tolerations
+	updated.Spec.Template.Spec.TopologySpreadConstraints = expected.Spec.Template.Spec.TopologySpreadConstraints
 	updated.Spec.Template.Spec.Affinity = expected.Spec.Template.Spec.Affinity
 	replicas := int32(1)
 	if expected.Spec.Replicas != nil {

pkg/operator/controller/ingress/deployment_test.go

@@ -92,6 +92,17 @@ func checkDeploymentHasContainer(t *testing.T, deployment *appsv1.Deployment, na
 	}
 }
 
+func checkDeploymentHash(t *testing.T, deployment *appsv1.Deployment) {
+	t.Helper()
+	expectedHash := deploymentTemplateHash(deployment)
+	actualHash, haveHashLabel := deployment.Spec.Template.Labels[controller.ControllerDeploymentHashLabel]
+	if !haveHashLabel {
+		t.Error("router Deployment is missing hash label")
+	} else if actualHash != expectedHash {
+		t.Errorf("router Deployment has wrong hash; expected: %s, got: %s", expectedHash, actualHash)
+	}
+}
+
 func checkRollingUpdateParams(t *testing.T, deployment *appsv1.Deployment, maxUnavailable intstr.IntOrString, maxSurge intstr.IntOrString) {
 	t.Helper()
 
@@ -182,13 +193,7 @@ func TestDesiredRouterDeployment(t *testing.T) {
 		t.Errorf("invalid router Deployment: %v", err)
 	}
 
-	expectedHash := deploymentTemplateHash(deployment)
-	actualHash, haveHashLabel := deployment.Spec.Template.Labels[controller.ControllerDeploymentHashLabel]
-	if !haveHashLabel {
-		t.Error("router Deployment is missing hash label")
-	} else if actualHash != expectedHash {
-		t.Errorf("router Deployment has wrong hash; expected: %s, got: %s", expectedHash, actualHash)
-	}
+	checkDeploymentHash(t, deployment)
 
 	checkDeploymentHasEnvVar(t, deployment, WildcardRouteAdmissionPolicy, true, "false")
 
@@ -345,13 +350,7 @@ func TestDesiredRouterDeployment(t *testing.T) {
 	if err != nil {
 		t.Errorf("invalid router Deployment: %v", err)
 	}
-	expectedHash = deploymentTemplateHash(deployment)
-	actualHash, haveHashLabel = deployment.Spec.Template.Labels[controller.ControllerDeploymentHashLabel]
-	if !haveHashLabel {
-		t.Error("router Deployment is missing hash label")
-	} else if actualHash != expectedHash {
-		t.Errorf("router Deployment has wrong hash; expected: %s, got: %s", expectedHash, actualHash)
-	}
+	checkDeploymentHash(t, deployment)
 	checkRollingUpdateParams(t, deployment, intstr.FromString("25%"), intstr.FromString("25%"))
 	if deployment.Spec.Template.Spec.HostNetwork != false {
 		t.Error("expected host network to be false")
@@ -423,13 +422,7 @@ func TestDesiredRouterDeployment(t *testing.T) {
 	if err != nil {
 		t.Errorf("invalid router Deployment: %v", err)
 	}
-	expectedHash = deploymentTemplateHash(deployment)
-	actualHash, haveHashLabel = deployment.Spec.Template.Labels[controller.ControllerDeploymentHashLabel]
-	if !haveHashLabel {
-		t.Error("router Deployment is missing hash label")
-	} else if actualHash != expectedHash {
-		t.Errorf("router Deployment has wrong hash; expected: %s, got: %s", expectedHash, actualHash)
-	}
+	checkDeploymentHash(t, deployment)
 	checkRollingUpdateParams(t, deployment, intstr.FromString("25%"), intstr.FromString("25%"))
 	if deployment.Spec.Template.Spec.HostNetwork != false {
 		t.Error("expected host network to be false")
@@ -513,10 +506,7 @@ func TestDesiredRouterDeployment(t *testing.T) {
 	if err != nil {
 		t.Errorf("invalid router Deployment: %v", err)
 	}
-	actualHash, haveHashLabel = deployment.Spec.Template.Labels[controller.ControllerDeploymentHashLabel]
-	if haveHashLabel {
-		t.Errorf("router Deployment has unexpected hash label: %s", actualHash)
-	}
+	checkDeploymentHash(t, deployment)
 	if len(deployment.Spec.Template.Spec.NodeSelector) != 1 ||
 		deployment.Spec.Template.Spec.NodeSelector["xyzzy"] != "quux" {
 		t.Errorf("router Deployment has unexpected node selector: %#v",
@@ -804,6 +794,27 @@ func TestDeploymentConfigChanged(t *testing.T) {
 			},
 			expect: false,
 		},
+		{
+			description: "if .spec.template.spec.topologySpreadConstraints.maxSkew changes",
+			mutate: func(deployment *appsv1.Deployment) {
+				deployment.Spec.Template.Spec.TopologySpreadConstraints[0].MaxSkew = int32(2)
+			},
+			expect: true,
+		},
+		{
+			description: "if .spec.template.spec.topologySpreadConstraints is zeroed",
+			mutate: func(deployment *appsv1.Deployment) {
+				deployment.Spec.Template.Spec.TopologySpreadConstraints = []corev1.TopologySpreadConstraint{}
+			},
+			expect: true,
+		},
+		{
+			description: "if the hash in .spec.template.spec.topologySpreadConstraints changes",
+			mutate: func(deployment *appsv1.Deployment) {
+				deployment.Spec.Template.Spec.TopologySpreadConstraints[0].LabelSelector.MatchExpressions[0].Values = []string{"xyz"}
+			},
+			expect: false,
+		},
 		{
 			description: "if ROUTER_CANONICAL_HOSTNAME changes",
 			mutate: func(deployment *appsv1.Deployment) {
@@ -1154,6 +1165,20 @@ func TestDeploymentConfigChanged(t *testing.T) {
 							},
 						},
 						Tolerations: []corev1.Toleration{toleration, otherToleration},
+						TopologySpreadConstraints: []corev1.TopologySpreadConstraint{{
+							MaxSkew:           int32(1),
+							TopologyKey:       "topology.kubernetes.io/zone",
+							WhenUnsatisfiable: corev1.ScheduleAnyway,
+							LabelSelector: &metav1.LabelSelector{
+								MatchExpressions: []metav1.LabelSelectorRequirement{
+									{
+										Key:      controller.ControllerDeploymentHashLabel,
+										Operator: metav1.LabelSelectorOpIn,
+										Values:   []string{"default"},
+									},
+								},
+							},
+						}},
 					},
 				},
 				Replicas: &nineteen,