From e9c53b13f8ead9073bbc0fc73d7ce1279c954ab5 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Mon, 20 Apr 2020 15:41:59 -0700 Subject: [PATCH 1/2] pkg/cvo/status: Always set status.desired to match current CVO As described in [1], bumping status.desired when a new spec.desiredUpdate is set leads to trouble like: $ oc adm upgrade --to 4.3.13 Updating to 4.3.13 $ oc get -o json clusterversion version | jq -r '.status.conditions[] | .lastTransitionTime + " " + .type + " " + .status + " " + .message' | sort 2020-04-20T20:57:12Z RetrievedUpdates True 2020-04-20T21:23:40Z Available True Done applying 4.3.10 2020-04-20T22:01:55Z Upgradeable False Cluster operator kube-apiserver cannot be upgraded: DefaultSecurityContextConstraintsUpgradeable: Default SecurityContextConstraints object(s) have mutated [privileged] 2020-04-20T22:16:40Z Failing True Precondition "ClusterVersionUpgradeable" failed because of "DefaultSecurityContextConstraints_Mutated": Cluster operator kube-apiserver cannot be upgraded: DefaultSecurityContextConstraintsUpgradeable: Default SecurityContextConstraints object(s) have mutated [privileged] 2020-04-20T22:16:40Z Progressing True Unable to apply 4.3.13: it may not be safe to apply this update $ oc get -o json clusterversion version | jq -r '.status.desired.version' 4.3.13 $ oc adm upgrade --to=4.3.13 --force info: Cluster is already at version 4.3.13 $ oc adm upgrade --clear Cleared the update field, still at 4.3.13 $ oc get -o json clusterversion version | jq -r '.status.desired.version' 4.3.10 Where the "already at..." and "still at ..." messages are relying on the status.desired semantics of [2]: > desired is the version that the cluster is reconciling towards. Ideally the property would have been called 'current' or some such, but it's too late to change it now. When the user sets spec.desiredUpdate, they are making their intention clear (and bumping history at this point is appropriate, because we need *somewhere* to store the 'verified' history entry). But to match the "reconciling towards" semantics, this commit shifts the actual status.desired bump so it happens right after the new CVO comes up (with the new currentVersion). This change reverses the earlier: Prefers the payload version over the operator's version... comments from 961873d66a (sync: Do config syncing in the background, 2019-01-11, #82). It also sets the stage for a world in which [3] has been fixed and the CVO continues to apply the current release's manifests while vetting a new target's preconditions in parallel. [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1826115 [2]: https://github.com/openshift/api/blob/0f159fee64dbf711d40dac3fa2ec8b563a2aaca8/config/v1/types_cluster_version.go#L82-L87 [3]: https://bugzilla.redhat.com/show_bug.cgi?id=1822752 --- pkg/cvo/cvo_scenarios_test.go | 150 +++++++++++++++++----------------- pkg/cvo/cvo_test.go | 38 ++++----- pkg/cvo/status.go | 9 +- 3 files changed, 100 insertions(+), 97 deletions(-) diff --git a/pkg/cvo/cvo_scenarios_test.go b/pkg/cvo/cvo_scenarios_test.go index 053c23666d..f09a60d968 100644 --- a/pkg/cvo/cvo_scenarios_test.go +++ b/pkg/cvo/cvo_scenarios_test.go @@ -176,7 +176,7 @@ func TestCVO_StartupAndSync(t *testing.T) { // o.releaseImage = "image/image:1" o.releaseVersion = "4.0.1" - desired := configv1.Update{Version: "4.0.1", Image: "image/image:1"} + current := o.currentVersion() // client.ClearActions() err = o.sync(o.queueKey()) @@ -200,7 +200,7 @@ func TestCVO_StartupAndSync(t *testing.T) { }, Status: configv1.ClusterVersionStatus{ ObservedGeneration: 1, - Desired: desired, + Desired: current, History: []configv1.UpdateHistory{ {State: configv1.PartialUpdate, Image: "image/image:1", Version: "4.0.1", StartedTime: defaultStartedTime}, }, @@ -283,8 +283,8 @@ func TestCVO_StartupAndSync(t *testing.T) { }, Status: configv1.ClusterVersionStatus{ ObservedGeneration: 1, - // Prefers the image version over the operator's version (although in general they will remain in sync) - Desired: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, + // Prefers the operator's version over Cincinnati's nominal release version (although in general they will remain in sync) + Desired: current, VersionHash: "6GC9TkkG9PA=", History: []configv1.UpdateHistory{ // Because image and operator had mismatched versions, we get two entries (which shouldn't happen unless there is a bug in the CVO) @@ -447,7 +447,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { // o.releaseImage = "image/image:1" o.releaseVersion = "4.0.1" - desired := configv1.Update{Version: "4.0.1", Image: "image/image:1"} + current := o.currentVersion() // client.ClearActions() err = o.sync(o.queueKey()) @@ -470,7 +470,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { Channel: "fast", }, Status: configv1.ClusterVersionStatus{ - Desired: desired, + Desired: current, ObservedGeneration: 1, History: []configv1.UpdateHistory{ {State: configv1.PartialUpdate, Image: "image/image:1", Version: "4.0.1", StartedTime: defaultStartedTime}, @@ -554,8 +554,8 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { }, Status: configv1.ClusterVersionStatus{ ObservedGeneration: 1, - // Prefers the image version over the operator's version (although in general they will remain in sync) - Desired: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, + // Prefers the operator's version over Cincinnati's nominal release version (although in general they will remain in sync) + Desired: current, VersionHash: "6GC9TkkG9PA=", History: []configv1.UpdateHistory{ // Because image and operator had mismatched versions, we get two entries (which shouldn't happen unless there is a bug in the CVO) @@ -708,7 +708,7 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { // o.releaseImage = "image/image:1" o.releaseVersion = "4.0.1" - desired := configv1.Update{Version: "4.0.1", Image: "image/image:1"} + current := o.currentVersion() // client.ClearActions() err = o.sync(o.queueKey()) @@ -731,7 +731,7 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { Channel: "fast", }, Status: configv1.ClusterVersionStatus{ - Desired: desired, + Desired: current, ObservedGeneration: 1, History: []configv1.UpdateHistory{ {State: configv1.PartialUpdate, Image: "image/image:1", Version: "4.0.1", StartedTime: defaultStartedTime}, @@ -815,8 +815,8 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { }, Status: configv1.ClusterVersionStatus{ ObservedGeneration: 1, - // Prefers the image version over the operator's version (although in general they will remain in sync) - Desired: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, + // Prefers the operator's version over Cincinnati's nominal release version (although in general they will remain in sync) + Desired: current, VersionHash: "6GC9TkkG9PA=", History: []configv1.UpdateHistory{ // Because image and operator had mismatched versions, we get two entries (which shouldn't happen unless there is a bug in the CVO) @@ -890,7 +890,8 @@ func TestCVO_UpgradeUnverifiedPayload(t *testing.T) { // o.releaseImage = "image/image:0" o.releaseVersion = "1.0.0-abc" - desired := configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"} + current := o.currentVersion() + target := configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"} uid, _ := uuid.NewRandom() clusterUID := configv1.ClusterID(uid.String()) cvs["version"] = &configv1.ClusterVersion{ @@ -901,11 +902,11 @@ func TestCVO_UpgradeUnverifiedPayload(t *testing.T) { Spec: configv1.ClusterVersionSpec{ ClusterID: clusterUID, Channel: "fast", - DesiredUpdate: &desired, + DesiredUpdate: &target, }, Status: configv1.ClusterVersionStatus{ - // Prefers the image version over the operator's version (although in general they will remain in sync) - Desired: desired, + // Prefers the operator's version over Cincinnati's nominal release version (although in general they will remain in sync) + Desired: current, VersionHash: "6GC9TkkG9PA=", History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Image: "image/image:0", Version: "1.0.0-abc", Verified: true, StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, @@ -954,12 +955,12 @@ func TestCVO_UpgradeUnverifiedPayload(t *testing.T) { verifyAllStatus(t, worker.StatusCh(), SyncWorkerStatus{ Step: "RetrievePayload", - Actual: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"}, + Actual: target, }, SyncWorkerStatus{ Step: "RetrievePayload", Failure: payloadErr, - Actual: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"}, + Actual: target, }, ) @@ -983,11 +984,11 @@ func TestCVO_UpgradeUnverifiedPayload(t *testing.T) { Spec: configv1.ClusterVersionSpec{ ClusterID: clusterUID, Channel: "fast", - DesiredUpdate: &desired, + DesiredUpdate: &target, }, Status: configv1.ClusterVersionStatus{ - // Prefers the image version over the operator's version (although in general they will remain in sync) - Desired: desired, + // Prefers the operator's version over Cincinnati's nominal release version (although in general they will remain in sync) + Desired: current, VersionHash: "6GC9TkkG9PA=", History: []configv1.UpdateHistory{ {State: configv1.PartialUpdate, Image: "image/image:1", Version: "1.0.1-abc", StartedTime: defaultStartedTime}, @@ -1006,7 +1007,7 @@ func TestCVO_UpgradeUnverifiedPayload(t *testing.T) { // Step 2: Set allowUnverifiedImages to true, trigger a sync and the operator should apply the payload // // set an updtae - copied := desired + copied := target copied.Force = true actual.Spec.DesiredUpdate = &copied retriever.Set(PayloadInfo{Directory: "testdata/payloadtest-2", VerificationError: payloadErr}, nil) @@ -1092,7 +1093,7 @@ func TestCVO_UpgradeUnverifiedPayload(t *testing.T) { }, Status: configv1.ClusterVersionStatus{ ObservedGeneration: 1, - Desired: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1", Force: true}, + Desired: current, VersionHash: "6GC9TkkG9PA=", History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Image: "image/image:1", Version: "1.0.1-abc", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, @@ -1115,7 +1116,8 @@ func TestCVO_UpgradeUnverifiedPayloadRetriveOnce(t *testing.T) { // o.releaseImage = "image/image:0" o.releaseVersion = "1.0.0-abc" - desired := configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"} + current := o.currentVersion() + target := configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"} uid, _ := uuid.NewRandom() clusterUID := configv1.ClusterID(uid.String()) cvs["version"] = &configv1.ClusterVersion{ @@ -1126,11 +1128,11 @@ func TestCVO_UpgradeUnverifiedPayloadRetriveOnce(t *testing.T) { Spec: configv1.ClusterVersionSpec{ ClusterID: clusterUID, Channel: "fast", - DesiredUpdate: &desired, + DesiredUpdate: &target, }, Status: configv1.ClusterVersionStatus{ - // Prefers the image version over the operator's version (although in general they will remain in sync) - Desired: desired, + // Prefers the operator's version over Cincinnati's nominal release version (although in general they will remain in sync) + Desired: current, VersionHash: "6GC9TkkG9PA=", History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Image: "image/image:0", Version: "1.0.0-abc", Verified: true, StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, @@ -1179,12 +1181,12 @@ func TestCVO_UpgradeUnverifiedPayloadRetriveOnce(t *testing.T) { verifyAllStatus(t, worker.StatusCh(), SyncWorkerStatus{ Step: "RetrievePayload", - Actual: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"}, + Actual: target, }, SyncWorkerStatus{ Step: "RetrievePayload", Failure: payloadErr, - Actual: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"}, + Actual: target, }, ) @@ -1208,11 +1210,11 @@ func TestCVO_UpgradeUnverifiedPayloadRetriveOnce(t *testing.T) { Spec: configv1.ClusterVersionSpec{ ClusterID: clusterUID, Channel: "fast", - DesiredUpdate: &desired, + DesiredUpdate: &target, }, Status: configv1.ClusterVersionStatus{ - // Prefers the image version over the operator's version (although in general they will remain in sync) - Desired: desired, + // Prefers the operator's version over Cincinnati's nominal release version (although in general they will remain in sync) + Desired: current, VersionHash: "6GC9TkkG9PA=", History: []configv1.UpdateHistory{ {State: configv1.PartialUpdate, Image: "image/image:1", Version: "1.0.1-abc", StartedTime: defaultStartedTime}, @@ -1231,7 +1233,7 @@ func TestCVO_UpgradeUnverifiedPayloadRetriveOnce(t *testing.T) { // Step 2: Set allowUnverifiedImages to true, trigger a sync and the operator should apply the payload // // set an updtae - copied := desired + copied := target copied.Force = true actual.Spec.DesiredUpdate = &copied retriever.Set(PayloadInfo{Directory: "testdata/payloadtest-2", VerificationError: payloadErr}, nil) @@ -1317,7 +1319,7 @@ func TestCVO_UpgradeUnverifiedPayloadRetriveOnce(t *testing.T) { }, Status: configv1.ClusterVersionStatus{ ObservedGeneration: 1, - Desired: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1", Force: true}, + Desired: current, VersionHash: "6GC9TkkG9PA=", History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Image: "image/image:1", Version: "1.0.1-abc", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, @@ -1377,7 +1379,8 @@ func TestCVO_UpgradePreconditionFailing(t *testing.T) { // o.releaseImage = "image/image:0" o.releaseVersion = "1.0.0-abc" - desired := configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"} + current := o.currentVersion() + target := configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"} uid, _ := uuid.NewRandom() clusterUID := configv1.ClusterID(uid.String()) cvs["version"] = &configv1.ClusterVersion{ @@ -1388,11 +1391,11 @@ func TestCVO_UpgradePreconditionFailing(t *testing.T) { Spec: configv1.ClusterVersionSpec{ ClusterID: clusterUID, Channel: "fast", - DesiredUpdate: &desired, + DesiredUpdate: &target, }, Status: configv1.ClusterVersionStatus{ - // Prefers the image version over the operator's version (although in general they will remain in sync) - Desired: desired, + // Prefers the operator's version over Cincinnati's nominal release version (although in general they will remain in sync) + Desired: current, VersionHash: "6GC9TkkG9PA=", History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Image: "image/image:0", Version: "1.0.0-abc", Verified: true, StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, @@ -1464,11 +1467,11 @@ func TestCVO_UpgradePreconditionFailing(t *testing.T) { Spec: configv1.ClusterVersionSpec{ ClusterID: clusterUID, Channel: "fast", - DesiredUpdate: &desired, + DesiredUpdate: &target, }, Status: configv1.ClusterVersionStatus{ - // Prefers the image version over the operator's version (although in general they will remain in sync) - Desired: desired, + // Prefers the operator's version over Cincinnati's nominal release version (although in general they will remain in sync) + Desired: current, VersionHash: "6GC9TkkG9PA=", History: []configv1.UpdateHistory{ {State: configv1.PartialUpdate, Image: "image/image:1", Version: "1.0.1-abc", StartedTime: defaultStartedTime}, @@ -1487,7 +1490,7 @@ func TestCVO_UpgradePreconditionFailing(t *testing.T) { // Step 2: Set allowUnverifiedImages to true, trigger a sync and the operator should apply the payload // // set an updtae - copied := desired + copied := target copied.Force = true actual.Spec.DesiredUpdate = &copied // @@ -1578,7 +1581,7 @@ func TestCVO_UpgradePreconditionFailing(t *testing.T) { }, Status: configv1.ClusterVersionStatus{ ObservedGeneration: 1, - Desired: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1", Force: true}, + Desired: current, VersionHash: "6GC9TkkG9PA=", History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Image: "image/image:1", Version: "1.0.1-abc", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, @@ -1601,7 +1604,8 @@ func TestCVO_UpgradeVerifiedPayload(t *testing.T) { // o.releaseImage = "image/image:0" o.releaseVersion = "1.0.0-abc" - desired := configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"} + current := o.currentVersion() + target := configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"} uid, _ := uuid.NewRandom() clusterUID := configv1.ClusterID(uid.String()) cvs["version"] = &configv1.ClusterVersion{ @@ -1613,11 +1617,11 @@ func TestCVO_UpgradeVerifiedPayload(t *testing.T) { Spec: configv1.ClusterVersionSpec{ ClusterID: clusterUID, Channel: "fast", - DesiredUpdate: &desired, + DesiredUpdate: &target, }, Status: configv1.ClusterVersionStatus{ - // Prefers the image version over the operator's version (although in general they will remain in sync) - Desired: desired, + // Prefers the operator's version over Cincinnati's nominal release version (although in general they will remain in sync) + Desired: current, VersionHash: "6GC9TkkG9PA=", History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Image: "image/image:0", Version: "1.0.0-abc", Verified: true, StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, @@ -1666,13 +1670,13 @@ func TestCVO_UpgradeVerifiedPayload(t *testing.T) { verifyAllStatus(t, worker.StatusCh(), SyncWorkerStatus{ Step: "RetrievePayload", - Actual: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"}, + Actual: target, Generation: 1, }, SyncWorkerStatus{ Step: "RetrievePayload", Failure: payloadErr, - Actual: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"}, + Actual: target, Generation: 1, }, ) @@ -1697,12 +1701,12 @@ func TestCVO_UpgradeVerifiedPayload(t *testing.T) { Spec: configv1.ClusterVersionSpec{ ClusterID: clusterUID, Channel: "fast", - DesiredUpdate: &desired, + DesiredUpdate: &target, }, Status: configv1.ClusterVersionStatus{ ObservedGeneration: 1, - // Prefers the image version over the operator's version (although in general they will remain in sync) - Desired: desired, + // Prefers the operator's version over Cincinnati's nominal release version (although in general they will remain in sync) + Desired: current, VersionHash: "6GC9TkkG9PA=", History: []configv1.UpdateHistory{ {State: configv1.PartialUpdate, Image: "image/image:1", Version: "1.0.1-abc", StartedTime: defaultStartedTime}, @@ -1720,7 +1724,7 @@ func TestCVO_UpgradeVerifiedPayload(t *testing.T) { // Step 2: Simulate a verified payload being retrieved and ensure the operator sets verified // - copied := desired + copied := target actual.ObjectMeta.Generation = 2 actual.Spec.DesiredUpdate = &copied retriever.Set(PayloadInfo{Directory: "testdata/payloadtest-2", Verified: true}, nil) @@ -1801,7 +1805,7 @@ func TestCVO_UpgradeVerifiedPayload(t *testing.T) { }, Status: configv1.ClusterVersionStatus{ ObservedGeneration: 2, - Desired: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"}, + Desired: current, VersionHash: "6GC9TkkG9PA=", History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Image: "image/image:1", Version: "1.0.1-abc", Verified: true, StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, @@ -1829,7 +1833,7 @@ func TestCVO_RestartAndReconcile(t *testing.T) { // o.releaseImage = "image/image:1" o.releaseVersion = "1.0.0-abc" - desired := configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"} + current := o.currentVersion() uid, _ := uuid.NewRandom() clusterUID := configv1.ClusterID(uid.String()) cvs["version"] = &configv1.ClusterVersion{ @@ -1842,8 +1846,8 @@ func TestCVO_RestartAndReconcile(t *testing.T) { Channel: "fast", }, Status: configv1.ClusterVersionStatus{ - // Prefers the image version over the operator's version (although in general they will remain in sync) - Desired: desired, + // Prefers the operator's version over Cincinnati's nominal release version (although in general they will remain in sync) + Desired: current, VersionHash: "6GC9TkkG9PA=", History: []configv1.UpdateHistory{ // TODO: this is wrong, should be single partial entry @@ -1996,7 +2000,7 @@ func TestCVO_ErrorDuringReconcile(t *testing.T) { // o.releaseImage = "image/image:1" o.releaseVersion = "1.0.0-abc" - desired := configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"} + current := o.currentVersion() uid, _ := uuid.NewRandom() clusterUID := configv1.ClusterID(uid.String()) cvs["version"] = &configv1.ClusterVersion{ @@ -2009,8 +2013,8 @@ func TestCVO_ErrorDuringReconcile(t *testing.T) { Channel: "fast", }, Status: configv1.ClusterVersionStatus{ - // Prefers the image version over the operator's version (although in general they will remain in sync) - Desired: desired, + // Prefers the operator's version over Cincinnati's nominal release version (although in general they will remain in sync) + Desired: current, VersionHash: "6GC9TkkG9PA=", History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Image: "image/image:1", Version: "1.0.0-abc", Verified: true, StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, @@ -2165,8 +2169,8 @@ func TestCVO_ErrorDuringReconcile(t *testing.T) { Channel: "fast", }, Status: configv1.ClusterVersionStatus{ - // Prefers the image version over the operator's version (although in general they will remain in sync) - Desired: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, + // Prefers the operator's version over Cincinnati's nominal release version (although in general they will remain in sync) + Desired: current, VersionHash: "6GC9TkkG9PA=", History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Image: "image/image:1", Version: "1.0.0-abc", Verified: true, StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, @@ -2206,7 +2210,7 @@ func TestCVO_ParallelError(t *testing.T) { // o.releaseImage = "image/image:1" o.releaseVersion = "1.0.0-abc" - desired := configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"} + current := o.currentVersion() uid, _ := uuid.NewRandom() clusterUID := configv1.ClusterID(uid.String()) cvs["version"] = &configv1.ClusterVersion{ @@ -2219,8 +2223,8 @@ func TestCVO_ParallelError(t *testing.T) { Channel: "fast", }, Status: configv1.ClusterVersionStatus{ - // Prefers the image version over the operator's version (although in general they will remain in sync) - Desired: desired, + // Prefers the operator's version over Cincinnati's nominal release version (although in general they will remain in sync) + Desired: current, History: []configv1.UpdateHistory{}, Conditions: []configv1.ClusterOperatorStatusCondition{}, }, @@ -2332,8 +2336,8 @@ func TestCVO_ParallelError(t *testing.T) { Channel: "fast", }, Status: configv1.ClusterVersionStatus{ - // Prefers the image version over the operator's version (although in general they will remain in sync) - Desired: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, + // Prefers the operator's version over Cincinnati's nominal release version (although in general they will remain in sync) + Desired: current, VersionHash: "7m-gGRrpkDU=", History: []configv1.UpdateHistory{ {State: configv1.PartialUpdate, Image: "image/image:1", Version: "1.0.0-abc", StartedTime: defaultStartedTime}, @@ -2361,7 +2365,7 @@ func TestCVO_VerifyInitializingPayloadState(t *testing.T) { // o.releaseImage = "image/image:1" o.releaseVersion = "1.0.0-abc" - desired := configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"} + current := o.currentVersion() uid, _ := uuid.NewRandom() clusterUID := configv1.ClusterID(uid.String()) cvs["version"] = &configv1.ClusterVersion{ @@ -2374,8 +2378,8 @@ func TestCVO_VerifyInitializingPayloadState(t *testing.T) { Channel: "fast", }, Status: configv1.ClusterVersionStatus{ - // Prefers the image version over the operator's version (although in general they will remain in sync) - Desired: desired, + // Prefers the operator's version over Cincinnati's nominal release version (although in general they will remain in sync) + Desired: current, VersionHash: "6GC9TkkG9PA=", History: []configv1.UpdateHistory{ {State: configv1.PartialUpdate, Image: "image/image:1", Version: "1.0.0-abc", StartedTime: defaultStartedTime}, @@ -2419,7 +2423,7 @@ func TestCVO_VerifyUpdatingPayloadState(t *testing.T) { // o.releaseImage = "image/image:1" o.releaseVersion = "1.0.0-abc" - desired := configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"} + current := o.currentVersion() uid, _ := uuid.NewRandom() clusterUID := configv1.ClusterID(uid.String()) cvs["version"] = &configv1.ClusterVersion{ @@ -2432,8 +2436,8 @@ func TestCVO_VerifyUpdatingPayloadState(t *testing.T) { Channel: "fast", }, Status: configv1.ClusterVersionStatus{ - // Prefers the image version over the operator's version (although in general they will remain in sync) - Desired: desired, + // Prefers the operator's version over Cincinnati's nominal release version (although in general they will remain in sync) + Desired: current, VersionHash: "6GC9TkkG9PA=", History: []configv1.UpdateHistory{ {State: configv1.PartialUpdate, Image: "image/image:1", Version: "1.0.0-abc", StartedTime: defaultStartedTime}, diff --git a/pkg/cvo/cvo_test.go b/pkg/cvo/cvo_test.go index ad1eb18ac7..ccb07a869e 100644 --- a/pkg/cvo/cvo_test.go +++ b/pkg/cvo/cvo_test.go @@ -27,8 +27,8 @@ import ( "k8s.io/apimachinery/pkg/util/diff" "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/discovery" - "k8s.io/client-go/rest" kfake "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/rest" ktesting "k8s.io/client-go/testing" "k8s.io/client-go/util/workqueue" "k8s.io/klog" @@ -346,7 +346,7 @@ func TestOperator_sync(t *testing.T) { {State: configv1.PartialUpdate, Version: "0.0.1-abc", Image: "image/image:v4.0.1", StartedTime: defaultStartedTime}, {State: configv1.PartialUpdate, Version: "4.0.1", Image: "image/image:v4.0.1", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, }, - Desired: configv1.Update{Version: "0.0.1-abc", Image: "image/image:v4.0.1"}, + Desired: configv1.Update{Version: "4.0.1", Image: "image/image:v4.0.1"}, VersionHash: "", Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionFalse}, @@ -419,7 +419,7 @@ func TestOperator_sync(t *testing.T) { {State: configv1.PartialUpdate, Version: "0.0.1-abc", Image: "image/image:v4.0.1", StartedTime: defaultStartedTime}, {State: configv1.PartialUpdate, Version: "4.0.1", Image: "image/image:v4.0.1", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, }, - Desired: configv1.Update{Version: "0.0.1-abc", Image: "image/image:v4.0.1"}, + Desired: configv1.Update{Image: "image/image:v4.0.1", Version: "4.0.1"}, VersionHash: "", Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionFalse}, @@ -493,7 +493,7 @@ func TestOperator_sync(t *testing.T) { {State: configv1.CompletedUpdate, Version: "0.0.1-abc", Image: "image/image:v4.0.1", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, {State: configv1.PartialUpdate, Version: "4.0.1", Image: "image/image:v4.0.1", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, }, - Desired: configv1.Update{Version: "0.0.1-abc", Image: "image/image:v4.0.1"}, + Desired: configv1.Update{Image: "image/image:v4.0.1", Version: "4.0.1"}, VersionHash: "", Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionTrue, Message: "Done applying 0.0.1-abc"}, @@ -559,7 +559,7 @@ func TestOperator_sync(t *testing.T) { Channel: "fast", }, Status: configv1.ClusterVersionStatus{ - Desired: configv1.Update{Version: "0.0.1-abc", Image: "image/image:v4.0.1"}, + Desired: configv1.Update{Image: "image/image:v4.0.1", Version: "4.0.1"}, History: []configv1.UpdateHistory{ {State: configv1.PartialUpdate, Version: "0.0.1-abc", Image: "image/image:v4.0.1", StartedTime: defaultStartedTime}, {State: configv1.PartialUpdate, Version: "4.0.1", Image: "image/image:v4.0.1", StartedTime: metav1.Time{Time: time.Unix(0, 0)}, CompletionTime: &defaultCompletionTime}, @@ -1134,7 +1134,7 @@ func TestOperator_sync(t *testing.T) { CompletionTime: &defaultCompletionTime, }, }, - Desired: configv1.Update{Image: "image/image:v4.0.1", Version: ""}, + Desired: configv1.Update{Image: "image/image:v4.0.1", Version: "4.0.1"}, VersionHash: "", Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionFalse}, @@ -1207,7 +1207,7 @@ func TestOperator_sync(t *testing.T) { History: []configv1.UpdateHistory{ {State: configv1.PartialUpdate, Image: "image/image:v4.0.1", Version: "0.0.1-abc", StartedTime: defaultStartedTime}, }, - Desired: configv1.Update{Image: "image/image:v4.0.1", Version: "0.0.1-abc"}, + Desired: configv1.Update{Image: "image/image:v4.0.1"}, VersionHash: "xyz", Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionFalse}, @@ -1348,7 +1348,7 @@ func TestOperator_sync(t *testing.T) { History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Version: "0.0.1-abc", Image: "image/image:v4.0.1", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, }, - Desired: configv1.Update{Image: "image/image:v4.0.1", Version: "0.0.1-abc"}, + Desired: configv1.Update{Image: "image/image:v4.0.1"}, ObservedGeneration: 2, Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionTrue, Message: "Done applying 0.0.1-abc"}, @@ -1421,7 +1421,7 @@ func TestOperator_sync(t *testing.T) { History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Version: "0.0.1-abc", Image: "image/image:v4.0.1", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, }, - Desired: configv1.Update{Image: "image/image:v4.0.1", Version: "0.0.1-abc"}, + Desired: configv1.Update{Image: "image/image:v4.0.1"}, ObservedGeneration: 2, Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionTrue, Message: "Done applying 0.0.1-abc"}, @@ -1497,7 +1497,7 @@ func TestOperator_sync(t *testing.T) { History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Version: "0.0.1-abc", Image: "image/image:v4.0.1", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, }, - Desired: configv1.Update{Image: "image/image:v4.0.1", Version: "0.0.1-abc"}, + Desired: configv1.Update{Image: "image/image:v4.0.1"}, ObservedGeneration: 2, Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionTrue, Message: "Done applying 0.0.1-abc"}, @@ -1571,7 +1571,7 @@ func TestOperator_sync(t *testing.T) { History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Version: "0.0.1-abc", Image: "image/image:v4.0.1", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, }, - Desired: configv1.Update{Image: "image/image:v4.0.1", Version: "0.0.1-abc"}, + Desired: configv1.Update{Image: "image/image:v4.0.1"}, ObservedGeneration: 2, Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionTrue, Message: "Done applying 0.0.1-abc"}, @@ -1654,7 +1654,7 @@ func TestOperator_sync(t *testing.T) { History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Version: "0.0.1-abc", Image: "image/image:v4.0.1", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, }, - Desired: configv1.Update{Image: "image/image:v4.0.1", Version: "0.0.1-abc"}, + Desired: configv1.Update{Image: "image/image:v4.0.1"}, ObservedGeneration: 2, Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionTrue, Message: "Done applying 0.0.1-abc"}, @@ -1732,7 +1732,7 @@ func TestOperator_sync(t *testing.T) { History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Version: "0.0.1-abc", Image: "image/image:v4.0.1", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, }, - Desired: configv1.Update{Image: "image/image:v4.0.1", Version: "0.0.1-abc"}, + Desired: configv1.Update{Image: "image/image:v4.0.1"}, ObservedGeneration: 2, Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionTrue, Message: "Done applying 0.0.1-abc"}, @@ -1794,7 +1794,7 @@ func TestOperator_sync(t *testing.T) { History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Version: "4.0.1", Image: "image/image:v4.0.1", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, }, - Desired: configv1.Update{Image: "image/image:v4.0.1", Version: "4.0.1"}, + Desired: configv1.Update{Image: "image/image:v4.0.1"}, ObservedGeneration: 2, Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionTrue, Message: "Done applying 4.0.1"}, @@ -1862,7 +1862,7 @@ func TestOperator_sync(t *testing.T) { History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Version: "4.0.1", Image: "image/image:v4.0.1", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, }, - Desired: configv1.Update{Image: "image/image:v4.0.1", Version: "4.0.1"}, + Desired: configv1.Update{Image: "image/image:v4.0.1"}, AvailableUpdates: []configv1.Update{ {Version: "4.0.2", Image: "test/image:1"}, {Version: "4.0.3", Image: "test/image:2"}, @@ -1937,7 +1937,7 @@ func TestOperator_sync(t *testing.T) { History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Version: "4.0.1", Image: "image/image:v4.0.1", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, }, - Desired: configv1.Update{Image: "image/image:v4.0.1", Version: "4.0.1"}, + Desired: configv1.Update{Image: "image/image:v4.0.1"}, AvailableUpdates: []configv1.Update{ {Version: "4.0.2", Image: "test/image:1"}, {Version: "4.0.3", Image: "test/image:2"}, @@ -2150,9 +2150,7 @@ func TestOperator_sync(t *testing.T) { History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Version: "0.0.1-abc", Image: "image/image:v4.0.1", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, }, - Desired: configv1.Update{ - Version: "0.0.1-abc", Image: "image/image:v4.0.1", - }, + Desired: configv1.Update{Image: "image/image:v4.0.1"}, VersionHash: "", Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: ClusterVersionInvalid, Status: configv1.ConditionTrue, Reason: "InvalidClusterVersion", Message: "The cluster version is invalid:\n* spec.upstream: Invalid value: \"#%GG\": must be a valid URL or empty\n* spec.clusterID: Invalid value: \"not-valid-cluster-id\": must be an RFC4122-variant UUID\n"}, @@ -2228,7 +2226,7 @@ func TestOperator_sync(t *testing.T) { History: []configv1.UpdateHistory{ {State: configv1.PartialUpdate, Image: "image/image:v4.0.1", Version: "0.0.1-abc", StartedTime: defaultStartedTime}, }, - Desired: configv1.Update{Image: "image/image:v4.0.1", Version: "0.0.1-abc"}, + Desired: configv1.Update{Image: "image/image:v4.0.1"}, VersionHash: "", Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionFalse}, diff --git a/pkg/cvo/status.go b/pkg/cvo/status.go index 65e5df406a..f271054011 100644 --- a/pkg/cvo/status.go +++ b/pkg/cvo/status.go @@ -126,8 +126,6 @@ func mergeOperatorHistory(config *configv1.ClusterVersion, desired configv1.Upda // TODO: prune Z versions over transitions to Y versions, keep initial installed version pruneStatusHistory(config, 50) - - config.Status.Desired = desired } func pruneStatusHistory(config *configv1.ClusterVersion, maxHistory int) { @@ -181,6 +179,7 @@ func (optr *Operator) syncStatus(original, config *configv1.ClusterVersion, stat version := versionString(status.Actual) mergeOperatorHistory(config, status.Actual, status.Verified, now, status.Completed > 0) + config.Status.Desired = optr.currentVersion() // update validation errors var reason string @@ -400,7 +399,9 @@ func (optr *Operator) syncFailingStatus(original *configv1.ClusterVersion, ierr LastTransitionTime: now, }) - mergeOperatorHistory(config, optr.currentVersion(), false, now, false) + current := optr.currentVersion() + mergeOperatorHistory(config, current, false, now, false) + config.Status.Desired = current updated, err := applyClusterVersionStatus(optr.client.ConfigV1(), config, original) optr.rememberLastUpdate(updated) @@ -410,7 +411,7 @@ func (optr *Operator) syncFailingStatus(original *configv1.ClusterVersion, ierr return ierr } -// applyClusterVersionStatus attempts to overwrite the status subresource of required. If +// applyClusterVersionStatus attempts to overwrite the status subresource, if required. If // original is provided it is compared to required and no update will be made if the // object does not change. The method will retry a conflict by retrieving the latest live // version and updating the metadata of required. required is modified if the object on From 23fa499692aefbed3982b212d47df809f64c7cec Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Tue, 21 Apr 2020 12:17:50 -0700 Subject: [PATCH 2/2] WIP: integration fixups --- pkg/start/start_integration_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/start/start_integration_test.go b/pkg/start/start_integration_test.go index 446fba9d4d..adcc7dde1a 100644 --- a/pkg/start/start_integration_test.go +++ b/pkg/start/start_integration_test.go @@ -243,6 +243,7 @@ func TestIntegrationCVO_initializeAndUpgrade(t *testing.T) { worker := cvo.NewSyncWorker(retriever, cvo.NewResourceBuilder(cfg, cfg, nil), 5*time.Second, wait.Backoff{Steps: 3}, "") controllers.CVO.SetSyncWorkerForTesting(worker) + current := controllers.CVO.currentVersion() ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -259,7 +260,7 @@ func TestIntegrationCVO_initializeAndUpgrade(t *testing.T) { t.Logf("verify the available cluster version's status matches our expectations") t.Logf("Cluster version:\n%s", printCV(lastCV)) - verifyClusterVersionStatus(t, lastCV, configv1.Update{Image: payloadImage1, Version: "0.0.1"}, 1) + verifyClusterVersionStatus(t, lastCV, current, 1) verifyReleasePayload(t, kc, ns, "0.0.1", payloadImage1) t.Logf("wait for the next resync and verify that status didn't change")