Use DeploymentAvailable instead of custom test for CSV status.

CSVs can transition to Failed (reason ComponentUnhealthy) if a single
pod managed by one of its deployments is deleted. This commit changes
the deployment availability test from a comparison between
.status.availableReplicas and .status.updatedReplicas to a check that
passes unless the deployment's Available condition exists and is
False.

Signed-off-by: Ben Luddy <[email protected]>

Author: benluddy

deploy/chart/templates/_packageserver.deployment-spec.yaml

@@ -2,6 +2,9 @@
 spec:
   strategy:
     type: RollingUpdate
+    rollingUpdate:
+      maxUnavailable: {{ .Values.package.maxUnavailable }}
+      maxSurge: {{ .Values.package.maxSurge }}
   replicas: {{ .Values.package.replicaCount }}
   selector:
     matchLabels:

deploy/chart/values.yaml

@@ -38,6 +38,8 @@ catalog:
 
 package:
   replicaCount: 2
+  maxUnavailable: 1
+  maxSurge: 1
   image:
     ref: quay.io/operator-framework/olm:master
     pullPolicy: Always

deploy/ocp/values.yaml

@@ -63,6 +63,8 @@ catalog:
       memory: 80Mi
 package:
   replicaCount: 2
+  maxUnavailable: 1
+  maxSurge: 1
   image:
     ref: quay.io/operator-framework/olm@sha256:e9de77ac5c08643202ad42a0311d15c98ffbfd8a1dc2eab4e9f2dcaeed7110ac
     pullPolicy: IfNotPresent

deploy/upstream/values.yaml

@@ -22,6 +22,8 @@ catalog:
     internalPort: 8080
 package:
   replicaCount: 2
+  maxUnavailable: 1
+  maxSurge: 1
   image:
     ref: quay.io/operator-framework/olm@sha256:e9de77ac5c08643202ad42a0311d15c98ffbfd8a1dc2eab4e9f2dcaeed7110ac
     pullPolicy: Always

pkg/controller/install/deployment_test.go

@@ -353,6 +353,10 @@ func TestInstallStrategyDeploymentCheckInstallErrors(t *testing.T) {
 			dep.Spec.Template.SetAnnotations(map[string]string{"test": "annotation"})
 			dep.Spec.RevisionHistoryLimit = &revisionHistoryLimit
 			dep.SetLabels(labels.CloneAndAddLabel(dep.ObjectMeta.GetLabels(), DeploymentSpecHashLabelKey, HashDeploymentSpec(dep.Spec)))
+			dep.Status.Conditions = append(dep.Status.Conditions, appsv1.DeploymentCondition{
+				Type:   appsv1.DeploymentAvailable,
+				Status: corev1.ConditionTrue,
+			})
 			fakeClient.FindAnyDeploymentsMatchingLabelsReturns(
 				[]*appsv1.Deployment{
 					&dep,

pkg/controller/install/status_viewer.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 
 	appsv1 "k8s.io/api/apps/v1"
+	corev1 "k8s.io/api/core/v1"
 )
 
 const TimedOutReason = "ProgressDeadlineExceeded"
@@ -26,9 +27,12 @@ func DeploymentStatus(deployment *appsv1.Deployment) (string, bool, error) {
 		if deployment.Status.Replicas > deployment.Status.UpdatedReplicas {
 			return fmt.Sprintf("Waiting for rollout to finish: %d old replicas are pending termination...\n", deployment.Status.Replicas-deployment.Status.UpdatedReplicas), false, nil
 		}
-		// waiting for new replicas to report as available
-		if deployment.Status.AvailableReplicas < deployment.Status.UpdatedReplicas {
-			return fmt.Sprintf("Waiting for rollout to finish: %d of %d updated replicas are available...\n", deployment.Status.AvailableReplicas, deployment.Status.UpdatedReplicas), false, nil
+		if c := getDeploymentCondition(deployment.Status, appsv1.DeploymentAvailable); c == nil || c.Status != corev1.ConditionTrue {
+			msg := fmt.Sprintf("deployment %q missing condition %q", deployment.Name, appsv1.DeploymentAvailable)
+			if c != nil {
+				msg = fmt.Sprintf("deployment %q not available: %s", deployment.Name, c.Message)
+			}
+			return fmt.Sprintf("Waiting for rollout to finish: %s\n", msg), false, nil
 		}
 		// deployment is finished
 		return fmt.Sprintf("deployment %q successfully rolled out\n", deployment.Name), true, nil

pkg/controller/install/status_viewer_test.go

@@ -1,9 +1,11 @@
 package install
 
 import (
+	"fmt"
 	"testing"
 
 	apps "k8s.io/api/apps/v1"
+	core "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -15,6 +17,26 @@ func TestDeploymentStatusViewerStatus(t *testing.T) {
 		msg          string
 		done         bool
 	}{
+		{
+			generation:   0,
+			specReplicas: 1,
+			status: apps.DeploymentStatus{
+				ObservedGeneration:  1,
+				Replicas:            1,
+				UpdatedReplicas:     0,
+				AvailableReplicas:   1,
+				UnavailableReplicas: 0,
+				Conditions: []apps.DeploymentCondition{
+					{
+						Type:   apps.DeploymentProgressing,
+						Reason: "NotTimedOut",
+					},
+				},
+			},
+
+			msg:  "Waiting for rollout to finish: 0 out of 1 new replicas have been updated...\n",
+			done: false,
+		},
 		{
 			generation:   0,
 			specReplicas: 1,
@@ -52,9 +74,16 @@ func TestDeploymentStatusViewerStatus(t *testing.T) {
 				UpdatedReplicas:     2,
 				AvailableReplicas:   1,
 				UnavailableReplicas: 1,
+				Conditions: []apps.DeploymentCondition{
+					{
+						Type:    apps.DeploymentAvailable,
+						Status:  core.ConditionFalse,
+						Message: "Deployment does not have minimum availability.",
+					},
+				},
 			},
 
-			msg:  "Waiting for rollout to finish: 1 of 2 updated replicas are available...\n",
+			msg:  "Waiting for rollout to finish: deployment \"foo\" not available: Deployment does not have minimum availability.\n",
 			done: false,
 		},
 		{
@@ -64,13 +93,38 @@ func TestDeploymentStatusViewerStatus(t *testing.T) {
 				ObservedGeneration:  1,
 				Replicas:            2,
 				UpdatedReplicas:     2,
-				AvailableReplicas:   2,
-				UnavailableReplicas: 0,
+				AvailableReplicas:   1,
+				UnavailableReplicas: 1,
+				Conditions: []apps.DeploymentCondition{
+					{
+						Type:   apps.DeploymentAvailable,
+						Status: core.ConditionTrue,
+					},
+				},
 			},
 
 			msg:  "deployment \"foo\" successfully rolled out\n",
 			done: true,
 		},
+		{
+			generation:   1,
+			specReplicas: 2,
+			status: apps.DeploymentStatus{
+				ObservedGeneration:  1,
+				Replicas:            2,
+				UpdatedReplicas:     2,
+				AvailableReplicas:   1,
+				UnavailableReplicas: 1,
+				Conditions: []apps.DeploymentCondition{
+					{
+						Type:   "Fooing",
+						Status: core.ConditionTrue,
+					},
+				},
+			},
+			msg:  "Waiting for rollout to finish: deployment \"foo\" missing condition \"Available\"\n",
+			done: false,
+		},
 		{
 			generation:   2,
 			specReplicas: 2,
@@ -87,33 +141,35 @@ func TestDeploymentStatusViewerStatus(t *testing.T) {
 		},
 	}
 
-	for _, test := range tests {
-		d := &apps.Deployment{
-			ObjectMeta: metav1.ObjectMeta{
-				Namespace:  "bar",
-				Name:       "foo",
-				UID:        "8764ae47-9092-11e4-8393-42010af018ff",
-				Generation: test.generation,
-			},
-			Spec: apps.DeploymentSpec{
-				Replicas: &test.specReplicas,
-			},
-			Status: test.status,
-		}
-		msg, done, err := DeploymentStatus(d)
-		if err != nil {
-			t.Fatalf("DeploymentStatusViewer.Status(): %v", err)
-		}
-		if done != test.done || msg != test.msg {
-			t.Errorf("DeploymentStatusViewer.Status() for deployment with generation %d, %d replicas specified, and status %+v returned %q, %t, want %q, %t",
-				test.generation,
-				test.specReplicas,
-				test.status,
-				msg,
-				done,
-				test.msg,
-				test.done,
-			)
-		}
+	for i, test := range tests {
+		t.Run(fmt.Sprintf("%d", i+1), func(t *testing.T) {
+			d := &apps.Deployment{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace:  "bar",
+					Name:       "foo",
+					UID:        "8764ae47-9092-11e4-8393-42010af018ff",
+					Generation: test.generation,
+				},
+				Spec: apps.DeploymentSpec{
+					Replicas: &test.specReplicas,
+				},
+				Status: test.status,
+			}
+			msg, done, err := DeploymentStatus(d)
+			if err != nil {
+				t.Fatalf("DeploymentStatusViewer.Status(): %v", err)
+			}
+			if done != test.done || msg != test.msg {
+				t.Errorf("DeploymentStatusViewer.Status() for deployment with generation %d, %d replicas specified, and status %+v returned %q, %t, want %q, %t",
+					test.generation,
+					test.specReplicas,
+					test.status,
+					msg,
+					done,
+					test.msg,
+					test.done,
+				)
+			}
+		})
 	}
 }

pkg/controller/operators/olm/operator_test.go

@@ -372,6 +372,10 @@ func deployment(deploymentName, namespace, serviceAccountName string, templateAn
 			Replicas:          singleInstance,
 			AvailableReplicas: singleInstance,
 			UpdatedReplicas:   singleInstance,
+			Conditions: []appsv1.DeploymentCondition{{
+				Type:   appsv1.DeploymentAvailable,
+				Status: corev1.ConditionTrue,
+			}},
 		},
 	}
 }
@@ -3420,6 +3424,10 @@ func TestUpdates(t *testing.T) {
 		dep.Status.Replicas = 1
 		dep.Status.UpdatedReplicas = 1
 		dep.Status.AvailableReplicas = 1
+		dep.Status.Conditions = []appsv1.DeploymentCondition{{
+			Type:   appsv1.DeploymentAvailable,
+			Status: corev1.ConditionTrue,
+		}}
 		_, err = client.KubernetesInterface().AppsV1().Deployments(namespace).UpdateStatus(context.TODO(), dep, metav1.UpdateOptions{})
 		require.NoError(t, err)
 	}
@@ -4437,6 +4445,10 @@ func TestSyncOperatorGroups(t *testing.T) {
 				dep.Status.Replicas = 1
 				dep.Status.UpdatedReplicas = 1
 				dep.Status.AvailableReplicas = 1
+				dep.Status.Conditions = []appsv1.DeploymentCondition{{
+					Type:   appsv1.DeploymentAvailable,
+					Status: corev1.ConditionTrue,
+				}}
 				_, err = client.KubernetesInterface().AppsV1().Deployments(tt.initial.operatorGroup.GetNamespace()).UpdateStatus(context.TODO(), dep, metav1.UpdateOptions{})
 				require.NoError(t, err)
 			}