Merge pull request #2125 from benluddy/csv-deployment-availability

Use DeploymentAvailable instead of custom test for CSV status.

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)
  }