Replace liveness-grace-period-seconds annotation

Remove the
"unsupported.do-not-use.openshift.io/override-liveness-grace-period-seconds"
annotation from router pods, and instead set terminationGracePeriodSeconds
on the pods' liveness probes.

Previously, cluster-ingress-operator started setting this
OpenShift-specific annotation on router pods as a short-term measure to
configure the liveness probe's grace period in order to fix
BZ#1899941. This annotation was implemented by a carry patch in
openshift/kubernetes.  Since then, upstream Kubernetes has added the
terminationGracePeriodSeconds field for configuring probes.  Using the new
API field enables us to remove the carry patch.

This commit is related to bug 1899941.

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

Follow-up to commit 4017234.

This commit fixes OCPBUGS-4703.

https://issues.redhat.com/browse/OCPBUGS-4703

* assets/router/deployment.yaml: Remove the unsupported annotation, and
specify terminationGracePeriodSeconds on the liveness probe.
* pkg/manifests/bindata.go: Regenerate.
* pkg/operator/controller/ingress/deployment.go (deploymentConfigChanged):
Remove annotations that the operator owns when those annotations are not
expected to be set.
(copyProbe): Copy the terminationGracePeriodSeconds field.
* pkg/operator/controller/ingress/deployment_test.go
(TestDesiredRouterDeploymentSpecTemplate): Expect the unsupported
annotation not to be set.  Expect terminationGracePeriodSeconds to be set
on the liveness probe.
(TestDeploymentConfigChanged): Verify that deploymentConfigChanged
correctly sets the liveness probe's termination grace period seconds and
removes the unsupported annotation.

assets/router/deployment.yaml

@@ -8,7 +8,6 @@ spec:
   template:
     metadata:
       annotations:
-        "unsupported.do-not-use.openshift.io/override-liveness-grace-period-seconds": "10"
         target.workload.openshift.io/management: '{"effect": "PreferredDuringScheduling"}'
     spec:
       serviceAccountName: router
@@ -34,6 +33,7 @@ spec:
             httpGet:
               path: /healthz
               port: 1936
+            terminationGracePeriodSeconds: 10
           readinessProbe:
             httpGet:
               path: /healthz/ready

pkg/operator/controller/ingress/deployment.go

@@ -1519,11 +1519,15 @@ func deploymentConfigChanged(current, expected *appsv1.Deployment) (bool, *appsv
 
 	annotations := []string{LivenessGracePeriodSecondsAnnotation, WorkloadPartitioningManagement}
 	for _, key := range annotations {
-		if val, ok := expected.Spec.Template.Annotations[key]; ok && len(val) > 0 {
+		currentVal, have := current.Spec.Template.Annotations[key]
+		expectedVal, want := expected.Spec.Template.Annotations[key]
+		if want && (!have || currentVal != expectedVal) {
 			if updated.Spec.Template.Annotations == nil {
 				updated.Spec.Template.Annotations = make(map[string]string)
 			}
-			updated.Spec.Template.Annotations[key] = val
+			updated.Spec.Template.Annotations[key] = expectedVal
+		} else if have && !want {
+			delete(updated.Spec.Template.Annotations, key)
 		}
 	}
 
@@ -1571,6 +1575,9 @@ func copyProbe(a, b *corev1.Probe) {
 			b.ProbeHandler.HTTPGet.Scheme = a.ProbeHandler.HTTPGet.Scheme
 		}
 	}
+	if a.TerminationGracePeriodSeconds != nil {
+		b.TerminationGracePeriodSeconds = a.TerminationGracePeriodSeconds
+	}
 
 	// Users are permitted to modify the timeout, so *don't* copy it.
 

pkg/operator/controller/ingress/deployment_test.go

@@ -362,14 +362,12 @@ func TestDesiredRouterDeploymentSpecTemplate(t *testing.T) {
 		}
 	}
 
-	if expected, got := 2, len(deployment.Spec.Template.Annotations); expected != got {
+	if expected, got := 1, len(deployment.Spec.Template.Annotations); expected != got {
 		t.Errorf("expected len(annotations)=%v, got %v", expected, got)
 	}
 
-	if val, ok := deployment.Spec.Template.Annotations[LivenessGracePeriodSecondsAnnotation]; !ok {
-		t.Errorf("missing annotation %q", LivenessGracePeriodSecondsAnnotation)
-	} else if expected := "10"; expected != val {
-		t.Errorf("expected annotation %q to be %q, got %q", LivenessGracePeriodSecondsAnnotation, expected, val)
+	if val, ok := deployment.Spec.Template.Annotations[LivenessGracePeriodSecondsAnnotation]; ok {
+		t.Errorf("expected annotation %[1]q not to be set, got %[1]s=%[2]s", LivenessGracePeriodSecondsAnnotation, val)
 	}
 
 	if val, ok := deployment.Spec.Template.Annotations[WorkloadPartitioningManagement]; !ok {
@@ -389,6 +387,9 @@ func TestDesiredRouterDeploymentSpecTemplate(t *testing.T) {
 	if len(deployment.Spec.Template.Spec.Containers[0].LivenessProbe.ProbeHandler.HTTPGet.Host) != 0 {
 		t.Errorf("expected empty liveness probe host, got %q", deployment.Spec.Template.Spec.Containers[0].LivenessProbe.ProbeHandler.HTTPGet.Host)
 	}
+	if deployment.Spec.Template.Spec.Containers[0].LivenessProbe.TerminationGracePeriodSeconds == nil {
+		t.Error("expected liveness probe's termination grace period to be set")
+	}
 	if len(deployment.Spec.Template.Spec.Containers[0].ReadinessProbe.ProbeHandler.HTTPGet.Host) != 0 {
 		t.Errorf("expected empty readiness probe host, got %q", deployment.Spec.Template.Spec.Containers[0].ReadinessProbe.ProbeHandler.HTTPGet.Host)
 	}
@@ -1327,6 +1328,14 @@ func TestDeploymentConfigChanged(t *testing.T) {
 			},
 			expect: true,
 		},
+		{
+			description: "if the liveness probe's terminationGracePeriodSeconds is changed",
+			mutate: func(deployment *appsv1.Deployment) {
+				v := int64(123)
+				deployment.Spec.Template.Spec.Containers[0].LivenessProbe.TerminationGracePeriodSeconds = &v
+			},
+			expect: true,
+		},
 		{
 			description: "if readiness probe values are set to non-default values",
 			mutate: func(deployment *appsv1.Deployment) {
@@ -1438,6 +1447,15 @@ func TestDeploymentConfigChanged(t *testing.T) {
 			},
 			expect: true,
 		},
+		{
+			// This test case can be removed after OpenShift 4.13.
+			// See <https://issues.redhat.com/browse/OCPBUGS-4703>.
+			description: "if the unsupported.do-not-use.openshift.io/override-liveness-grace-period-seconds annotation is removed",
+			mutate: func(deployment *appsv1.Deployment) {
+				delete(deployment.Spec.Template.Annotations, LivenessGracePeriodSecondsAnnotation)
+			},
+			expect: true,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -1464,7 +1482,8 @@ func TestDeploymentConfigChanged(t *testing.T) {
 							controller.ControllerDeploymentHashLabel: "1",
 						},
 						Annotations: map[string]string{
-							WorkloadPartitioningManagement: "{\"effect\": \"PreferredDuringScheduling\"}",
+							LivenessGracePeriodSecondsAnnotation: "10",
+							WorkloadPartitioningManagement:       "{\"effect\": \"PreferredDuringScheduling\"}",
 						},
 					},
 					Spec: corev1.PodSpec{