Commit 52ed327c authored by Miciah Masters's avatar Miciah Masters Committed by Miciah Dashiel Butler Masters
Browse files

Specify topology spread constraints

Specify topology spread constraints on router deployments to encourage the
scheduler to spread router pod replicas across availability zones.

Add a to-do to replace the hard anti-affinity rule with soft anti-affinity
once the descheduler has support for enforcing soft anti-affinity rules.

This commit fixes bug 1900819.

https://bugzilla.redhat.com/show_bug.cgi?id=1900819

* pkg/operator/controller/ingress/deployment.go (desiredRouterDeployment):
Rename needDeploymentHash to configureAffinity as we now always need to
compute the deployment hash but do not always specify an affinity policy.
Specify spec.template.spec.topologySpreadConstraints.  Specify max skew 1,
unsatisfiable-constraint action "ScheduleAnyway", topology key
"topology.kubernetes.io/zone", and a label selector on the deployment hash
label so as to specify a preference for spreading replicas of the same
generation out but not prevent scheduling replicas of different generations
on the same node or scheduling more replicas than there are availability
zones.  Add to-do for changing to soft anti-affinity.
(hashableDeployment): Hash spec.template.spec.topologySpreadConstraints.
(cmpMatchExpressions, zeroOutDeploymentHash): New functions, factored out
of hashableDeployment.
* pkg/operator/controller/ingress/deployment_test.go (checkDeploymentHash):
New function, factored out of TestDesiredRouterDeployment.
(TestDesiredRouterDeployment): Use checkDeploymentHash.  Expect the
deployment hash label always to be set.
(TestDeploymentConfigChanged): Add test cases for
spec.template.spec.topologySpreadConstraints.
parent 3f35d0c0
......@@ -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 {
......
......@@ -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,
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment