Merge pull request #863 from Miciah/OCPBUGS-4703-replace-liveness-gra…

…ce-period-seconds-annotation OCPBUGS-4703: Replace liveness-grace-period-seconds annotation
openshift · Jan 13, 2023 · dc49def · dc49def
2 parents 8d2294d + cacbafd
commit dc49def
Show file tree

Hide file tree

Showing 4 changed files with 39 additions and 13 deletions.
diff --git a/assets/router/deployment.yaml b/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

diff --git a/pkg/manifests/bindata.go b/pkg/manifests/bindata.go
diff --git a/pkg/operator/controller/ingress/deployment.go b/pkg/operator/controller/ingress/deployment.go
@@ -1562,11 +1562,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)
 		}
 	}
 
@@ -1614,6 +1618,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.
 

diff --git a/pkg/operator/controller/ingress/deployment_test.go b/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)
 	}
@@ -1342,6 +1343,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) {
@@ -1453,6 +1462,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 {
@@ -1479,7 +1497,8 @@ func TestDeploymentConfigChanged(t *testing.T) {
 							controller.ControllerDeploymentHashLabel: "1",
 						},
 						Annotations: map[string]string{
-							WorkloadPartitioningManagement: "{\"effect\": \"PreferredDuringScheduling\"}",
+							LivenessGracePeriodSecondsAnnotation: "10",
+							WorkloadPartitioningManagement:       "{\"effect\": \"PreferredDuringScheduling\"}",
 						},
 					},
 					Spec: corev1.PodSpec{