Skip to content

Commit 5b539f5

Browse files
tchaptchap
committed
controller/workload: Clean up status reporting and test infrastructure
Replace inline error message loops with errors.Join, use ptr.Deref for replicas, fix areCondidtionsEqual typo. Add status.NewVersionGetter and podListErr support to tests, remove unnecessary Template/Labels from test fixtures.
1 parent eae3c72 commit 5b539f5

2 files changed

Lines changed: 42 additions & 40 deletions

File tree

‎pkg/operator/apiserver/controller/workload/workload.go‎

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
corev1listers "k8s.io/client-go/listers/core/v1"
1717
"k8s.io/client-go/tools/cache"
1818
"k8s.io/client-go/util/workqueue"
19+
"k8s.io/utils/ptr"
1920

2021
operatorv1 "github.com/openshift/api/operator/v1"
2122
applyoperatorv1 "github.com/openshift/client-go/operator/applyconfigurations/operator/v1"
@@ -218,8 +219,8 @@ func (c *Controller) updateOperatorStatus(ctx context.Context, previousStatus *o
218219

219220
if !preconditionsReady {
220221
var message string
221-
for _, err := range errs {
222-
message = message + err.Error() + "\n"
222+
if len(errs) > 0 {
223+
message = errors.Join(errs...).Error()
223224
}
224225
if len(message) == 0 {
225226
message = "the operator didn't specify what preconditions are missing"
@@ -249,14 +250,10 @@ func (c *Controller) updateOperatorStatus(ctx context.Context, previousStatus *o
249250
}
250251

251252
if len(errs) > 0 {
252-
message := ""
253-
for _, err := range errs {
254-
message = message + err.Error() + "\n"
255-
}
256253
workloadDegradedCondition = workloadDegradedCondition.
257254
WithStatus(operatorv1.ConditionTrue).
258255
WithReason("SyncError").
259-
WithMessage(message)
256+
WithMessage(errors.Join(errs...).Error())
260257
} else if workload == nil {
261258
workloadDegradedCondition = workloadDegradedCondition.
262259
WithStatus(operatorv1.ConditionTrue).
@@ -298,10 +295,7 @@ func (c *Controller) updateOperatorStatus(ctx context.Context, previousStatus *o
298295
WithReason("AsExpected")
299296
}
300297

301-
desiredReplicas := int32(1)
302-
if workload.Spec.Replicas != nil {
303-
desiredReplicas = *(workload.Spec.Replicas)
304-
}
298+
desiredReplicas := ptr.Deref(workload.Spec.Replicas, 1)
305299

306300
// If the workload is up to date, then we are no longer progressing
307301
workloadAtHighestGeneration := workload.ObjectMeta.Generation == workload.Status.ObservedGeneration

‎pkg/operator/apiserver/controller/workload/workload_test.go‎

Lines changed: 37 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
operatorv1 "github.com/openshift/api/operator/v1"
1616
"github.com/openshift/library-go/pkg/controller/factory"
1717
"github.com/openshift/library-go/pkg/operator/events"
18+
"github.com/openshift/library-go/pkg/operator/status"
1819
"github.com/openshift/library-go/pkg/operator/v1helpers"
1920
appsv1 "k8s.io/api/apps/v1"
2021
"k8s.io/apimachinery/pkg/api/equality"
@@ -57,13 +58,15 @@ func TestUpdateOperatorStatus(t *testing.T) {
5758

5859
workload *appsv1.Deployment
5960
pods []*corev1.Pod
61+
podListErr error
6062
operatorConfigAtHighestRevision bool
6163
operatorPreconditionsNotReady bool
6264
preconditionError error
6365
errors []error
6466
previousConditions []operatorv1.OperatorCondition
6567

66-
validateOperatorStatus func(*operatorv1.OperatorStatus) error
68+
validateOperatorStatus func(*operatorv1.OperatorStatus) error
69+
validateVersionRecorder func(status.VersionGetter) error
6770
}{
6871
{
6972
name: "scenario: no workload, no errors thus we are degraded and we are progressing",
@@ -95,7 +98,7 @@ func TestUpdateOperatorStatus(t *testing.T) {
9598
Message: "deployment/: could not be retrieved",
9699
},
97100
}
98-
return areCondidtionsEqual(expectedConditions, actualStatus.Conditions)
101+
return areConditionsEqual(expectedConditions, actualStatus.Conditions)
99102
},
100103
},
101104
{
@@ -112,7 +115,7 @@ func TestUpdateOperatorStatus(t *testing.T) {
112115
{
113116
Type: fmt.Sprintf("%sWorkloadDegraded", defaultControllerName),
114117
Status: operatorv1.ConditionTrue,
115-
Message: "nasty error\n",
118+
Message: "nasty error",
116119
Reason: "SyncError",
117120
},
118121
{
@@ -129,7 +132,7 @@ func TestUpdateOperatorStatus(t *testing.T) {
129132
Message: "deployment/: could not be retrieved",
130133
},
131134
}
132-
return areCondidtionsEqual(expectedConditions, actualStatus.Conditions)
135+
return areConditionsEqual(expectedConditions, actualStatus.Conditions)
133136
},
134137
},
135138
{
@@ -140,7 +143,6 @@ func TestUpdateOperatorStatus(t *testing.T) {
140143
Namespace: "openshift-apiserver",
141144
},
142145
Spec: appsv1.DeploymentSpec{
143-
Template: corev1.PodTemplateSpec{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"foo": "bar"}}},
144146
Replicas: ptr.To[int32](3),
145147
},
146148
Status: appsv1.DeploymentStatus{
@@ -152,7 +154,7 @@ func TestUpdateOperatorStatus(t *testing.T) {
152154
},
153155
pods: []*corev1.Pod{
154156
{
155-
ObjectMeta: metav1.ObjectMeta{Name: "apiserver", Namespace: "openshift-apiserver", Labels: map[string]string{"foo": "bar"}},
157+
ObjectMeta: metav1.ObjectMeta{Name: "apiserver", Namespace: "openshift-apiserver"},
156158
Status: corev1.PodStatus{
157159
Phase: corev1.PodPending,
158160
ContainerStatuses: []corev1.ContainerStatus{
@@ -203,7 +205,7 @@ func TestUpdateOperatorStatus(t *testing.T) {
203205
Message: "deployment/apiserver.openshift-apiserver: 0/3 pods have been updated to the latest generation and 0/3 pods are available",
204206
},
205207
}
206-
return areCondidtionsEqual(expectedConditions, actualStatus.Conditions)
208+
return areConditionsEqual(expectedConditions, actualStatus.Conditions)
207209
},
208210
},
209211
{
@@ -214,7 +216,6 @@ func TestUpdateOperatorStatus(t *testing.T) {
214216
Namespace: "openshift-apiserver",
215217
},
216218
Spec: appsv1.DeploymentSpec{
217-
Template: corev1.PodTemplateSpec{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"foo": "bar"}}},
218219
Replicas: ptr.To[int32](3),
219220
},
220221
Status: appsv1.DeploymentStatus{
@@ -226,7 +227,7 @@ func TestUpdateOperatorStatus(t *testing.T) {
226227
},
227228
pods: []*corev1.Pod{
228229
{
229-
ObjectMeta: metav1.ObjectMeta{Name: "apiserver", Namespace: "openshift-apiserver", Labels: map[string]string{"foo": "bar"}},
230+
ObjectMeta: metav1.ObjectMeta{Name: "apiserver", Namespace: "openshift-apiserver"},
230231
Status: corev1.PodStatus{
231232
Phase: corev1.PodPending,
232233
ContainerStatuses: []corev1.ContainerStatus{
@@ -276,7 +277,7 @@ func TestUpdateOperatorStatus(t *testing.T) {
276277
Message: "deployment/apiserver.openshift-apiserver: 0/3 pods have been updated to the latest generation and 0/3 pods are available",
277278
},
278279
}
279-
return areCondidtionsEqual(expectedConditions, actualStatus.Conditions)
280+
return areConditionsEqual(expectedConditions, actualStatus.Conditions)
280281
},
281282
},
282283
{
@@ -287,7 +288,6 @@ func TestUpdateOperatorStatus(t *testing.T) {
287288
Namespace: "openshift-apiserver",
288289
},
289290
Spec: appsv1.DeploymentSpec{
290-
Template: corev1.PodTemplateSpec{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"foo": "bar"}}},
291291
Replicas: ptr.To[int32](3),
292292
},
293293
Status: appsv1.DeploymentStatus{
@@ -343,7 +343,7 @@ func TestUpdateOperatorStatus(t *testing.T) {
343343
Message: "",
344344
},
345345
}
346-
return areCondidtionsEqual(expectedConditions, actualStatus.Conditions)
346+
return areConditionsEqual(expectedConditions, actualStatus.Conditions)
347347
},
348348
},
349349
{
@@ -389,7 +389,7 @@ func TestUpdateOperatorStatus(t *testing.T) {
389389
Message: "",
390390
},
391391
}
392-
return areCondidtionsEqual(expectedConditions, actualStatus.Conditions)
392+
return areConditionsEqual(expectedConditions, actualStatus.Conditions)
393393
},
394394
},
395395
{
@@ -439,7 +439,7 @@ func TestUpdateOperatorStatus(t *testing.T) {
439439
Message: "deployment/apiserver.openshift-apiserver: observed generation is 99, desired generation is 100.",
440440
},
441441
}
442-
return areCondidtionsEqual(expectedConditions, actualStatus.Conditions)
442+
return areConditionsEqual(expectedConditions, actualStatus.Conditions)
443443
},
444444
},
445445

@@ -490,7 +490,7 @@ func TestUpdateOperatorStatus(t *testing.T) {
490490
Message: "deployment/apiserver.openshift-apiserver: observed generation is 99, desired generation is 100.",
491491
},
492492
}
493-
return areCondidtionsEqual(expectedConditions, actualStatus.Conditions)
493+
return areConditionsEqual(expectedConditions, actualStatus.Conditions)
494494
},
495495
},
496496
{
@@ -523,7 +523,7 @@ func TestUpdateOperatorStatus(t *testing.T) {
523523
Message: "the operator didn't specify what preconditions are missing",
524524
},
525525
}
526-
return areCondidtionsEqual(expectedConditions, actualStatus.Conditions)
526+
return areConditionsEqual(expectedConditions, actualStatus.Conditions)
527527
},
528528
},
529529
{
@@ -573,7 +573,7 @@ func TestUpdateOperatorStatus(t *testing.T) {
573573
Message: "deployment/apiserver.openshift-apiserver: 1/3 pods have been updated to the latest generation and 2/3 pods are available",
574574
},
575575
}
576-
return areCondidtionsEqual(expectedConditions, actualStatus.Conditions)
576+
return areConditionsEqual(expectedConditions, actualStatus.Conditions)
577577
},
578578
},
579579
{
@@ -624,7 +624,7 @@ func TestUpdateOperatorStatus(t *testing.T) {
624624
Reason: "AsExpected",
625625
},
626626
}
627-
return areCondidtionsEqual(expectedConditions, actualStatus.Conditions)
627+
return areConditionsEqual(expectedConditions, actualStatus.Conditions)
628628
},
629629
},
630630
{
@@ -635,8 +635,6 @@ func TestUpdateOperatorStatus(t *testing.T) {
635635
Namespace: "openshift-apiserver",
636636
},
637637
Spec: appsv1.DeploymentSpec{
638-
Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}},
639-
Template: corev1.PodTemplateSpec{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"foo": "bar"}}},
640638
Replicas: ptr.To[int32](3),
641639
},
642640
Status: appsv1.DeploymentStatus{
@@ -673,7 +671,7 @@ func TestUpdateOperatorStatus(t *testing.T) {
673671
Message: "deployment/apiserver.openshift-apiserver: 3/3 pods have been updated to the latest generation and 2/3 pods are available",
674672
},
675673
}
676-
return areCondidtionsEqual(expectedConditions, actualStatus.Conditions)
674+
return areConditionsEqual(expectedConditions, actualStatus.Conditions)
677675
},
678676
},
679677
}
@@ -704,16 +702,20 @@ func TestUpdateOperatorStatus(t *testing.T) {
704702
syncErrrors: scenario.errors,
705703
}
706704

705+
versionRecorder := status.NewVersionGetter()
706+
707707
// act
708708
target := &Controller{
709-
operatorClient: fakeOperatorClient,
710-
targetNamespace: targetNs,
711-
podsLister: &fakePodLister{pods: scenario.pods},
712-
delegate: delegate,
709+
operatorClient: fakeOperatorClient,
710+
targetNamespace: targetNs,
711+
targetOperandVersion: "v1.0.0-test",
712+
podsLister: &fakePodLister{pods: scenario.pods, err: scenario.podListErr},
713+
delegate: delegate,
714+
versionRecorder: versionRecorder,
713715
}
714716

715717
err := target.sync(context.TODO(), factory.NewSyncContext("workloadcontroller_test", events.NewInMemoryRecorder("workloadcontroller_test", clocktesting.NewFakePassiveClock(time.Now()))))
716-
if err != nil && len(scenario.errors) == 0 {
718+
if err != nil && len(scenario.errors) == 0 && scenario.podListErr == nil {
717719
t.Fatal(err)
718720
}
719721

@@ -726,28 +728,34 @@ func TestUpdateOperatorStatus(t *testing.T) {
726728
if err != nil {
727729
t.Fatal(err)
728730
}
731+
if scenario.validateVersionRecorder != nil {
732+
if err := scenario.validateVersionRecorder(versionRecorder); err != nil {
733+
t.Fatal(err)
734+
}
735+
}
729736
})
730737
}
731738
}
732739

733740
type fakePodLister struct {
734741
pods []*corev1.Pod
742+
err error
735743
}
736744

737745
type fakePodNamespaceLister struct {
738746
lister *fakePodLister
739747
}
740748

741749
func (f *fakePodNamespaceLister) List(selector labels.Selector) (ret []*corev1.Pod, err error) {
742-
return f.lister.pods, nil
750+
return f.lister.pods, f.lister.err
743751
}
744752

745753
func (f *fakePodNamespaceLister) Get(name string) (*corev1.Pod, error) {
746754
panic("implement me")
747755
}
748756

749757
func (f *fakePodLister) List(selector labels.Selector) (ret []*corev1.Pod, err error) {
750-
return f.pods, nil
758+
return f.pods, f.err
751759
}
752760

753761
func (f *fakePodLister) Pods(namespace string) corev1listers.PodNamespaceLister {
@@ -756,7 +764,7 @@ func (f *fakePodLister) Pods(namespace string) corev1listers.PodNamespaceLister
756764
}
757765
}
758766

759-
func areCondidtionsEqual(expectedConditions []operatorv1.OperatorCondition, actualConditions []operatorv1.OperatorCondition) error {
767+
func areConditionsEqual(expectedConditions []operatorv1.OperatorCondition, actualConditions []operatorv1.OperatorCondition) error {
760768
if len(expectedConditions) != len(actualConditions) {
761769
return fmt.Errorf("expected %d conditions but got %d", len(expectedConditions), len(actualConditions))
762770
}

0 commit comments

Comments
 (0)