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/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,
+				)
+			}
+		})
 	}
 }

test/e2e/csv_e2e_test.go

@@ -9,6 +9,8 @@ import (
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+	"github.com/onsi/gomega/types"
+
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
@@ -27,6 +29,7 @@ import (
 
 	v1 "github.com/operator-framework/api/pkg/operators/v1"
 	"github.com/operator-framework/api/pkg/operators/v1alpha1"
+	operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient"
@@ -35,19 +38,139 @@ import (
 )
 
 var _ = Describe("ClusterServiceVersion", func() {
+	HavePhase := func(goal operatorsv1alpha1.ClusterServiceVersionPhase) types.GomegaMatcher {
+		return WithTransform(func(csv *operatorsv1alpha1.ClusterServiceVersion) operatorsv1alpha1.ClusterServiceVersionPhase {
+			return csv.Status.Phase
+		}, Equal(goal))
+	}
+
 	var (
 		c   operatorclient.ClientInterface
 		crc versioned.Interface
 	)
-	BeforeEach(func() {
 
+	BeforeEach(func() {
 		c = newKubeClient()
 		crc = newCRClient()
 	})
+
 	AfterEach(func() {
 		TearDown(testNamespace)
 	})
 
+	When("a csv exists specifying two replicas with one max unavailable", func() {
+		var (
+			csv v1alpha1.ClusterServiceVersion
+		)
+
+		const (
+			TestReadinessGate = "operatorframework.io/test-readiness-gate"
+		)
+
+		BeforeEach(func() {
+			csv = v1alpha1.ClusterServiceVersion{
+				ObjectMeta: metav1.ObjectMeta{
+					GenerateName: "test-csv",
+					Namespace:    testNamespace,
+				},
+				Spec: v1alpha1.ClusterServiceVersionSpec{
+					InstallStrategy: operatorsv1alpha1.NamedInstallStrategy{
+						StrategyName: operatorsv1alpha1.InstallStrategyNameDeployment,
+						StrategySpec: operatorsv1alpha1.StrategyDetailsDeployment{
+							DeploymentSpecs: []operatorsv1alpha1.StrategyDeploymentSpec{
+								{
+									Name: "deployment",
+									Spec: appsv1.DeploymentSpec{
+										Strategy: appsv1.DeploymentStrategy{
+											Type: appsv1.RollingUpdateDeploymentStrategyType,
+											RollingUpdate: &appsv1.RollingUpdateDeployment{
+												MaxUnavailable: &[]intstr.IntOrString{intstr.FromInt(1)}[0],
+											},
+										},
+										Selector: &metav1.LabelSelector{
+											MatchLabels: map[string]string{"app": "foobar"},
+										},
+										Replicas: &[]int32{2}[0],
+										Template: corev1.PodTemplateSpec{
+											ObjectMeta: metav1.ObjectMeta{
+												Labels: map[string]string{"app": "foobar"},
+											},
+											Spec: corev1.PodSpec{
+												Containers: []corev1.Container{
+													{
+														Name:  "foobar",
+														Image: *dummyImage,
+													},
+												},
+												ReadinessGates: []corev1.PodReadinessGate{
+													{ConditionType: TestReadinessGate},
+												},
+											},
+										},
+									},
+								},
+							},
+						},
+					},
+					InstallModes: []v1alpha1.InstallMode{{
+						Type:      v1alpha1.InstallModeTypeAllNamespaces,
+						Supported: true,
+					}},
+				},
+			}
+			Expect(ctx.Ctx().Client().Create(context.Background(), &csv)).To(Succeed())
+
+			Eventually(func() (*v1alpha1.ClusterServiceVersion, error) {
+				var ps corev1.PodList
+				if err := ctx.Ctx().Client().List(context.Background(), &ps, client.MatchingLabels{"app": "foobar"}); err != nil {
+					return nil, err
+				}
+
+				if len(ps.Items) != 2 {
+					return nil, fmt.Errorf("%d pods match deployment selector, want %d", len(ps.Items), 2)
+				}
+
+				for _, pod := range ps.Items {
+					index := -1
+					for i, c := range pod.Status.Conditions {
+						if c.Type == TestReadinessGate {
+							index = i
+							break
+						}
+					}
+					if index == -1 {
+						index = len(pod.Status.Conditions)
+						pod.Status.Conditions = append(pod.Status.Conditions, corev1.PodCondition{Type: TestReadinessGate})
+					}
+					if pod.Status.Conditions[index].Status == corev1.ConditionTrue {
+						continue
+					}
+					pod.Status.Conditions[index].Status = corev1.ConditionTrue
+					if err := ctx.Ctx().Client().Status().Update(context.Background(), &pod); err != nil {
+						return nil, err
+					}
+				}
+
+				if err := ctx.Ctx().Client().Get(context.Background(), client.ObjectKeyFromObject(&csv), &csv); err != nil {
+					return nil, err
+				}
+				return &csv, nil
+			}).Should(HavePhase(operatorsv1alpha1.CSVPhaseSucceeded))
+		})
+
+		It("remains in phase Succeeded when only one pod is available", func() {
+			var ps corev1.PodList
+			Expect(ctx.Ctx().Client().List(context.Background(), &ps, client.MatchingLabels{"app": "foobar"})).To(Succeed())
+			Expect(ps.Items).To(Not(BeEmpty()))
+
+			Expect(ctx.Ctx().Client().Delete(context.Background(), &ps.Items[0])).To(Succeed())
+
+			Consistently(func() (*operatorsv1alpha1.ClusterServiceVersion, error) {
+				return &csv, ctx.Ctx().Client().Get(context.Background(), client.ObjectKeyFromObject(&csv), &csv)
+			}).Should(HavePhase(operatorsv1alpha1.CSVPhaseSucceeded))
+		})
+	})
+
 	When("a copied csv exists", func() {
 		var (
 			target   corev1.Namespace