Merge pull request #391 from Miciah/updateRouterDeployment-add-diff-t…

…o-log-message Bug 1834989: hashableDeployment: Fix liveness/readiness probes
openshift · May 17, 2020 · 8a8beb1 · 8a8beb1
2 parents d0758a0 + a9e00bb
commit 8a8beb1
Show file tree

Hide file tree

Showing 2 changed files with 98 additions and 4 deletions.
diff --git a/pkg/operator/controller/ingress/deployment.go b/pkg/operator/controller/ingress/deployment.go
@@ -11,6 +11,8 @@ import (
 	"strings"
 
 	"github.com/davecgh/go-spew/spew"
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 
 	operatorv1 "github.com/openshift/api/operator/v1"
 	"github.com/openshift/cluster-ingress-operator/pkg/manifests"
@@ -724,9 +726,8 @@ func hashableDeployment(deployment *appsv1.Deployment, onlyTemplate bool) *appsv
 			Image:           container.Image,
 			ImagePullPolicy: container.ImagePullPolicy,
 			Name:            container.Name,
-			LivenessProbe:   container.LivenessProbe,
-			ReadinessProbe:  container.ReadinessProbe,
-			VolumeMounts:    container.VolumeMounts,
+			LivenessProbe:   hashableProbe(container.LivenessProbe),
+			ReadinessProbe:  hashableProbe(container.ReadinessProbe),
 		}
 	}
 	sort.Slice(containers, func(i, j int) bool {
@@ -771,6 +772,43 @@ func hashableDeployment(deployment *appsv1.Deployment, onlyTemplate bool) *appsv
 	return &hashableDeployment
 }
 
+// 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
+// respective default values, will be zeroed.
+func hashableProbe(probe *corev1.Probe) *corev1.Probe {
+	if probe == nil {
+		return nil
+	}
+
+	var hashableProbe corev1.Probe
+
+	if probe.Handler.HTTPGet != nil {
+		hashableProbe.Handler.HTTPGet = &corev1.HTTPGetAction{
+			Path: probe.Handler.HTTPGet.Path,
+			Port: probe.Handler.HTTPGet.Port,
+			Host: probe.Handler.HTTPGet.Host,
+		}
+		if probe.Handler.HTTPGet.Scheme != "HTTP" {
+			hashableProbe.Handler.HTTPGet.Scheme = probe.Handler.HTTPGet.Scheme
+		}
+	}
+	if probe.TimeoutSeconds != int32(1) {
+		hashableProbe.TimeoutSeconds = probe.TimeoutSeconds
+	}
+	if probe.PeriodSeconds != int32(10) {
+		hashableProbe.PeriodSeconds = probe.PeriodSeconds
+	}
+	if probe.SuccessThreshold != int32(1) {
+		hashableProbe.SuccessThreshold = probe.SuccessThreshold
+	}
+	if probe.FailureThreshold != int32(3) {
+		hashableProbe.FailureThreshold = probe.FailureThreshold
+	}
+
+	return &hashableProbe
+}
+
 // currentRouterDeployment returns the current router deployment.
 func (r *reconciler) currentRouterDeployment(ci *operatorv1.IngressController) (*appsv1.Deployment, error) {
 	deployment := &appsv1.Deployment{}
@@ -799,10 +837,12 @@ func (r *reconciler) updateRouterDeployment(current, desired *appsv1.Deployment)
 		return nil
 	}
 
+	// Diff before updating because the client may mutate the object.
+	diff := cmp.Diff(current, updated, cmpopts.EquateEmpty())
 	if err := r.client.Update(context.TODO(), updated); err != nil {
 		return fmt.Errorf("failed to update router deployment %s/%s: %v", updated.Namespace, updated.Name, err)
 	}
-	log.Info("updated router deployment", "namespace", updated.Namespace, "name", updated.Name)
+	log.Info("updated router deployment", "namespace", updated.Namespace, "name", updated.Name, "diff", diff)
 	return nil
 }
 
@@ -848,6 +888,8 @@ func deploymentConfigChanged(current, expected *appsv1.Deployment) (bool, *appsv
 	updated.Spec.Template.Spec.NodeSelector = expected.Spec.Template.Spec.NodeSelector
 	updated.Spec.Template.Spec.Containers[0].Env = expected.Spec.Template.Spec.Containers[0].Env
 	updated.Spec.Template.Spec.Containers[0].Image = expected.Spec.Template.Spec.Containers[0].Image
+	updated.Spec.Template.Spec.Containers[0].LivenessProbe = expected.Spec.Template.Spec.Containers[0].LivenessProbe
+	updated.Spec.Template.Spec.Containers[0].ReadinessProbe = expected.Spec.Template.Spec.Containers[0].ReadinessProbe
 	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.Affinity = expected.Spec.Template.Spec.Affinity

diff --git a/pkg/operator/controller/ingress/deployment_test.go b/pkg/operator/controller/ingress/deployment_test.go
@@ -725,6 +725,38 @@ func TestDeploymentConfigChanged(t *testing.T) {
 			},
 			expect: false,
 		},
+		{
+			description: "if probe values are set to default values",
+			mutate: func(deployment *appsv1.Deployment) {
+				deployment.Spec.Template.Spec.Containers[0].LivenessProbe.Handler.HTTPGet.Scheme = "HTTP"
+				deployment.Spec.Template.Spec.Containers[0].LivenessProbe.TimeoutSeconds = int32(1)
+				deployment.Spec.Template.Spec.Containers[0].LivenessProbe.PeriodSeconds = int32(10)
+				deployment.Spec.Template.Spec.Containers[0].LivenessProbe.SuccessThreshold = int32(1)
+				deployment.Spec.Template.Spec.Containers[0].LivenessProbe.FailureThreshold = int32(3)
+				deployment.Spec.Template.Spec.Containers[0].ReadinessProbe.Handler.HTTPGet.Scheme = "HTTP"
+				deployment.Spec.Template.Spec.Containers[0].ReadinessProbe.TimeoutSeconds = int32(1)
+				deployment.Spec.Template.Spec.Containers[0].ReadinessProbe.PeriodSeconds = int32(10)
+				deployment.Spec.Template.Spec.Containers[0].ReadinessProbe.SuccessThreshold = int32(1)
+				deployment.Spec.Template.Spec.Containers[0].ReadinessProbe.FailureThreshold = int32(3)
+			},
+			expect: false,
+		},
+		{
+			description: "if probe values are set to non-default values",
+			mutate: func(deployment *appsv1.Deployment) {
+				deployment.Spec.Template.Spec.Containers[0].LivenessProbe.Handler.HTTPGet.Scheme = "HTTPS"
+				deployment.Spec.Template.Spec.Containers[0].LivenessProbe.TimeoutSeconds = int32(2)
+				deployment.Spec.Template.Spec.Containers[0].LivenessProbe.PeriodSeconds = int32(20)
+				deployment.Spec.Template.Spec.Containers[0].LivenessProbe.SuccessThreshold = int32(2)
+				deployment.Spec.Template.Spec.Containers[0].LivenessProbe.FailureThreshold = int32(30)
+				deployment.Spec.Template.Spec.Containers[0].ReadinessProbe.Handler.HTTPGet.Scheme = "HTTPS"
+				deployment.Spec.Template.Spec.Containers[0].ReadinessProbe.TimeoutSeconds = int32(2)
+				deployment.Spec.Template.Spec.Containers[0].ReadinessProbe.PeriodSeconds = int32(20)
+				deployment.Spec.Template.Spec.Containers[0].ReadinessProbe.SuccessThreshold = int32(2)
+				deployment.Spec.Template.Spec.Containers[0].ReadinessProbe.FailureThreshold = int32(30)
+			},
+			expect: true,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -791,6 +823,26 @@ func TestDeploymentConfigChanged(t *testing.T) {
 									},
 								},
 								Image: "openshift/origin-cluster-ingress-operator:v4.0",
+								LivenessProbe: &corev1.Probe{
+									Handler: corev1.Handler{
+										HTTPGet: &corev1.HTTPGetAction{
+											Path: "/healthz",
+											Port: intstr.IntOrString{
+												IntVal: int32(1936),
+											},
+										},
+									},
+								},
+								ReadinessProbe: &corev1.Probe{
+									Handler: corev1.Handler{
+										HTTPGet: &corev1.HTTPGetAction{
+											Path: "/healthz/ready",
+											Port: intstr.IntOrString{
+												IntVal: int32(1936),
+											},
+										},
+									},
+								},
 							},
 						},
 						Affinity: &corev1.Affinity{