Skip to content

Commit ce6169c

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #1031 from petr-muller/ota-1159-refactor-syncStatus-for-testability
OTA-1159: [1/x] Refactor syncStatus for testability
2 parents a679864 + 3ec2445 commit ce6169c

File tree

3 files changed

+67
-60
lines changed

3 files changed

+67
-60
lines changed

‎pkg/cvo/cvo.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -811,18 +811,18 @@ func versionStringFromRelease(release configv1.Release) string {
811811
// are used as fallbacks for any properties not defined in the release
812812
// image itself.
813813
func (optr *Operator) currentVersion() configv1.Release {
814-
return optr.mergeReleaseMetadata(optr.release)
814+
return mergeReleaseMetadata(optr.release, optr.getAvailableUpdates)
815815
}
816816

817817
// mergeReleaseMetadata returns a deep copy of the input release.
818818
// Values from any matching availableUpdates release are used as
819819
// fallbacks for any properties not defined in the input release.
820-
func (optr *Operator) mergeReleaseMetadata(release configv1.Release) configv1.Release {
820+
func mergeReleaseMetadata(release configv1.Release, getAvailableUpdates func() *availableUpdates) configv1.Release {
821821
merged := *release.DeepCopy()
822822

823823
if merged.Version == "" || len(merged.URL) == 0 || merged.Channels == nil {
824824
// only fill in missing values from availableUpdates, to avoid clobbering data from payload.LoadUpdate.
825-
availableUpdates := optr.getAvailableUpdates()
825+
availableUpdates := getAvailableUpdates()
826826
if availableUpdates != nil {
827827
var update *configv1.Release
828828
if equalDigest(merged.Image, availableUpdates.Current.Image) {

‎pkg/cvo/cvo_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4343,7 +4343,7 @@ func TestOperator_mergeReleaseMetadata(t *testing.T) {
43434343
} {
43444344
t.Run(testCase.name, func(t *testing.T) {
43454345
optr := Operator{availableUpdates: testCase.availableUpdates}
4346-
actual := optr.mergeReleaseMetadata(testCase.input)
4346+
actual := mergeReleaseMetadata(testCase.input, optr.getAvailableUpdates)
43474347
if !reflect.DeepEqual(actual, testCase.expected) {
43484348
t.Fatalf("unexpected: %s", diff.ObjectReflectDiff(testCase.expected, actual))
43494349
}

‎pkg/cvo/status.go

Lines changed: 63 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -65,21 +65,21 @@ func mergeEqualVersions(current *configv1.UpdateHistory, desired configv1.Releas
6565
return false
6666
}
6767

68-
func mergeOperatorHistory(config *configv1.ClusterVersion, desired configv1.Release, verified bool, now metav1.Time,
68+
func mergeOperatorHistory(cvStatus *configv1.ClusterVersionStatus, desired configv1.Release, verified bool, now metav1.Time,
6969
completed bool, acceptedRisksMsg string, localPayload bool) {
7070

7171
// if we have no image, we cannot reproduce the update later and so it cannot be part of the history
7272
if len(desired.Image) == 0 {
7373
// make the array empty
74-
if config.Status.History == nil {
75-
config.Status.History = []configv1.UpdateHistory{}
74+
if cvStatus.History == nil {
75+
cvStatus.History = []configv1.UpdateHistory{}
7676
}
7777
return
7878
}
7979

80-
if len(config.Status.History) == 0 {
80+
if len(cvStatus.History) == 0 {
8181
klog.V(2).Infof("initialize new history completed=%t desired=%#v", completed, desired)
82-
config.Status.History = append(config.Status.History, configv1.UpdateHistory{
82+
cvStatus.History = append(cvStatus.History, configv1.UpdateHistory{
8383
Version: desired.Version,
8484
Image: desired.Image,
8585

@@ -89,7 +89,7 @@ func mergeOperatorHistory(config *configv1.ClusterVersion, desired configv1.Rele
8989
})
9090
}
9191

92-
last := &config.Status.History[0]
92+
last := &cvStatus.History[0]
9393

9494
if len(last.State) == 0 {
9595
last.State = configv1.PartialUpdate
@@ -114,7 +114,7 @@ func mergeOperatorHistory(config *configv1.ClusterVersion, desired configv1.Rele
114114
last.CompletionTime = &now
115115
}
116116
if completed {
117-
config.Status.History = append([]configv1.UpdateHistory{
117+
cvStatus.History = append([]configv1.UpdateHistory{
118118
{
119119
Version: desired.Version,
120120
Image: desired.Image,
@@ -124,9 +124,9 @@ func mergeOperatorHistory(config *configv1.ClusterVersion, desired configv1.Rele
124124
CompletionTime: &now,
125125
AcceptedRisks: acceptedRisksMsg,
126126
},
127-
}, config.Status.History...)
127+
}, cvStatus.History...)
128128
} else {
129-
config.Status.History = append([]configv1.UpdateHistory{
129+
cvStatus.History = append([]configv1.UpdateHistory{
130130
{
131131
Version: desired.Version,
132132
Image: desired.Image,
@@ -135,27 +135,27 @@ func mergeOperatorHistory(config *configv1.ClusterVersion, desired configv1.Rele
135135
StartedTime: now,
136136
AcceptedRisks: acceptedRisksMsg,
137137
},
138-
}, config.Status.History...)
138+
}, cvStatus.History...)
139139
}
140140
}
141141

142142
// leave this here in case we find other future history bugs and need to debug it
143-
if klog.V(2).Enabled() && len(config.Status.History) > 1 {
144-
if config.Status.History[0].Image == config.Status.History[1].Image && config.Status.History[0].Version == config.Status.History[1].Version {
145-
data, _ := json.MarshalIndent(config.Status.History, "", " ")
143+
if klog.V(2).Enabled() && len(cvStatus.History) > 1 {
144+
if cvStatus.History[0].Image == cvStatus.History[1].Image && cvStatus.History[0].Version == cvStatus.History[1].Version {
145+
data, _ := json.MarshalIndent(cvStatus.History, "", " ")
146146
panic(fmt.Errorf("tried to update cluster version history to contain duplicate image entries: %s", string(data)))
147147
}
148148
}
149149

150150
// payloads can be verified during sync
151151
if verified {
152-
config.Status.History[0].Verified = true
152+
cvStatus.History[0].Verified = true
153153
}
154154

155155
// Prune least informative history entry when at maxHistory.
156-
config.Status.History = prune(config.Status.History, MaxHistory)
156+
cvStatus.History = prune(cvStatus.History, MaxHistory)
157157

158-
config.Status.Desired = desired
158+
cvStatus.Desired = desired
159159
}
160160

161161
// ClusterVersionInvalid indicates that the cluster version has an error that prevents the server from
@@ -198,35 +198,49 @@ func (optr *Operator) syncStatus(ctx context.Context, original, config *configv1
198198
original = config.DeepCopy()
199199
}
200200

201-
config.Status.ObservedGeneration = status.Generation
201+
updateClusterVersionStatus(&config.Status, status, optr.release, optr.getAvailableUpdates, validationErrs)
202+
203+
if klog.V(6).Enabled() {
204+
klog.Infof("Apply config: %s", diff.ObjectReflectDiff(original, config))
205+
}
206+
updated, err := applyClusterVersionStatus(ctx, optr.client.ConfigV1(), config, original)
207+
optr.rememberLastUpdate(updated)
208+
return err
209+
}
210+
211+
// updateClusterVersionStatus updates the passed cvStatus with the latest status information
212+
func updateClusterVersionStatus(cvStatus *configv1.ClusterVersionStatus, status *SyncWorkerStatus,
213+
release configv1.Release, getAvailableUpdates func() *availableUpdates, validationErrs field.ErrorList) {
214+
215+
cvStatus.ObservedGeneration = status.Generation
202216
if len(status.VersionHash) > 0 {
203-
config.Status.VersionHash = status.VersionHash
217+
cvStatus.VersionHash = status.VersionHash
204218
}
205219

206220
now := metav1.Now()
207221
version := versionStringFromRelease(status.Actual)
208-
if status.Actual.Image == optr.release.Image {
222+
if status.Actual.Image == release.Image {
209223
// backfill any missing information from the operator (payload).
210224
if status.Actual.Version == "" {
211-
status.Actual.Version = optr.release.Version
225+
status.Actual.Version = release.Version
212226
}
213227
if len(status.Actual.URL) == 0 {
214-
status.Actual.URL = optr.release.URL
228+
status.Actual.URL = release.URL
215229
}
216230
if status.Actual.Channels == nil {
217-
status.Actual.Channels = append(optr.release.Channels[:0:0], optr.release.Channels...) // copy
231+
status.Actual.Channels = append(release.Channels[:0:0], release.Channels...) // copy
218232
}
219233
}
220-
desired := optr.mergeReleaseMetadata(status.Actual)
234+
desired := mergeReleaseMetadata(status.Actual, getAvailableUpdates)
221235

222236
risksMsg := ""
223237
if desired.Image == status.loadPayloadStatus.Update.Image {
224238
risksMsg = status.loadPayloadStatus.AcceptedRisks
225239
}
226240

227-
mergeOperatorHistory(config, desired, status.Verified, now, status.Completed > 0, risksMsg, status.loadPayloadStatus.Local)
241+
mergeOperatorHistory(cvStatus, desired, status.Verified, now, status.Completed > 0, risksMsg, status.loadPayloadStatus.Local)
228242

229-
config.Status.Capabilities = status.CapabilitiesStatus.Status
243+
cvStatus.Capabilities = status.CapabilitiesStatus.Status
230244

231245
// update validation errors
232246
var reason string
@@ -242,42 +256,42 @@ func (optr *Operator) syncStatus(ctx context.Context, original, config *configv1
242256
}
243257
reason = "InvalidClusterVersion"
244258

245-
resourcemerge.SetOperatorStatusCondition(&config.Status.Conditions, configv1.ClusterOperatorStatusCondition{
259+
resourcemerge.SetOperatorStatusCondition(&cvStatus.Conditions, configv1.ClusterOperatorStatusCondition{
246260
Type: ClusterVersionInvalid,
247261
Status: configv1.ConditionTrue,
248262
Reason: reason,
249263
Message: buf.String(),
250264
LastTransitionTime: now,
251265
})
252266
} else {
253-
resourcemerge.RemoveOperatorStatusCondition(&config.Status.Conditions, ClusterVersionInvalid)
267+
resourcemerge.RemoveOperatorStatusCondition(&cvStatus.Conditions, ClusterVersionInvalid)
254268
}
255269

256270
// set the implicitly enabled capabilities condition
257-
setImplicitlyEnabledCapabilitiesCondition(config, status.CapabilitiesStatus.ImplicitlyEnabledCaps, now)
271+
setImplicitlyEnabledCapabilitiesCondition(cvStatus, status.CapabilitiesStatus.ImplicitlyEnabledCaps, now)
258272

259273
// set the desired release accepted condition
260-
setDesiredReleaseAcceptedCondition(config, status.loadPayloadStatus, now)
274+
setDesiredReleaseAcceptedCondition(cvStatus, status.loadPayloadStatus, now)
261275

262276
// set the available condition
263277
if status.Completed > 0 {
264-
resourcemerge.SetOperatorStatusCondition(&config.Status.Conditions, configv1.ClusterOperatorStatusCondition{
278+
resourcemerge.SetOperatorStatusCondition(&cvStatus.Conditions, configv1.ClusterOperatorStatusCondition{
265279
Type: configv1.OperatorAvailable,
266280
Status: configv1.ConditionTrue,
267281
Message: fmt.Sprintf("Done applying %s", version),
268282
LastTransitionTime: now,
269283
})
270284
}
271285
// default the available condition if not set
272-
if resourcemerge.FindOperatorStatusCondition(config.Status.Conditions, configv1.OperatorAvailable) == nil {
273-
resourcemerge.SetOperatorStatusCondition(&config.Status.Conditions, configv1.ClusterOperatorStatusCondition{
286+
if resourcemerge.FindOperatorStatusCondition(cvStatus.Conditions, configv1.OperatorAvailable) == nil {
287+
resourcemerge.SetOperatorStatusCondition(&cvStatus.Conditions, configv1.ClusterOperatorStatusCondition{
274288
Type: configv1.OperatorAvailable,
275289
Status: configv1.ConditionFalse,
276290
LastTransitionTime: now,
277291
})
278292
}
279293

280-
progressReason, progressMessage, skipFailure := convertErrorToProgressing(config.Status.History, now.Time, status)
294+
progressReason, progressMessage, skipFailure := convertErrorToProgressing(cvStatus.History, now.Time, status)
281295

282296
if err := status.Failure; err != nil && !skipFailure {
283297
var reason string
@@ -292,7 +306,7 @@ func (optr *Operator) syncStatus(ctx context.Context, original, config *configv1
292306
}
293307

294308
// set the failing condition
295-
resourcemerge.SetOperatorStatusCondition(&config.Status.Conditions, configv1.ClusterOperatorStatusCondition{
309+
resourcemerge.SetOperatorStatusCondition(&cvStatus.Conditions, configv1.ClusterOperatorStatusCondition{
296310
Type: ClusterStatusFailing,
297311
Status: configv1.ConditionTrue,
298312
Reason: reason,
@@ -302,15 +316,15 @@ func (optr *Operator) syncStatus(ctx context.Context, original, config *configv1
302316

303317
// update progressing
304318
if status.Reconciling {
305-
resourcemerge.SetOperatorStatusCondition(&config.Status.Conditions, configv1.ClusterOperatorStatusCondition{
319+
resourcemerge.SetOperatorStatusCondition(&cvStatus.Conditions, configv1.ClusterOperatorStatusCondition{
306320
Type: configv1.OperatorProgressing,
307321
Status: configv1.ConditionFalse,
308322
Reason: reason,
309323
Message: fmt.Sprintf("Error while reconciling %s: %s", version, msg),
310324
LastTransitionTime: now,
311325
})
312326
} else {
313-
resourcemerge.SetOperatorStatusCondition(&config.Status.Conditions, configv1.ClusterOperatorStatusCondition{
327+
resourcemerge.SetOperatorStatusCondition(&cvStatus.Conditions, configv1.ClusterOperatorStatusCondition{
314328
Type: configv1.OperatorProgressing,
315329
Status: configv1.ConditionTrue,
316330
Reason: reason,
@@ -321,15 +335,15 @@ func (optr *Operator) syncStatus(ctx context.Context, original, config *configv1
321335

322336
} else {
323337
// clear the failure condition
324-
resourcemerge.SetOperatorStatusCondition(&config.Status.Conditions, configv1.ClusterOperatorStatusCondition{Type: ClusterStatusFailing, Status: configv1.ConditionFalse, LastTransitionTime: now})
338+
resourcemerge.SetOperatorStatusCondition(&cvStatus.Conditions, configv1.ClusterOperatorStatusCondition{Type: ClusterStatusFailing, Status: configv1.ConditionFalse, LastTransitionTime: now})
325339

326340
// update progressing
327341
if status.Reconciling {
328342
message := fmt.Sprintf("Cluster version is %s", version)
329343
if len(validationErrs) > 0 {
330344
message = fmt.Sprintf("Stopped at %s: the cluster version is invalid", version)
331345
}
332-
resourcemerge.SetOperatorStatusCondition(&config.Status.Conditions, configv1.ClusterOperatorStatusCondition{
346+
resourcemerge.SetOperatorStatusCondition(&cvStatus.Conditions, configv1.ClusterOperatorStatusCondition{
333347
Type: configv1.OperatorProgressing,
334348
Status: configv1.ConditionFalse,
335349
Reason: reason,
@@ -355,7 +369,7 @@ func (optr *Operator) syncStatus(ctx context.Context, original, config *configv1
355369
default:
356370
message = fmt.Sprintf("Working towards %s", version)
357371
}
358-
resourcemerge.SetOperatorStatusCondition(&config.Status.Conditions, configv1.ClusterOperatorStatusCondition{
372+
resourcemerge.SetOperatorStatusCondition(&cvStatus.Conditions, configv1.ClusterOperatorStatusCondition{
359373
Type: configv1.OperatorProgressing,
360374
Status: configv1.ConditionTrue,
361375
Reason: reason,
@@ -366,23 +380,16 @@ func (optr *Operator) syncStatus(ctx context.Context, original, config *configv1
366380
}
367381

368382
// default retrieved updates if it is not set
369-
if resourcemerge.FindOperatorStatusCondition(config.Status.Conditions, configv1.RetrievedUpdates) == nil {
370-
resourcemerge.SetOperatorStatusCondition(&config.Status.Conditions, configv1.ClusterOperatorStatusCondition{
383+
if resourcemerge.FindOperatorStatusCondition(cvStatus.Conditions, configv1.RetrievedUpdates) == nil {
384+
resourcemerge.SetOperatorStatusCondition(&cvStatus.Conditions, configv1.ClusterOperatorStatusCondition{
371385
Type: configv1.RetrievedUpdates,
372386
Status: configv1.ConditionFalse,
373387
LastTransitionTime: now,
374388
})
375389
}
376-
377-
if klog.V(6).Enabled() {
378-
klog.Infof("Apply config: %s", diff.ObjectReflectDiff(original, config))
379-
}
380-
updated, err := applyClusterVersionStatus(ctx, optr.client.ConfigV1(), config, original)
381-
optr.rememberLastUpdate(updated)
382-
return err
383390
}
384391

385-
func setImplicitlyEnabledCapabilitiesCondition(config *configv1.ClusterVersion, implicitlyEnabled []configv1.ClusterVersionCapability,
392+
func setImplicitlyEnabledCapabilitiesCondition(cvStatus *configv1.ClusterVersionStatus, implicitlyEnabled []configv1.ClusterVersionCapability,
386393
now metav1.Time) {
387394

388395
if len(implicitlyEnabled) > 0 {
@@ -394,15 +401,15 @@ func setImplicitlyEnabledCapabilitiesCondition(config *configv1.ClusterVersion,
394401
sort.Strings(caps)
395402
message = message + strings.Join([]string(caps), ", ")
396403

397-
resourcemerge.SetOperatorStatusCondition(&config.Status.Conditions, configv1.ClusterOperatorStatusCondition{
404+
resourcemerge.SetOperatorStatusCondition(&cvStatus.Conditions, configv1.ClusterOperatorStatusCondition{
398405
Type: ImplicitlyEnabledCapabilities,
399406
Status: configv1.ConditionTrue,
400407
Reason: "CapabilitiesImplicitlyEnabled",
401408
Message: message,
402409
LastTransitionTime: now,
403410
})
404411
} else {
405-
resourcemerge.SetOperatorStatusCondition(&config.Status.Conditions, configv1.ClusterOperatorStatusCondition{
412+
resourcemerge.SetOperatorStatusCondition(&cvStatus.Conditions, configv1.ClusterOperatorStatusCondition{
406413
Type: ImplicitlyEnabledCapabilities,
407414
Status: configv1.ConditionFalse,
408415
Reason: "AsExpected",
@@ -412,9 +419,9 @@ func setImplicitlyEnabledCapabilitiesCondition(config *configv1.ClusterVersion,
412419
}
413420
}
414421

415-
func setDesiredReleaseAcceptedCondition(config *configv1.ClusterVersion, status LoadPayloadStatus, now metav1.Time) {
422+
func setDesiredReleaseAcceptedCondition(cvStatus *configv1.ClusterVersionStatus, status LoadPayloadStatus, now metav1.Time) {
416423
if status.Step == "PayloadLoaded" {
417-
resourcemerge.SetOperatorStatusCondition(&config.Status.Conditions, configv1.ClusterOperatorStatusCondition{
424+
resourcemerge.SetOperatorStatusCondition(&cvStatus.Conditions, configv1.ClusterOperatorStatusCondition{
418425
Type: DesiredReleaseAccepted,
419426
Status: configv1.ConditionTrue,
420427
Reason: status.Step,
@@ -423,15 +430,15 @@ func setDesiredReleaseAcceptedCondition(config *configv1.ClusterVersion, status
423430
})
424431
} else if status.Step != "" {
425432
if status.Failure != nil {
426-
resourcemerge.SetOperatorStatusCondition(&config.Status.Conditions, configv1.ClusterOperatorStatusCondition{
433+
resourcemerge.SetOperatorStatusCondition(&cvStatus.Conditions, configv1.ClusterOperatorStatusCondition{
427434
Type: DesiredReleaseAccepted,
428435
Status: configv1.ConditionFalse,
429436
Reason: status.Step,
430437
Message: status.Message,
431438
LastTransitionTime: now,
432439
})
433440
} else {
434-
resourcemerge.SetOperatorStatusCondition(&config.Status.Conditions, configv1.ClusterOperatorStatusCondition{
441+
resourcemerge.SetOperatorStatusCondition(&cvStatus.Conditions, configv1.ClusterOperatorStatusCondition{
435442
Type: DesiredReleaseAccepted,
436443
Status: configv1.ConditionUnknown,
437444
Reason: status.Step,
@@ -534,7 +541,7 @@ func (optr *Operator) syncFailingStatus(ctx context.Context, original *configv1.
534541
LastTransitionTime: now,
535542
})
536543

537-
mergeOperatorHistory(config, optr.currentVersion(), false, now, false, "", false)
544+
mergeOperatorHistory(&config.Status, optr.currentVersion(), false, now, false, "", false)
538545

539546
updated, err := applyClusterVersionStatus(ctx, optr.client.ConfigV1(), config, original)
540547
optr.rememberLastUpdate(updated)

0 commit comments

Comments
 (0)