Skip to content

Commit 9359b9d

Browse files
openshift-merge-robotopenshift-merge-robot
authored
Merge pull request #2125 from benluddy/csv-deployment-availability
Use DeploymentAvailable instead of custom test for CSV status.
2 parents 3ccd4d3 + 929fa8b commit 9359b9d

File tree

9 files changed

+243
-35
lines changed

9 files changed

+243
-35
lines changed

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,9 @@
22
spec:
33
strategy:
44
type: RollingUpdate
5+
rollingUpdate:
6+
maxUnavailable: {{ .Values.package.maxUnavailable }}
7+
maxSurge: {{ .Values.package.maxSurge }}
58
replicas: {{ .Values.package.replicaCount }}
69
selector:
710
matchLabels:

‎deploy/chart/values.yaml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ catalog:
3838

3939
package:
4040
replicaCount: 2
41+
maxUnavailable: 1
42+
maxSurge: 1
4143
image:
4244
ref: quay.io/operator-framework/olm:master
4345
pullPolicy: Always

‎deploy/ocp/values.yaml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,8 @@ catalog:
6363
memory: 80Mi
6464
package:
6565
replicaCount: 2
66+
maxUnavailable: 1
67+
maxSurge: 1
6668
image:
6769
ref: quay.io/operator-framework/olm@sha256:e9de77ac5c08643202ad42a0311d15c98ffbfd8a1dc2eab4e9f2dcaeed7110ac
6870
pullPolicy: IfNotPresent

‎deploy/upstream/values.yaml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ catalog:
2222
internalPort: 8080
2323
package:
2424
replicaCount: 2
25+
maxUnavailable: 1
26+
maxSurge: 1
2527
image:
2628
ref: quay.io/operator-framework/olm@sha256:e9de77ac5c08643202ad42a0311d15c98ffbfd8a1dc2eab4e9f2dcaeed7110ac
2729
pullPolicy: Always

‎pkg/controller/install/deployment_test.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,10 @@ func TestInstallStrategyDeploymentCheckInstallErrors(t *testing.T) {
353353
dep.Spec.Template.SetAnnotations(map[string]string{"test": "annotation"})
354354
dep.Spec.RevisionHistoryLimit = &revisionHistoryLimit
355355
dep.SetLabels(labels.CloneAndAddLabel(dep.ObjectMeta.GetLabels(), DeploymentSpecHashLabelKey, HashDeploymentSpec(dep.Spec)))
356+
dep.Status.Conditions = append(dep.Status.Conditions, appsv1.DeploymentCondition{
357+
Type: appsv1.DeploymentAvailable,
358+
Status: corev1.ConditionTrue,
359+
})
356360
fakeClient.FindAnyDeploymentsMatchingLabelsReturns(
357361
[]*appsv1.Deployment{
358362
&dep,

‎pkg/controller/install/status_viewer.go‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"fmt"
77

88
appsv1 "k8s.io/api/apps/v1"
9+
corev1 "k8s.io/api/core/v1"
910
)
1011

1112
const TimedOutReason = "ProgressDeadlineExceeded"
@@ -26,9 +27,12 @@ func DeploymentStatus(deployment *appsv1.Deployment) (string, bool, error) {
2627
if deployment.Status.Replicas > deployment.Status.UpdatedReplicas {
2728
return fmt.Sprintf("Waiting for rollout to finish: %d old replicas are pending termination...\n", deployment.Status.Replicas-deployment.Status.UpdatedReplicas), false, nil
2829
}
29-
// waiting for new replicas to report as available
30-
if deployment.Status.AvailableReplicas < deployment.Status.UpdatedReplicas {
31-
return fmt.Sprintf("Waiting for rollout to finish: %d of %d updated replicas are available...\n", deployment.Status.AvailableReplicas, deployment.Status.UpdatedReplicas), false, nil
30+
if c := getDeploymentCondition(deployment.Status, appsv1.DeploymentAvailable); c == nil || c.Status != corev1.ConditionTrue {
31+
msg := fmt.Sprintf("deployment %q missing condition %q", deployment.Name, appsv1.DeploymentAvailable)
32+
if c != nil {
33+
msg = fmt.Sprintf("deployment %q not available: %s", deployment.Name, c.Message)
34+
}
35+
return fmt.Sprintf("Waiting for rollout to finish: %s\n", msg), false, nil
3236
}
3337
// deployment is finished
3438
return fmt.Sprintf("deployment %q successfully rolled out\n", deployment.Name), true, nil

‎pkg/controller/install/status_viewer_test.go‎

Lines changed: 87 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
package install
22

33
import (
4+
"fmt"
45
"testing"
56

67
apps "k8s.io/api/apps/v1"
8+
core "k8s.io/api/core/v1"
79
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
810
)
911

@@ -15,6 +17,26 @@ func TestDeploymentStatusViewerStatus(t *testing.T) {
1517
msg string
1618
done bool
1719
}{
20+
{
21+
generation: 0,
22+
specReplicas: 1,
23+
status: apps.DeploymentStatus{
24+
ObservedGeneration: 1,
25+
Replicas: 1,
26+
UpdatedReplicas: 0,
27+
AvailableReplicas: 1,
28+
UnavailableReplicas: 0,
29+
Conditions: []apps.DeploymentCondition{
30+
{
31+
Type: apps.DeploymentProgressing,
32+
Reason: "NotTimedOut",
33+
},
34+
},
35+
},
36+
37+
msg: "Waiting for rollout to finish: 0 out of 1 new replicas have been updated...\n",
38+
done: false,
39+
},
1840
{
1941
generation: 0,
2042
specReplicas: 1,
@@ -52,9 +74,16 @@ func TestDeploymentStatusViewerStatus(t *testing.T) {
5274
UpdatedReplicas: 2,
5375
AvailableReplicas: 1,
5476
UnavailableReplicas: 1,
77+
Conditions: []apps.DeploymentCondition{
78+
{
79+
Type: apps.DeploymentAvailable,
80+
Status: core.ConditionFalse,
81+
Message: "Deployment does not have minimum availability.",
82+
},
83+
},
5584
},
5685

57-
msg: "Waiting for rollout to finish: 1 of 2 updated replicas are available...\n",
86+
msg: "Waiting for rollout to finish: deployment \"foo\" not available: Deployment does not have minimum availability.\n",
5887
done: false,
5988
},
6089
{
@@ -64,13 +93,38 @@ func TestDeploymentStatusViewerStatus(t *testing.T) {
6493
ObservedGeneration: 1,
6594
Replicas: 2,
6695
UpdatedReplicas: 2,
67-
AvailableReplicas: 2,
68-
UnavailableReplicas: 0,
96+
AvailableReplicas: 1,
97+
UnavailableReplicas: 1,
98+
Conditions: []apps.DeploymentCondition{
99+
{
100+
Type: apps.DeploymentAvailable,
101+
Status: core.ConditionTrue,
102+
},
103+
},
69104
},
70105

71106
msg: "deployment \"foo\" successfully rolled out\n",
72107
done: true,
73108
},
109+
{
110+
generation: 1,
111+
specReplicas: 2,
112+
status: apps.DeploymentStatus{
113+
ObservedGeneration: 1,
114+
Replicas: 2,
115+
UpdatedReplicas: 2,
116+
AvailableReplicas: 1,
117+
UnavailableReplicas: 1,
118+
Conditions: []apps.DeploymentCondition{
119+
{
120+
Type: "Fooing",
121+
Status: core.ConditionTrue,
122+
},
123+
},
124+
},
125+
msg: "Waiting for rollout to finish: deployment \"foo\" missing condition \"Available\"\n",
126+
done: false,
127+
},
74128
{
75129
generation: 2,
76130
specReplicas: 2,
@@ -87,33 +141,35 @@ func TestDeploymentStatusViewerStatus(t *testing.T) {
87141
},
88142
}
89143

90-
for _, test := range tests {
91-
d := &apps.Deployment{
92-
ObjectMeta: metav1.ObjectMeta{
93-
Namespace: "bar",
94-
Name: "foo",
95-
UID: "8764ae47-9092-11e4-8393-42010af018ff",
96-
Generation: test.generation,
97-
},
98-
Spec: apps.DeploymentSpec{
99-
Replicas: &test.specReplicas,
100-
},
101-
Status: test.status,
102-
}
103-
msg, done, err := DeploymentStatus(d)
104-
if err != nil {
105-
t.Fatalf("DeploymentStatusViewer.Status(): %v", err)
106-
}
107-
if done != test.done || msg != test.msg {
108-
t.Errorf("DeploymentStatusViewer.Status() for deployment with generation %d, %d replicas specified, and status %+v returned %q, %t, want %q, %t",
109-
test.generation,
110-
test.specReplicas,
111-
test.status,
112-
msg,
113-
done,
114-
test.msg,
115-
test.done,
116-
)
117-
}
144+
for i, test := range tests {
145+
t.Run(fmt.Sprintf("%d", i+1), func(t *testing.T) {
146+
d := &apps.Deployment{
147+
ObjectMeta: metav1.ObjectMeta{
148+
Namespace: "bar",
149+
Name: "foo",
150+
UID: "8764ae47-9092-11e4-8393-42010af018ff",
151+
Generation: test.generation,
152+
},
153+
Spec: apps.DeploymentSpec{
154+
Replicas: &test.specReplicas,
155+
},
156+
Status: test.status,
157+
}
158+
msg, done, err := DeploymentStatus(d)
159+
if err != nil {
160+
t.Fatalf("DeploymentStatusViewer.Status(): %v", err)
161+
}
162+
if done != test.done || msg != test.msg {
163+
t.Errorf("DeploymentStatusViewer.Status() for deployment with generation %d, %d replicas specified, and status %+v returned %q, %t, want %q, %t",
164+
test.generation,
165+
test.specReplicas,
166+
test.status,
167+
msg,
168+
done,
169+
test.msg,
170+
test.done,
171+
)
172+
}
173+
})
118174
}
119175
}

‎pkg/controller/operators/olm/operator_test.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -372,6 +372,10 @@ func deployment(deploymentName, namespace, serviceAccountName string, templateAn
372372
Replicas: singleInstance,
373373
AvailableReplicas: singleInstance,
374374
UpdatedReplicas: singleInstance,
375+
Conditions: []appsv1.DeploymentCondition{{
376+
Type: appsv1.DeploymentAvailable,
377+
Status: corev1.ConditionTrue,
378+
}},
375379
},
376380
}
377381
}
@@ -3420,6 +3424,10 @@ func TestUpdates(t *testing.T) {
34203424
dep.Status.Replicas = 1
34213425
dep.Status.UpdatedReplicas = 1
34223426
dep.Status.AvailableReplicas = 1
3427+
dep.Status.Conditions = []appsv1.DeploymentCondition{{
3428+
Type: appsv1.DeploymentAvailable,
3429+
Status: corev1.ConditionTrue,
3430+
}}
34233431
_, err = client.KubernetesInterface().AppsV1().Deployments(namespace).UpdateStatus(context.TODO(), dep, metav1.UpdateOptions{})
34243432
require.NoError(t, err)
34253433
}
@@ -4437,6 +4445,10 @@ func TestSyncOperatorGroups(t *testing.T) {
44374445
dep.Status.Replicas = 1
44384446
dep.Status.UpdatedReplicas = 1
44394447
dep.Status.AvailableReplicas = 1
4448+
dep.Status.Conditions = []appsv1.DeploymentCondition{{
4449+
Type: appsv1.DeploymentAvailable,
4450+
Status: corev1.ConditionTrue,
4451+
}}
44404452
_, err = client.KubernetesInterface().AppsV1().Deployments(tt.initial.operatorGroup.GetNamespace()).UpdateStatus(context.TODO(), dep, metav1.UpdateOptions{})
44414453
require.NoError(t, err)
44424454
}

0 commit comments

Comments
 (0)