From e94580f609e346bf8bee71ef2e362bf76cc8b0df Mon Sep 17 00:00:00 2001 From: Jan Chaloupka Date: Thu, 25 Jul 2024 11:19:43 +0200 Subject: [PATCH 1/6] TestCVO: set known caps from configv1.KnownClusterVersionCapabilities --- pkg/cvo/cvo_scenarios_test.go | 120 ++++++++++++++++++---------------- 1 file changed, 62 insertions(+), 58 deletions(-) diff --git a/pkg/cvo/cvo_scenarios_test.go b/pkg/cvo/cvo_scenarios_test.go index dfd8eb32f1..6afe12469d 100644 --- a/pkg/cvo/cvo_scenarios_test.go +++ b/pkg/cvo/cvo_scenarios_test.go @@ -35,6 +35,7 @@ import ( var architecture string var sortedCaps = configv1.ClusterVersionCapabilitySets[configv1.ClusterVersionCapabilitySetCurrent] +var sortedKnownCaps = configv1.KnownClusterVersionCapabilities func init() { architecture = runtime.GOARCH @@ -42,6 +43,9 @@ func init() { sort.Slice(sortedCaps, func(i, j int) bool { return sortedCaps[i] < sortedCaps[j] }) + sort.Slice(sortedKnownCaps, func(i, j int) bool { + return sortedKnownCaps[i] < sortedKnownCaps[j] + }) } func setupCVOTest(payloadDir string) (*Operator, map[string]apiruntime.Object, *fake.Clientset, *dynamicfake.FakeDynamicClient, func()) { @@ -226,7 +230,7 @@ func TestCVO_StartupAndSync(t *testing.T) { }, Capabilities: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: ImplicitlyEnabledCapabilities, Status: "False", Reason: "AsExpected", Message: "Capabilities match configured spec"}, @@ -283,7 +287,7 @@ func TestCVO_StartupAndSync(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, }, @@ -308,7 +312,7 @@ func TestCVO_StartupAndSync(t *testing.T) { }, CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, EnabledCapabilities: sortedCaps, }, }, @@ -335,7 +339,7 @@ func TestCVO_StartupAndSync(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, }, @@ -362,7 +366,7 @@ func TestCVO_StartupAndSync(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, }, @@ -403,7 +407,7 @@ func TestCVO_StartupAndSync(t *testing.T) { }, VersionHash: "DL-FFQ2Uem8=", Capabilities: configv1.ClusterVersionCapabilitiesStatus{ - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, EnabledCapabilities: sortedCaps, }, History: []configv1.UpdateHistory{ @@ -438,7 +442,7 @@ func TestCVO_StartupAndSync(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -464,7 +468,7 @@ func TestCVO_StartupAndSync(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -490,7 +494,7 @@ func TestCVO_StartupAndSync(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -517,7 +521,7 @@ func TestCVO_StartupAndSync(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -642,7 +646,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { {State: configv1.PartialUpdate, Image: "image/image:1", Version: "1.0.0-abc", StartedTime: defaultStartedTime, AcceptedRisks: "The update cannot be verified: some random error"}, }, Capabilities: configv1.ClusterVersionCapabilitiesStatus{ - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, EnabledCapabilities: sortedCaps, }, Conditions: []configv1.ClusterOperatorStatusCondition{ @@ -699,7 +703,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -727,7 +731,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -755,7 +759,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -810,7 +814,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { VersionHash: "DL-FFQ2Uem8=", Capabilities: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Image: "image/image:1", Version: "1.0.0-abc", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime, AcceptedRisks: "The update cannot be verified: some random error"}, @@ -844,7 +848,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -872,7 +876,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -900,7 +904,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -929,7 +933,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -1047,7 +1051,7 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { }, Capabilities: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: ImplicitlyEnabledCapabilities, Status: "False", Reason: "AsExpected", Message: "Capabilities match configured spec"}, @@ -1109,7 +1113,7 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, }, @@ -1136,7 +1140,7 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, }, @@ -1156,7 +1160,7 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -1209,7 +1213,7 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { VersionHash: "DL-FFQ2Uem8=", Capabilities: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Image: "image/image:1", Version: "1.0.0-abc", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, @@ -1243,7 +1247,7 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -1270,7 +1274,7 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -1297,7 +1301,7 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -1325,7 +1329,7 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -1541,7 +1545,7 @@ func TestCVO_UpgradeUnverifiedPayload(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -1588,7 +1592,7 @@ func TestCVO_UpgradeUnverifiedPayload(t *testing.T) { VersionHash: "DL-FFQ2Uem8=", Capabilities: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Image: "image/image:1", Version: "1.0.1-abc", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime, AcceptedRisks: "The update cannot be verified: some random error"}, @@ -1802,7 +1806,7 @@ func TestCVO_ResetPayloadLoadStatus(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -1836,7 +1840,7 @@ func TestCVO_ResetPayloadLoadStatus(t *testing.T) { VersionHash: "DL-FFQ2Uem8=", Capabilities: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Image: "image/image:0", Version: "1.0.0-abc", Verified: true, StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, @@ -1973,7 +1977,7 @@ func TestCVO_UpgradeFailedPayloadLoadWithCapsChanges(t *testing.T) { // confirm capabilities are updated checkStatus(t, actions[1], "update", "clusterversions", "status", "", configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: []configv1.ClusterVersionCapability{configv1.ClusterVersionCapabilityIngress, configv1.ClusterVersionCapabilityBaremetal, configv1.ClusterVersionCapabilityMarketplace}, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }) } @@ -2077,7 +2081,7 @@ func TestCVO_InitImplicitlyEnabledCaps(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, ImplicitlyEnabledCaps: []configv1.ClusterVersionCapability{configv1.ClusterVersionCapabilityBuild, configv1.ClusterVersionCapabilityCSISnapshot, configv1.ClusterVersionCapabilityCloudControllerManager, configv1.ClusterVersionCapabilityCloudCredential, configv1.ClusterVersionCapabilityConsole, configv1.ClusterVersionCapabilityDeploymentConfig, configv1.ClusterVersionCapabilityImageRegistry, configv1.ClusterVersionCapabilityIngress, configv1.ClusterVersionCapabilityInsights, configv1.ClusterVersionCapabilityMachineAPI, configv1.ClusterVersionCapabilityNodeTuning, configv1.ClusterVersionCapabilityOperatorLifecycleManager, configv1.ClusterVersionCapabilityStorage, configv1.ClusterVersionCapabilityMarketplace, configv1.ClusterVersionCapabilityOpenShiftSamples}, }, @@ -2120,7 +2124,7 @@ func TestCVO_InitImplicitlyEnabledCaps(t *testing.T) { }, Capabilities: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: ImplicitlyEnabledCapabilities, Status: configv1.ConditionTrue, Reason: "CapabilitiesImplicitlyEnabled", Message: "The following capabilities could not be disabled: Build, CSISnapshot, CloudControllerManager, CloudCredential, Console, DeploymentConfig, ImageRegistry, Ingress, Insights, MachineAPI, NodeTuning, OperatorLifecycleManager, Storage, marketplace, openshift-samples"}, @@ -2332,7 +2336,7 @@ func TestCVO_UpgradeUnverifiedPayloadRetrieveOnce(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -2380,7 +2384,7 @@ func TestCVO_UpgradeUnverifiedPayloadRetrieveOnce(t *testing.T) { VersionHash: "DL-FFQ2Uem8=", Capabilities: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Image: "image/image:1", Version: "1.0.1-abc", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime, AcceptedRisks: "The update cannot be verified: some random error"}, @@ -2417,7 +2421,7 @@ func TestCVO_UpgradeUnverifiedPayloadRetrieveOnce(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -2592,7 +2596,7 @@ func TestCVO_UpgradePreconditionFailing(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -2616,7 +2620,7 @@ func TestCVO_UpgradePreconditionFailing(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -2640,7 +2644,7 @@ func TestCVO_UpgradePreconditionFailing(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -2687,7 +2691,7 @@ func TestCVO_UpgradePreconditionFailing(t *testing.T) { VersionHash: "DL-FFQ2Uem8=", Capabilities: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Image: "image/image:1", Version: "1.0.1-abc", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, @@ -2826,7 +2830,7 @@ func TestCVO_UpgradePreconditionFailingAcceptedRisks(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -2945,7 +2949,7 @@ func TestCVO_UpgradeVerifiedPayload(t *testing.T) { }, Capabilities: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: ImplicitlyEnabledCapabilities, Status: "False", Reason: "AsExpected", Message: "Capabilities match configured spec"}, @@ -3077,7 +3081,7 @@ func TestCVO_RestartAndReconcile(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -3102,7 +3106,7 @@ func TestCVO_RestartAndReconcile(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -3127,7 +3131,7 @@ func TestCVO_RestartAndReconcile(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -3170,7 +3174,7 @@ func TestCVO_RestartAndReconcile(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -3194,7 +3198,7 @@ func TestCVO_RestartAndReconcile(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -3219,7 +3223,7 @@ func TestCVO_RestartAndReconcile(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -3244,7 +3248,7 @@ func TestCVO_RestartAndReconcile(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -3403,7 +3407,7 @@ func TestCVO_ErrorDuringReconcile(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -3439,7 +3443,7 @@ func TestCVO_ErrorDuringReconcile(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -3490,7 +3494,7 @@ func TestCVO_ErrorDuringReconcile(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -3531,7 +3535,7 @@ func TestCVO_ErrorDuringReconcile(t *testing.T) { VersionHash: "DL-FFQ2Uem8=", Capabilities: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, History: []configv1.UpdateHistory{ {State: configv1.CompletedUpdate, Image: "image/image:1", Version: "1.0.0-abc", Verified: true, StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, @@ -3659,7 +3663,7 @@ func TestCVO_ParallelError(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -3695,7 +3699,7 @@ func TestCVO_ParallelError(t *testing.T) { CapabilitiesStatus: CapabilityStatus{ Status: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, }, loadPayloadStatus: LoadPayloadStatus{ @@ -3739,7 +3743,7 @@ func TestCVO_ParallelError(t *testing.T) { VersionHash: "Gyh2W6qcDO4=", Capabilities: configv1.ClusterVersionCapabilitiesStatus{ EnabledCapabilities: sortedCaps, - KnownCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, }, History: []configv1.UpdateHistory{ {State: configv1.PartialUpdate, Image: "image/image:1", Version: "1.0.0-abc", StartedTime: defaultStartedTime}, From a411e9da061c6ba1b71bb2b1ce5972a8e0c03307 Mon Sep 17 00:00:00 2001 From: Hongkai Liu Date: Tue, 10 Sep 2024 16:58:16 -0400 Subject: [PATCH 2/6] OCPBUGS-35228: Fix desired before sync_worker's work is initialized If the CVO pod gets restarted after a user sends a cluster upgrade request with `--to-image`, the desired should not ALWAYS be taken from `.Spec.DesiredUpdate`, e.g., when the targeting payload is blocked on some precondition check failure. The user request will be written to `cv.spec.desiredUpdate` where only the `image` field is set, i.e., the `Version` field is empty. If the CVO gets restarted, then 1. The desied [1] from `cv.spec.desiredUpdate` will be used to set `w.status.Actual` [2] and then applied to `cv.status.desired`. 2. When the next `optr.sync()` happens, the precondition check may become non-blocking [3] as the `Version` field is empty now. 3. CVO will then start an upgrade that should have been blocked. [1]. https://github.com/openshift/cluster-version-operator/blob/1995380b6d755c29b926b846a64ca0039002c2cf/pkg/cvo/cvo.go#L680 [2]. https://github.com/openshift/cluster-version-operator/blob/1995380b6d755c29b926b846a64ca0039002c2cf/pkg/cvo/sync_worker.go#L502 [3]. https://github.com/openshift/cluster-version-operator/blob/1995380b6d755c29b926b846a64ca0039002c2cf/pkg/payload/precondition/clusterversion/rollback.go#L57 --- pkg/cvo/cvo.go | 16 +++++++++++++--- pkg/cvo/sync_worker.go | 6 ++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/pkg/cvo/cvo.go b/pkg/cvo/cvo.go index fa92bca901..70c5ece0df 100644 --- a/pkg/cvo/cvo.go +++ b/pkg/cvo/cvo.go @@ -678,15 +678,25 @@ func (optr *Operator) sync(ctx context.Context, key string) error { // identify the desired next version desired, ok := findUpdateFromConfig(config, optr.getArchitecture()) - if ok { - klog.V(2).Infof("Desired version from spec is %#v", desired) + stillInitializing := optr.configSync.StillInitializing() + if ok && !stillInitializing { + klog.V(2).Infof("Desired version from spec is %#v after initialization", desired) } else { + userDesired := desired currentVersion := optr.currentVersion() desired = configv1.Update{ Version: currentVersion.Version, Image: currentVersion.Image, } - klog.V(2).Infof("Desired version from operator is %#v", desired) + if ok { + // It implies stillInitializing == true + klog.V(2).Infof("Desired version from operator is %#v with user's request to go to %#v. "+ + "We are currently initializing the work for the request and will evaluate the version later", desired, userDesired) + // enqueue to trigger a reconciliation on ClusterVersion + optr.queue.Add(optr.queueKey()) + } else { + klog.V(2).Infof("Desired version from operator is %#v", desired) + } } // handle the case of a misconfigured CVO by doing nothing diff --git a/pkg/cvo/sync_worker.go b/pkg/cvo/sync_worker.go index 8c66c3469c..da727fd14b 100644 --- a/pkg/cvo/sync_worker.go +++ b/pkg/cvo/sync_worker.go @@ -35,6 +35,8 @@ type ConfigSyncWorker interface { // NotifyAboutManagedResourceActivity informs the sync worker about activity for a managed resource. NotifyAboutManagedResourceActivity(msg string) + // StillInitializing returns true if ConfigSyncWorker has no work to do yet + StillInitializing() bool } // PayloadInfo returns details about the payload when it was retrieved. @@ -227,6 +229,10 @@ func (w *SyncWorker) StatusCh() <-chan SyncWorkerStatus { return w.report } +func (w *SyncWorker) StillInitializing() bool { + return w.work == nil +} + // NotifyAboutManagedResourceActivity informs the sync worker about activity for a managed resource. func (w *SyncWorker) NotifyAboutManagedResourceActivity(message string) { select { From 6906bbfb9d075450e93680cdf8ccfc51d4e62c24 Mon Sep 17 00:00:00 2001 From: Hongkai Liu Date: Tue, 10 Sep 2024 21:47:04 -0400 Subject: [PATCH 3/6] Fix unit tests --- pkg/cvo/cvo.go | 2 +- pkg/cvo/cvo_scenarios_test.go | 9 ++++++++- pkg/cvo/sync_test.go | 14 ++++++++++++-- pkg/cvo/sync_worker.go | 15 +++++++++++---- 4 files changed, 32 insertions(+), 8 deletions(-) diff --git a/pkg/cvo/cvo.go b/pkg/cvo/cvo.go index 70c5ece0df..ce6def7027 100644 --- a/pkg/cvo/cvo.go +++ b/pkg/cvo/cvo.go @@ -678,7 +678,7 @@ func (optr *Operator) sync(ctx context.Context, key string) error { // identify the desired next version desired, ok := findUpdateFromConfig(config, optr.getArchitecture()) - stillInitializing := optr.configSync.StillInitializing() + stillInitializing := optr.configSync.StillInitializingFunc()() if ok && !stillInitializing { klog.V(2).Infof("Desired version from spec is %#v after initialization", desired) } else { diff --git a/pkg/cvo/cvo_scenarios_test.go b/pkg/cvo/cvo_scenarios_test.go index 6afe12469d..f79d20a798 100644 --- a/pkg/cvo/cvo_scenarios_test.go +++ b/pkg/cvo/cvo_scenarios_test.go @@ -1402,6 +1402,7 @@ func TestCVO_UpgradeUnverifiedPayload(t *testing.T) { t.Fatal("not the correct error type") } worker := o.configSync.(*SyncWorker) + worker.stillInitializingFunc = func() bool { return false } retriever := worker.retriever.(*fakeDirectoryRetriever) retriever.Set(PayloadInfo{}, payloadErr) @@ -1657,7 +1658,7 @@ func TestCVO_ResetPayloadLoadStatus(t *testing.T) { t.Fatal("not the correct error type") } worker := o.configSync.(*SyncWorker) - + worker.stillInitializingFunc = func() bool { return false } // checked by SyncWorker.syncPayload worker.payload = &payload.Update{Release: o.release} @@ -1908,6 +1909,7 @@ func TestCVO_UpgradeFailedPayloadLoadWithCapsChanges(t *testing.T) { t.Fatal("not the correct error type") } worker := o.configSync.(*SyncWorker) + worker.stillInitializingFunc = func() bool { return false } retriever := worker.retriever.(*fakeDirectoryRetriever) retriever.Set(PayloadInfo{}, payloadErr) @@ -2026,6 +2028,7 @@ func TestCVO_InitImplicitlyEnabledCaps(t *testing.T) { defer shutdownFn() worker := o.configSync.(*SyncWorker) + worker.stillInitializingFunc = func() bool { return false } go worker.Start(ctx, 1) @@ -2192,6 +2195,7 @@ func TestCVO_UpgradeUnverifiedPayloadRetrieveOnce(t *testing.T) { t.Fatal("not the correct error type") } worker := o.configSync.(*SyncWorker) + worker.stillInitializingFunc = func() bool { return false } retriever := worker.retriever.(*fakeDirectoryRetriever) retriever.Set(PayloadInfo{}, payloadErr) @@ -2480,6 +2484,7 @@ func TestCVO_UpgradePreconditionFailing(t *testing.T) { defer shutdownFn() worker := o.configSync.(*SyncWorker) + worker.stillInitializingFunc = func() bool { return false } worker.preconditions = []precondition.Precondition{&testPrecondition{SuccessAfter: 3}} go worker.Start(ctx, 1) @@ -2754,6 +2759,7 @@ func TestCVO_UpgradePreconditionFailingAcceptedRisks(t *testing.T) { defer shutdownFn() worker := o.configSync.(*SyncWorker) + worker.stillInitializingFunc = func() bool { return false } worker.preconditions = []precondition.Precondition{&testPreconditionAlwaysFail{PreConditionName: "PreCondition1"}, &testPreconditionAlwaysFail{PreConditionName: "PreCondition2"}} go worker.Start(ctx, 1) @@ -2909,6 +2915,7 @@ func TestCVO_UpgradeVerifiedPayload(t *testing.T) { t.Fatal("not the correct error type") } worker := o.configSync.(*SyncWorker) + worker.stillInitializingFunc = func() bool { return false } retriever := worker.retriever.(*fakeDirectoryRetriever) retriever.Set(PayloadInfo{}, payloadErr) retriever.Set(PayloadInfo{Directory: "testdata/payloadtest-2", Verified: true}, nil) diff --git a/pkg/cvo/sync_test.go b/pkg/cvo/sync_test.go index 84f1f6e62c..db1ea52b3e 100644 --- a/pkg/cvo/sync_test.go +++ b/pkg/cvo/sync_test.go @@ -471,8 +471,18 @@ func newAction(gvk schema.GroupVersionKind, namespace, name string) action { } type fakeSyncRecorder struct { - Returns *SyncWorkerStatus - Updates []configv1.Update + Returns *SyncWorkerStatus + Updates []configv1.Update + stillInitializingFunc func() bool +} + +func (r *fakeSyncRecorder) StillInitializingFunc() func() bool { + if r.stillInitializingFunc != nil { + return r.stillInitializingFunc + } + return func() bool { + return false + } } func (r *fakeSyncRecorder) StatusCh() <-chan SyncWorkerStatus { diff --git a/pkg/cvo/sync_worker.go b/pkg/cvo/sync_worker.go index da727fd14b..6b7a1a1693 100644 --- a/pkg/cvo/sync_worker.go +++ b/pkg/cvo/sync_worker.go @@ -35,8 +35,8 @@ type ConfigSyncWorker interface { // NotifyAboutManagedResourceActivity informs the sync worker about activity for a managed resource. NotifyAboutManagedResourceActivity(msg string) - // StillInitializing returns true if ConfigSyncWorker has no work to do yet - StillInitializing() bool + // StillInitializingFunc a function that returns true if ConfigSyncWorker has no work to do yet + StillInitializingFunc() func() bool } // PayloadInfo returns details about the payload when it was retrieved. @@ -188,6 +188,8 @@ type SyncWorker struct { // always be implicitly enabled. // This contributes to whether or not some manifests are included for reconciliation. alwaysEnableCapabilities []configv1.ClusterVersionCapability + + stillInitializingFunc func() bool } // NewSyncWorker initializes a ConfigSyncWorker that will retrieve payloads to disk, apply them via builder @@ -229,8 +231,13 @@ func (w *SyncWorker) StatusCh() <-chan SyncWorkerStatus { return w.report } -func (w *SyncWorker) StillInitializing() bool { - return w.work == nil +func (w *SyncWorker) StillInitializingFunc() func() bool { + if w.stillInitializingFunc != nil { + return w.stillInitializingFunc + } + return func() bool { + return w.work == nil + } } // NotifyAboutManagedResourceActivity informs the sync worker about activity for a managed resource. From e3a05d439a609e1eb5fc3c63c70a7825ee48f41f Mon Sep 17 00:00:00 2001 From: Hongkai Liu Date: Wed, 11 Sep 2024 10:14:31 -0400 Subject: [PATCH 4/6] Add test to cover the case of stillInitializing == true --- pkg/cvo/cvo_scenarios_test.go | 108 ++++++++++++++++++++++++++++++++++ 1 file changed, 108 insertions(+) diff --git a/pkg/cvo/cvo_scenarios_test.go b/pkg/cvo/cvo_scenarios_test.go index f79d20a798..97378199c8 100644 --- a/pkg/cvo/cvo_scenarios_test.go +++ b/pkg/cvo/cvo_scenarios_test.go @@ -2862,6 +2862,114 @@ func TestCVO_UpgradePreconditionFailingAcceptedRisks(t *testing.T) { }) } +func TestCVO_UpgradeVerifiedPayloadStillInitializing(t *testing.T) { + o, cvs, client, _, shutdownFn := setupCVOTest("testdata/payloadtest") + + // Setup: a successful sync from a previous run, and the operator at the same image as before + // + o.release.Image = "image/image:0" + o.release.Version = "1.0.0-abc" + desired := configv1.Release{Version: "1.0.1-abc", Image: "image/image:1"} + uid, _ := uuid.NewRandom() + clusterUID := configv1.ClusterID(uid.String()) + cvs["version"] = &configv1.ClusterVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "version", + ResourceVersion: "1", + Generation: 1, + }, + Spec: configv1.ClusterVersionSpec{ + ClusterID: clusterUID, + Channel: "fast", + DesiredUpdate: &configv1.Update{Version: desired.Version, Image: desired.Image}, + }, + Status: configv1.ClusterVersionStatus{ + // Prefers the image version over the operator's version (although in general they will remain in sync) + Desired: desired, + VersionHash: "DL-FFQ2Uem8=", + History: []configv1.UpdateHistory{ + {State: configv1.CompletedUpdate, Image: "image/image:0", Version: "1.0.0-abc", Verified: true, StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, + }, + Conditions: []configv1.ClusterOperatorStatusCondition{ + {Type: ImplicitlyEnabledCapabilities, Status: "False", Reason: "AsExpected", Message: "Capabilities match configured spec"}, + {Type: configv1.OperatorAvailable, Status: configv1.ConditionTrue, Message: "Done applying 1.0.0-abc"}, + {Type: ClusterStatusFailing, Status: configv1.ConditionFalse}, + {Type: configv1.OperatorProgressing, Status: configv1.ConditionFalse, Message: "Cluster version is 1.0.0-abc"}, + {Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse}, + }, + }, + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + defer shutdownFn() + + // make the image report unverified + payloadErr := &payload.UpdateError{ + Reason: "ImageVerificationFailed", + Message: "The update cannot be verified: some random error", + Nested: fmt.Errorf("some random error"), + } + if !isImageVerificationError(payloadErr) { + t.Fatal("not the correct error type") + } + worker := o.configSync.(*SyncWorker) + retriever := worker.retriever.(*fakeDirectoryRetriever) + retriever.Set(PayloadInfo{}, payloadErr) + retriever.Set(PayloadInfo{Directory: "testdata/payloadtest", Verified: true}, nil) + + go worker.Start(ctx, 1) + + // Step 1: Simulate a verified payload being retrieved and ensure the operator sets verified + // + client.ClearActions() + err := o.sync(ctx, o.queueKey()) + if err != nil { + t.Fatal(err) + } + actions := client.Actions() + if len(actions) != 2 { + t.Fatalf("%s", spew.Sdump(actions)) + } + expectGet(t, actions[0], "clusterversions", "", "version") + expectUpdateStatus(t, actions[1], "clusterversions", "", &configv1.ClusterVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "version", + ResourceVersion: "1", + Generation: 1, + }, + Spec: configv1.ClusterVersionSpec{ + ClusterID: clusterUID, + Channel: "fast", + DesiredUpdate: &configv1.Update{Version: desired.Version, Image: desired.Image}, + }, + Status: configv1.ClusterVersionStatus{ + ObservedGeneration: 1, + // Prefers the operator's version + Desired: configv1.Release{Version: o.release.Version, Image: o.release.Image}, + VersionHash: "DL-FFQ2Uem8=", + History: []configv1.UpdateHistory{ + {State: configv1.CompletedUpdate, Image: "image/image:0", Version: "1.0.0-abc", Verified: true, StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, + }, + Capabilities: configv1.ClusterVersionCapabilitiesStatus{ + EnabledCapabilities: sortedCaps, + KnownCapabilities: sortedKnownCaps, + }, + Conditions: []configv1.ClusterOperatorStatusCondition{ + {Type: ImplicitlyEnabledCapabilities, Status: "False", Reason: "AsExpected", Message: "Capabilities match configured spec"}, + {Type: configv1.OperatorAvailable, Status: configv1.ConditionTrue, Message: "Done applying 1.0.0-abc"}, + // cleared failing status and set progressing + {Type: ClusterStatusFailing, Status: configv1.ConditionFalse}, + {Type: configv1.OperatorProgressing, Status: configv1.ConditionFalse, Message: "Cluster version is 1.0.0-abc"}, + {Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse}, + {Type: DesiredReleaseAccepted, Status: configv1.ConditionTrue, Reason: "PayloadLoaded", + Message: `Payload loaded version="1.0.0-abc" image="image/image:0" architecture="` + architecture + `"`}, + }, + }, + }) +} + func TestCVO_UpgradeVerifiedPayload(t *testing.T) { o, cvs, client, _, shutdownFn := setupCVOTest("testdata/payloadtest-2") From a1ee1ca6c050b752c622264c6903823210671334 Mon Sep 17 00:00:00 2001 From: Hongkai Liu Date: Thu, 12 Sep 2024 15:34:38 -0400 Subject: [PATCH 5/6] Better naming of variables and functions --- pkg/cvo/cvo.go | 13 ++++++------- pkg/cvo/cvo_scenarios_test.go | 16 ++++++++-------- pkg/cvo/sync_test.go | 15 +++++---------- pkg/cvo/sync_worker.go | 17 ++++++++--------- 4 files changed, 27 insertions(+), 34 deletions(-) diff --git a/pkg/cvo/cvo.go b/pkg/cvo/cvo.go index ce6def7027..675fde10a7 100644 --- a/pkg/cvo/cvo.go +++ b/pkg/cvo/cvo.go @@ -677,21 +677,20 @@ func (optr *Operator) sync(ctx context.Context, key string) error { config := validation.ClearInvalidFields(original, errs) // identify the desired next version - desired, ok := findUpdateFromConfig(config, optr.getArchitecture()) - stillInitializing := optr.configSync.StillInitializingFunc()() - if ok && !stillInitializing { + desired, found := findUpdateFromConfig(config, optr.getArchitecture()) + initialized := optr.configSync.Initialized() + if found && initialized { klog.V(2).Infof("Desired version from spec is %#v after initialization", desired) } else { - userDesired := desired + pendingDesired := desired currentVersion := optr.currentVersion() desired = configv1.Update{ Version: currentVersion.Version, Image: currentVersion.Image, } - if ok { - // It implies stillInitializing == true + if !initialized { klog.V(2).Infof("Desired version from operator is %#v with user's request to go to %#v. "+ - "We are currently initializing the work for the request and will evaluate the version later", desired, userDesired) + "We are currently initializing the work for the request and will evaluate the version later", desired, pendingDesired) // enqueue to trigger a reconciliation on ClusterVersion optr.queue.Add(optr.queueKey()) } else { diff --git a/pkg/cvo/cvo_scenarios_test.go b/pkg/cvo/cvo_scenarios_test.go index 97378199c8..95a0bebf95 100644 --- a/pkg/cvo/cvo_scenarios_test.go +++ b/pkg/cvo/cvo_scenarios_test.go @@ -1402,7 +1402,7 @@ func TestCVO_UpgradeUnverifiedPayload(t *testing.T) { t.Fatal("not the correct error type") } worker := o.configSync.(*SyncWorker) - worker.stillInitializingFunc = func() bool { return false } + worker.initializedFunc = func() bool { return true } retriever := worker.retriever.(*fakeDirectoryRetriever) retriever.Set(PayloadInfo{}, payloadErr) @@ -1658,7 +1658,7 @@ func TestCVO_ResetPayloadLoadStatus(t *testing.T) { t.Fatal("not the correct error type") } worker := o.configSync.(*SyncWorker) - worker.stillInitializingFunc = func() bool { return false } + worker.initializedFunc = func() bool { return true } // checked by SyncWorker.syncPayload worker.payload = &payload.Update{Release: o.release} @@ -1909,7 +1909,7 @@ func TestCVO_UpgradeFailedPayloadLoadWithCapsChanges(t *testing.T) { t.Fatal("not the correct error type") } worker := o.configSync.(*SyncWorker) - worker.stillInitializingFunc = func() bool { return false } + worker.initializedFunc = func() bool { return true } retriever := worker.retriever.(*fakeDirectoryRetriever) retriever.Set(PayloadInfo{}, payloadErr) @@ -2028,7 +2028,7 @@ func TestCVO_InitImplicitlyEnabledCaps(t *testing.T) { defer shutdownFn() worker := o.configSync.(*SyncWorker) - worker.stillInitializingFunc = func() bool { return false } + worker.initializedFunc = func() bool { return true } go worker.Start(ctx, 1) @@ -2195,7 +2195,7 @@ func TestCVO_UpgradeUnverifiedPayloadRetrieveOnce(t *testing.T) { t.Fatal("not the correct error type") } worker := o.configSync.(*SyncWorker) - worker.stillInitializingFunc = func() bool { return false } + worker.initializedFunc = func() bool { return true } retriever := worker.retriever.(*fakeDirectoryRetriever) retriever.Set(PayloadInfo{}, payloadErr) @@ -2484,7 +2484,7 @@ func TestCVO_UpgradePreconditionFailing(t *testing.T) { defer shutdownFn() worker := o.configSync.(*SyncWorker) - worker.stillInitializingFunc = func() bool { return false } + worker.initializedFunc = func() bool { return true } worker.preconditions = []precondition.Precondition{&testPrecondition{SuccessAfter: 3}} go worker.Start(ctx, 1) @@ -2759,7 +2759,7 @@ func TestCVO_UpgradePreconditionFailingAcceptedRisks(t *testing.T) { defer shutdownFn() worker := o.configSync.(*SyncWorker) - worker.stillInitializingFunc = func() bool { return false } + worker.initializedFunc = func() bool { return true } worker.preconditions = []precondition.Precondition{&testPreconditionAlwaysFail{PreConditionName: "PreCondition1"}, &testPreconditionAlwaysFail{PreConditionName: "PreCondition2"}} go worker.Start(ctx, 1) @@ -3023,7 +3023,7 @@ func TestCVO_UpgradeVerifiedPayload(t *testing.T) { t.Fatal("not the correct error type") } worker := o.configSync.(*SyncWorker) - worker.stillInitializingFunc = func() bool { return false } + worker.initializedFunc = func() bool { return true } retriever := worker.retriever.(*fakeDirectoryRetriever) retriever.Set(PayloadInfo{}, payloadErr) retriever.Set(PayloadInfo{Directory: "testdata/payloadtest-2", Verified: true}, nil) diff --git a/pkg/cvo/sync_test.go b/pkg/cvo/sync_test.go index db1ea52b3e..77476ab684 100644 --- a/pkg/cvo/sync_test.go +++ b/pkg/cvo/sync_test.go @@ -471,18 +471,13 @@ func newAction(gvk schema.GroupVersionKind, namespace, name string) action { } type fakeSyncRecorder struct { - Returns *SyncWorkerStatus - Updates []configv1.Update - stillInitializingFunc func() bool + Returns *SyncWorkerStatus + Updates []configv1.Update + initializedFunc func() bool } -func (r *fakeSyncRecorder) StillInitializingFunc() func() bool { - if r.stillInitializingFunc != nil { - return r.stillInitializingFunc - } - return func() bool { - return false - } +func (r *fakeSyncRecorder) Initialized() bool { + return r.initializedFunc == nil || r.initializedFunc() } func (r *fakeSyncRecorder) StatusCh() <-chan SyncWorkerStatus { diff --git a/pkg/cvo/sync_worker.go b/pkg/cvo/sync_worker.go index 6b7a1a1693..d32801d077 100644 --- a/pkg/cvo/sync_worker.go +++ b/pkg/cvo/sync_worker.go @@ -35,8 +35,8 @@ type ConfigSyncWorker interface { // NotifyAboutManagedResourceActivity informs the sync worker about activity for a managed resource. NotifyAboutManagedResourceActivity(msg string) - // StillInitializingFunc a function that returns true if ConfigSyncWorker has no work to do yet - StillInitializingFunc() func() bool + // Initialized returns true if ConfigSyncWorker has work to do already + Initialized() bool } // PayloadInfo returns details about the payload when it was retrieved. @@ -189,7 +189,8 @@ type SyncWorker struct { // This contributes to whether or not some manifests are included for reconciliation. alwaysEnableCapabilities []configv1.ClusterVersionCapability - stillInitializingFunc func() bool + // initializedFunc is only for the unit-test purpose + initializedFunc func() bool } // NewSyncWorker initializes a ConfigSyncWorker that will retrieve payloads to disk, apply them via builder @@ -231,13 +232,11 @@ func (w *SyncWorker) StatusCh() <-chan SyncWorkerStatus { return w.report } -func (w *SyncWorker) StillInitializingFunc() func() bool { - if w.stillInitializingFunc != nil { - return w.stillInitializingFunc - } - return func() bool { - return w.work == nil +func (w *SyncWorker) Initialized() bool { + if w.initializedFunc != nil { + return w.initializedFunc() } + return w.work != nil } // NotifyAboutManagedResourceActivity informs the sync worker about activity for a managed resource. From ae4e180b19aa4f67f2a53785813ff040ee1690da Mon Sep 17 00:00:00 2001 From: Hongkai Liu Date: Tue, 17 Sep 2024 10:08:20 -0400 Subject: [PATCH 6/6] Improve some wording and simplify unit tests --- pkg/cvo/cvo.go | 2 +- pkg/cvo/cvo_scenarios_test.go | 23 ++++++++--------------- pkg/cvo/sync_worker.go | 2 +- 3 files changed, 10 insertions(+), 17 deletions(-) diff --git a/pkg/cvo/cvo.go b/pkg/cvo/cvo.go index 675fde10a7..08733b8d9f 100644 --- a/pkg/cvo/cvo.go +++ b/pkg/cvo/cvo.go @@ -690,7 +690,7 @@ func (optr *Operator) sync(ctx context.Context, key string) error { } if !initialized { klog.V(2).Infof("Desired version from operator is %#v with user's request to go to %#v. "+ - "We are currently initializing the work for the request and will evaluate the version later", desired, pendingDesired) + "We are currently initializing the worker and will evaluate the request later", desired, pendingDesired) // enqueue to trigger a reconciliation on ClusterVersion optr.queue.Add(optr.queueKey()) } else { diff --git a/pkg/cvo/cvo_scenarios_test.go b/pkg/cvo/cvo_scenarios_test.go index 95a0bebf95..e388aa8030 100644 --- a/pkg/cvo/cvo_scenarios_test.go +++ b/pkg/cvo/cvo_scenarios_test.go @@ -2862,10 +2862,10 @@ func TestCVO_UpgradePreconditionFailingAcceptedRisks(t *testing.T) { }) } -func TestCVO_UpgradeVerifiedPayloadStillInitializing(t *testing.T) { +func TestCVO_UpgradePayloadStillInitializing(t *testing.T) { o, cvs, client, _, shutdownFn := setupCVOTest("testdata/payloadtest") - // Setup: a successful sync from a previous run, and the operator at the same image as before + // Setup: an upgrade request from user to a new image and the operator at the same image as before // o.release.Image = "image/image:0" o.release.Version = "1.0.0-abc" @@ -2905,24 +2905,14 @@ func TestCVO_UpgradeVerifiedPayloadStillInitializing(t *testing.T) { defer shutdownFn() - // make the image report unverified - payloadErr := &payload.UpdateError{ - Reason: "ImageVerificationFailed", - Message: "The update cannot be verified: some random error", - Nested: fmt.Errorf("some random error"), - } - if !isImageVerificationError(payloadErr) { - t.Fatal("not the correct error type") - } worker := o.configSync.(*SyncWorker) retriever := worker.retriever.(*fakeDirectoryRetriever) - retriever.Set(PayloadInfo{}, payloadErr) retriever.Set(PayloadInfo{Directory: "testdata/payloadtest", Verified: true}, nil) go worker.Start(ctx, 1) - // Step 1: Simulate a verified payload being retrieved and ensure the operator sets verified - // + // Step 1: Simulate a payload being retrieved while the sync worker is not initialized + // and ensure the desired version from the operator is taken from the operator and a reconciliation is enqueued client.ClearActions() err := o.sync(ctx, o.queueKey()) if err != nil { @@ -2959,7 +2949,6 @@ func TestCVO_UpgradeVerifiedPayloadStillInitializing(t *testing.T) { Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: ImplicitlyEnabledCapabilities, Status: "False", Reason: "AsExpected", Message: "Capabilities match configured spec"}, {Type: configv1.OperatorAvailable, Status: configv1.ConditionTrue, Message: "Done applying 1.0.0-abc"}, - // cleared failing status and set progressing {Type: ClusterStatusFailing, Status: configv1.ConditionFalse}, {Type: configv1.OperatorProgressing, Status: configv1.ConditionFalse, Message: "Cluster version is 1.0.0-abc"}, {Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse}, @@ -2968,6 +2957,10 @@ func TestCVO_UpgradeVerifiedPayloadStillInitializing(t *testing.T) { }, }, }) + if l := o.queue.Len(); l != 1 { + t.Errorf("expecting queue length is 1 but got %d", l) + } + } func TestCVO_UpgradeVerifiedPayload(t *testing.T) { diff --git a/pkg/cvo/sync_worker.go b/pkg/cvo/sync_worker.go index d32801d077..59ff726458 100644 --- a/pkg/cvo/sync_worker.go +++ b/pkg/cvo/sync_worker.go @@ -35,7 +35,7 @@ type ConfigSyncWorker interface { // NotifyAboutManagedResourceActivity informs the sync worker about activity for a managed resource. NotifyAboutManagedResourceActivity(msg string) - // Initialized returns true if ConfigSyncWorker has work to do already + // Initialized returns true if the worker has work to do already Initialized() bool }