From 51cb979c59cc9c0b57035c0d693f198026d20680 Mon Sep 17 00:00:00 2001 From: Jack Ottofaro Date: Mon, 18 Apr 2022 16:10:27 -0400 Subject: [PATCH] Do not save desired update on load failures since this will cause equalSyncWork to continually return true and no reattempt to load the payload will occur until the desired update is updated by the user again, e.g. image change or change to force. As a result, no attempt is made to recheck precondition failures which may have been resolved and therefore result in a successful payload load. To take the specific https://bugzilla.redhat.com/show_bug.cgi?id=2072389 bug failure as an example, because no recheck is made on the RecentEtcdBackup precondition CVO does not detect that the Etcd backup has been completed and it's safe to continue with the update. In addition, as a result of change https://github.com/openshift/cluster-version-operator/pull/683, the etcd operator must check ReleaseAccepted!=true rather than Failing=true to trigger the start of the backup. --- pkg/cvo/cvo_scenarios_test.go | 4 ++-- pkg/cvo/sync_worker.go | 18 +++++++++++++----- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/pkg/cvo/cvo_scenarios_test.go b/pkg/cvo/cvo_scenarios_test.go index ec90571389..8e768b1cc9 100644 --- a/pkg/cvo/cvo_scenarios_test.go +++ b/pkg/cvo/cvo_scenarios_test.go @@ -2022,7 +2022,7 @@ func TestCVO_UpgradePreconditionFailing(t *testing.T) { t.Fatal(err) } actions = client.Actions() - if len(actions) != 1 { + if len(actions) != 2 { t.Fatalf("%s", spew.Sdump(actions)) } expectGet(t, actions[0], "clusterversions", "", "version") @@ -2170,7 +2170,7 @@ func TestCVO_UpgradePreconditionFailing(t *testing.T) { expectUpdateStatus(t, actions[1], "clusterversions", "", &configv1.ClusterVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "version", - ResourceVersion: "3", + ResourceVersion: "4", Generation: 1, }, Spec: configv1.ClusterVersionSpec{ diff --git a/pkg/cvo/sync_worker.go b/pkg/cvo/sync_worker.go index 1d44d9484b..a7f848100f 100644 --- a/pkg/cvo/sync_worker.go +++ b/pkg/cvo/sync_worker.go @@ -439,11 +439,8 @@ func (w *SyncWorker) Update(ctx context.Context, generation int64, desired confi oldDesired = &w.work.Desired } - w.work = work - - if !versionEqual && oldDesired == nil { - klog.Infof("Propagating initial target version %v to sync worker loop in state %s.", desired, state) - } else if !versionEqual && state == payload.InitializingPayload { + // since oldDesired is not nil this is not the first time update is invoked and therefore w.work is not nil + if !versionEqual && oldDesired != nil && state == payload.InitializingPayload { klog.Warningf("Ignoring detected version change from %v to %v during payload initialization", *oldDesired, work.Desired) w.work.Desired = *oldDesired if overridesEqual && capabilitiesEqual { @@ -455,9 +452,20 @@ func (w *SyncWorker) Update(ctx context.Context, generation int64, desired confi implicit, err := w.loadUpdatedPayload(ctx, work, cvoOptrName) w.lock.Lock() if err != nil { + // save latest capability settings if not first time through + if w.work != nil { + w.work.Capabilities = work.Capabilities + } return w.status.DeepCopy() } + if !versionEqual && oldDesired == nil { + klog.Infof("Propagating initial target version %v to sync worker loop in state %s.", desired, state) + } + + // update work to include desired version now that it has been successfully loaded + w.work = work + // Update capabilities settings and status to include any capabilities that were implicitly enabled due // to previously managed resources. w.work.Capabilities = capability.SetFromImplicitlyEnabledCapabilities(implicit, w.work.Capabilities)