Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use DeploymentAvailable instead of custom test for CSV status. #2125

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions deploy/chart/templates/_packageserver.deployment-spec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
spec:
strategy:
type: RollingUpdate
rollingUpdate:
maxUnavailable: {{ .Values.package.maxUnavailable }}
maxSurge: {{ .Values.package.maxSurge }}
replicas: {{ .Values.package.replicaCount }}
selector:
matchLabels:
Expand Down
2 changes: 2 additions & 0 deletions deploy/chart/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ catalog:

package:
replicaCount: 2
maxUnavailable: 1
maxSurge: 1
benluddy marked this conversation as resolved.
Show resolved Hide resolved
image:
ref: quay.io/operator-framework/olm:master
pullPolicy: Always
Expand Down
2 changes: 2 additions & 0 deletions deploy/ocp/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions deploy/upstream/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions pkg/controller/install/deployment_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
10 changes: 7 additions & 3 deletions pkg/controller/install/status_viewer.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"

appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
)

const TimedOutReason = "ProgressDeadlineExceeded"
Expand All @@ -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
Expand Down
118 changes: 87 additions & 31 deletions pkg/controller/install/status_viewer_test.go
Original file line number Diff line number Diff line change
@@ -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"
)

Expand All @@ -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,
Expand Down Expand Up @@ -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,
},
{
Expand All @@ -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,
Expand All @@ -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,
)
}
})
}
}
12 changes: 12 additions & 0 deletions pkg/controller/operators/olm/operator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}},
},
}
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down
Loading