From f3c564de43dd9a1b1fb8bcc5a5a3a97c757e3f67 Mon Sep 17 00:00:00 2001 From: David Hurta Date: Mon, 24 Nov 2025 22:29:35 +0100 Subject: [PATCH 1/3] availableupdates_test: Add test cases for spec.DesiredUpdate The osusWithSingleConditionalEdge logic was broken down into smaller parts to allow for the creation of list of test cases with expected values utilized in the added test function. Introduce a test fixture to reduce the number of return values from the osusWithSingleConditionalEdge function. Assisted-by: Claude Code --- pkg/cvo/availableupdates_test.go | 444 ++++++++++++++++++++++++++----- 1 file changed, 384 insertions(+), 60 deletions(-) diff --git a/pkg/cvo/availableupdates_test.go b/pkg/cvo/availableupdates_test.go index c73ec59ef7..e8f9d85dd9 100644 --- a/pkg/cvo/availableupdates_test.go +++ b/pkg/cvo/availableupdates_test.go @@ -6,6 +6,8 @@ import ( "fmt" "net/http" "net/http/httptest" + "net/url" + "runtime" "testing" "time" @@ -46,18 +48,77 @@ func (n notFoundConfigMapLister) Get(name string) (*corev1.ConfigMap, error) { return nil, k8serrors.NewNotFound(schema.GroupResource{Group: "", Resource: "configmap"}, name) } -// osusWithSingleConditionalEdge helper returns: -// 1. mock osus server that serves a simple conditional path between two versions. -// 2. mock condition that always evaluates to match -// 3. expected []ConditionalUpdate data after evaluation of the data served by mock osus server -// (assuming the mock condition (2) was used) -// 4. current version of the cluster that would issue the request to the mock osus server -func osusWithSingleConditionalEdge() (*httptest.Server, clusterconditions.Condition, []configv1.ConditionalUpdate, string) { - from := "4.5.5" - to := "4.5.6" - osus := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { +type release struct { + version string + image string +} + +type conditionalEdgeTestData struct { + from release + to release +} + +// newConditionalEdgeTestData constructs test data for a conditional edge between two versions. +func newConditionalEdgeTestData(from, to string) conditionalEdgeTestData { + return conditionalEdgeTestData{ + from: release{ + version: from, + image: fmt.Sprintf("payload/%s", from), + }, + to: release{ + version: to, + image: fmt.Sprintf("payload/%s", to), + }, + } +} + +// defaultConditionalEdgeTestData returns test data for a conditional edge from 4.5.5 to 4.5.6. +func defaultConditionalEdgeTestData() conditionalEdgeTestData { + return newConditionalEdgeTestData("4.5.5", "4.5.6") +} + +// expectedConditionalUpdatesFor returns expected ConditionalUpdate data after evaluation +// (assuming the returned mock condition is used) and the mock PromQL condition checker itself. +func expectedConditionalUpdatesFor(data conditionalEdgeTestData) ([]configv1.ConditionalUpdate, *mock.Mock) { + return []configv1.ConditionalUpdate{ + { + Release: configv1.Release{Version: data.to.version, Image: data.to.image}, + Risks: []configv1.ConditionalUpdateRisk{ + { + URL: "https://example.com/" + data.to.version, + Name: "FourFiveSix", + Message: "Four Five Five is just fine", + MatchingRules: []configv1.ClusterCondition{ + { + Type: "PromQL", + PromQL: &configv1.PromQLClusterCondition{PromQL: "this is a query"}, + }, + }, + }, + }, + Conditions: []metav1.Condition{ + { + Type: "Recommended", + Status: metav1.ConditionFalse, + Reason: "FourFiveSix", + Message: "Four Five Five is just fine https://example.com/" + data.to.version, + LastTransitionTime: metav1.Now(), + }, + }, + }, + }, &mock.Mock{ + ValidQueue: []error{nil}, + MatchQueue: []mock.MatchResult{{Match: true, Error: nil}}, + } +} + +// newMockOSUSServer returns an OSUS server and query parameters used in the last call to the OSUS server. +func newMockOSUSServer(data conditionalEdgeTestData) (*httptest.Server, *url.Values) { + var params url.Values + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + params = r.URL.Query() fmt.Fprintf(w, `{ - "nodes": [{"version": "%s", "payload": "payload/%s"}, {"version": "%s", "payload": "payload/%s"}], + "nodes": [{"version": "%s", "payload": "%s"}, {"version": "%s", "payload": "%s"}], "conditionalEdges": [ { "edges": [{"from": "%s", "to": "%s"}], @@ -72,52 +133,60 @@ func osusWithSingleConditionalEdge() (*httptest.Server, clusterconditions.Condit } ] } -`, from, from, to, to, from, to, to) - })) +`, data.from.version, data.from.image, data.to.version, data.to.image, data.from.version, + data.to.version, data.to.version) + })), ¶ms +} - updates := []configv1.ConditionalUpdate{ - { - Release: configv1.Release{Version: to, Image: "payload/" + to}, - Risks: []configv1.ConditionalUpdateRisk{ - { - URL: "https://example.com/" + to, - Name: "FourFiveSix", - Message: "Four Five Five is just fine", - MatchingRules: []configv1.ClusterCondition{ - { - Type: "PromQL", - PromQL: &configv1.PromQLClusterCondition{PromQL: "this is a query"}, - }, - }, - }, - }, - Conditions: []metav1.Condition{ - { - Type: "Recommended", - Status: metav1.ConditionFalse, - Reason: "FourFiveSix", - Message: "Four Five Five is just fine https://example.com/" + to, - LastTransitionTime: metav1.Now(), - }, - }, +type testFixture struct { + // Mock osus server + osus *httptest.Server + // Query parameters used in the last call to the osus server + lastQueryParams *url.Values + + // Expected updates after evaluation of the mockCondition + expectedConditionalUpdates []configv1.ConditionalUpdate + mockCondition clusterconditions.Condition + + currentRelease release +} + +// osusWithSingleConditionalEdge helper returns: +// 1. mock osus server that serves a simple conditional path between two versions. +// 2. mock condition that always evaluates to match +// 3. expected []ConditionalUpdate data after evaluation of the data served by mock osus server +// (assuming the mock condition (2) was used) +// 4. current version of the cluster that would issue the request to the mock osus server +// 5. current image of the cluster +// 6. query parameters used in the last call to the osus server +func osusWithSingleConditionalEdge(data conditionalEdgeTestData) testFixture { + server, lastQueryParams := newMockOSUSServer(data) + expectedUpdates, mockPromql := expectedConditionalUpdatesFor(data) + return testFixture{ + osus: server, + lastQueryParams: lastQueryParams, + expectedConditionalUpdates: expectedUpdates, + mockCondition: mockPromql, + currentRelease: release{ + version: data.from.version, + image: data.from.image, }, } - mockPromql := &mock.Mock{ - ValidQueue: []error{nil}, - MatchQueue: []mock.MatchResult{{Match: true, Error: nil}}, - } - - return osus, mockPromql, updates, from } -func newOperator(url, version string, promqlMock clusterconditions.Condition) (*availableUpdates, *Operator) { - currentRelease := configv1.Release{Version: version, Image: "payload/" + version} +func newOperator(url string, cluster release, promqlMock clusterconditions.Condition, arch string) (*availableUpdates, *Operator) { + var currentReleaseArch configv1.ClusterVersionArchitecture + if arch == string(configv1.ClusterVersionArchitectureMulti) { + currentReleaseArch = configv1.ClusterVersionArchitectureMulti + } + currentRelease := configv1.Release{Version: cluster.version, Image: cluster.image, Architecture: currentReleaseArch} + registry := clusterconditions.NewConditionRegistry() registry.Register("Always", &always.Always{}) registry.Register("PromQL", promqlMock) operator := &Operator{ updateService: url, - architecture: "amd64", + architecture: arch, proxyLister: notFoundProxyLister{}, cmConfigManagedLister: notFoundConfigMapLister{}, conditionRegistry: registry, @@ -125,8 +194,7 @@ func newOperator(url, version string, promqlMock clusterconditions.Condition) (* release: currentRelease, } availableUpdates := &availableUpdates{ - Architecture: "amd64", - Current: configv1.Release{Version: version, Image: "payload/" + version}, + Current: configv1.Release{Version: cluster.version, Image: cluster.image}, } return availableUpdates, operator } @@ -146,12 +214,18 @@ var availableUpdatesCmpOpts = []cmp.Option{ } func TestSyncAvailableUpdates(t *testing.T) { - fakeOsus, mockPromql, expectedConditionalUpdates, version := osusWithSingleConditionalEdge() - defer fakeOsus.Close() - expectedAvailableUpdates, optr := newOperator(fakeOsus.URL, version, mockPromql) - expectedAvailableUpdates.UpdateService = fakeOsus.URL - expectedAvailableUpdates.ConditionalUpdates = expectedConditionalUpdates + fixture := osusWithSingleConditionalEdge(defaultConditionalEdgeTestData()) + defer fixture.osus.Close() + expectedAvailableUpdates, optr := newOperator( + fixture.osus.URL, + fixture.currentRelease, + fixture.mockCondition, + runtime.GOARCH, + ) + expectedAvailableUpdates.UpdateService = fixture.osus.URL + expectedAvailableUpdates.ConditionalUpdates = fixture.expectedConditionalUpdates expectedAvailableUpdates.Channel = cvFixture.Spec.Channel + expectedAvailableUpdates.Architecture = runtime.GOARCH expectedAvailableUpdates.Condition = configv1.ClusterOperatorStatusCondition{ Type: configv1.RetrievedUpdates, Status: configv1.ConditionTrue, @@ -228,16 +302,22 @@ func TestSyncAvailableUpdates_ConditionalUpdateRecommendedConditions(t *testing. for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - fakeOsus, mockPromql, conditionalUpdates, version := osusWithSingleConditionalEdge() - defer fakeOsus.Close() - availableUpdates, optr := newOperator(fakeOsus.URL, version, mockPromql) + fixture := osusWithSingleConditionalEdge(defaultConditionalEdgeTestData()) + defer fixture.osus.Close() + availableUpdates, optr := newOperator( + fixture.osus.URL, + fixture.currentRelease, + fixture.mockCondition, + runtime.GOARCH, + ) + availableUpdates.Architecture = runtime.GOARCH optr.availableUpdates = availableUpdates - optr.availableUpdates.ConditionalUpdates = conditionalUpdates + optr.availableUpdates.ConditionalUpdates = fixture.expectedConditionalUpdates expectedConditions := []metav1.Condition{{}} - conditionalUpdates[0].Conditions[0].DeepCopyInto(&expectedConditions[0]) + fixture.expectedConditionalUpdates[0].Conditions[0].DeepCopyInto(&expectedConditions[0]) cv := cvFixture.DeepCopy() tc.modifyOriginalState(optr) - tc.modifyCV(cv, conditionalUpdates[0]) + tc.modifyCV(cv, fixture.expectedConditionalUpdates[0]) err := optr.syncAvailableUpdates(context.Background(), cv) @@ -434,3 +514,247 @@ func TestEvaluateConditionalUpdate(t *testing.T) { }) } } + +func TestSyncAvailableUpdatesDesiredUpdate(t *testing.T) { + data := newConditionalEdgeTestData("4.5.5", "4.5.6") + + // used only by relevant test cases where the evaluation of conditional updates is expected + expectedConditionalUpdates, _ := expectedConditionalUpdatesFor(data) + + type args struct { + operatorArchitecture string + desiredUpdate *configv1.Update + } + type expected struct { + arch string + queryParamArch string + + updates []configv1.Release + conditionalUpdates []configv1.ConditionalUpdate + } + tests := []struct { + name string + args args + expected expected + }{ + // -------------------------------- Valid set desiredUpdate field combinations -------------------------------- + // Some combinations, such as all fields being set, are omitted due to them causing API validation errors; + // as such, they are not possible and are not explicitly tested here. + // + // ---------------- Cases where the operator is multi arch + { + name: "operator is multi, image is specified, version is specified, architecture is not specified", + args: args{ + operatorArchitecture: "Multi", + desiredUpdate: &configv1.Update{ + Version: data.to.version, + Image: data.to.image, + }, + }, + expected: expected{ + arch: "Multi", + conditionalUpdates: expectedConditionalUpdates, + queryParamArch: "multi", + }, + }, + { + name: "operator is multi, image is specified, version is not specified, architecture is not specified", + args: args{ + operatorArchitecture: "Multi", + desiredUpdate: &configv1.Update{ + Image: data.to.image, + }, + }, + expected: expected{ + arch: "Multi", + conditionalUpdates: expectedConditionalUpdates, + queryParamArch: "multi", + }, + }, + { + name: "operator is multi, image is not specified, version is specified, architecture is specified", + args: args{ + operatorArchitecture: "Multi", + desiredUpdate: &configv1.Update{ + Architecture: "Multi", + Version: data.to.version, + }, + }, + expected: expected{ + arch: "Multi", + conditionalUpdates: expectedConditionalUpdates, + queryParamArch: "multi", + }, + }, + { + name: "operator is multi, image is not specified, version is specified, architecture is not specified", + args: args{ + operatorArchitecture: "Multi", + desiredUpdate: &configv1.Update{ + Version: data.to.version, + }, + }, + expected: expected{ + arch: "Multi", + conditionalUpdates: expectedConditionalUpdates, + queryParamArch: "multi", + }, + }, + // ---------------- Cases where the operator is single arch + { + name: "operator is not multi, image is specified, version is specified, architecture is not specified", + args: args{ + operatorArchitecture: runtime.GOARCH, + desiredUpdate: &configv1.Update{ + Version: data.to.version, + Image: data.to.image, + }, + }, + expected: expected{ + arch: runtime.GOARCH, + conditionalUpdates: expectedConditionalUpdates, + queryParamArch: runtime.GOARCH, + }, + }, + { + name: "operator is not multi, image is specified, version is not specified, architecture is not specified", + args: args{ + operatorArchitecture: runtime.GOARCH, + desiredUpdate: &configv1.Update{ + Image: data.to.image, + }, + }, + expected: expected{ + arch: runtime.GOARCH, + conditionalUpdates: expectedConditionalUpdates, + queryParamArch: runtime.GOARCH, + }, + }, + { + name: "operator is not multi, image is not specified, version is specified, architecture is specified - migration to multi arch issued", + args: args{ + operatorArchitecture: runtime.GOARCH, + desiredUpdate: &configv1.Update{ + // Migration to multi-arch is issued + Architecture: "Multi", + Version: data.from.version, + }, + }, + expected: expected{ + arch: "Multi", + // Migrating from single to multi architecture. + // Only valid update for required heterogeneous graph is heterogeneous version of current version. + // + // Note: Testing utilises an OSUS with a single conditional edge and not individual graphs for + // individual architectures. To bypass the need for a more extensive testing logic, which would contain + // different release images for the same versions, there is a present logic to test for the query parameters. + // If the actual provided parameters are correct, we can act as if the graph references another images. + // + // However, it will not exercise any possible "my.image == update.image" conditions. + updates: []configv1.Release{{Version: data.from.version, Image: data.from.image}}, + queryParamArch: "multi", + }, + }, + { + name: "operator is not multi, image is not specified, version is specified, architecture is specified - migration && update", + args: args{ + operatorArchitecture: runtime.GOARCH, + desiredUpdate: &configv1.Update{ + // Migration in combination with an update to a new version is issued + Architecture: "Multi", + Version: data.to.version, + }, + }, + expected: expected{ + arch: "Multi", + // The `to` version is not present in the available updates as only migration is available + updates: []configv1.Release{{Version: data.from.version, Image: data.from.image}}, + queryParamArch: "multi", + }, + }, + { + name: "operator is not multi, image is not specified, version is specified, architecture is not specified", + args: args{ + operatorArchitecture: runtime.GOARCH, + desiredUpdate: &configv1.Update{ + Version: data.to.version, + }, + }, + expected: expected{ + arch: runtime.GOARCH, + conditionalUpdates: expectedConditionalUpdates, + queryParamArch: runtime.GOARCH, + }, + }, + // -------------------------------- Desired Update Is NOT set -------------------------------- + // ---------------- The operator is multi arch + { + name: "operator is multi, desired update is not specified", + args: args{ + operatorArchitecture: "Multi", + desiredUpdate: nil, + }, + expected: expected{ + arch: "Multi", + conditionalUpdates: expectedConditionalUpdates, + queryParamArch: "multi", + }, + }, + // ---------------- The operator is single arch + { + name: "operator is not multi, desired update is not specified", + args: args{ + operatorArchitecture: runtime.GOARCH, + desiredUpdate: nil, + }, + expected: expected{ + arch: runtime.GOARCH, + conditionalUpdates: expectedConditionalUpdates, + queryParamArch: runtime.GOARCH, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fixture := osusWithSingleConditionalEdge(data) + defer fixture.osus.Close() + + expectedAvailableUpdates, optr := newOperator( + fixture.osus.URL, + fixture.currentRelease, + fixture.mockCondition, + tt.args.operatorArchitecture, + ) + expectedAvailableUpdates.Architecture = tt.expected.arch + expectedAvailableUpdates.Updates = tt.expected.updates + expectedAvailableUpdates.ConditionalUpdates = tt.expected.conditionalUpdates + expectedAvailableUpdates.UpdateService = fixture.osus.URL + expectedAvailableUpdates.Channel = cvFixture.Spec.Channel + expectedAvailableUpdates.Condition = configv1.ClusterOperatorStatusCondition{ + Type: configv1.RetrievedUpdates, + Status: configv1.ConditionTrue, + } + + expectedQueryParams := url.Values{ + "arch": {tt.expected.queryParamArch}, + "id": {string(cvFixture.Spec.ClusterID)}, + "channel": {cvFixture.Spec.Channel}, + "version": {fixture.currentRelease.version}, + } + + cv := cvFixture.DeepCopy() + cv.Spec.DesiredUpdate = tt.args.desiredUpdate + if err := optr.syncAvailableUpdates(context.Background(), cv); err != nil { + t.Fatalf("syncAvailableUpdates() unexpected error: %v", err) + } + + if diff := cmp.Diff(&expectedQueryParams, fixture.lastQueryParams); diff != "" { + t.Fatalf("actual query parameters differ from expected:\n%s", diff) + } + + if diff := cmp.Diff(expectedAvailableUpdates, optr.availableUpdates, availableUpdatesCmpOpts...); diff != "" { + t.Fatalf("available updates differ from expected:\n%s", diff) + } + }) + } +} From f46f52cbdf5152df0387f392bb0e6fc88b71296f Mon Sep 17 00:00:00 2001 From: David Hurta Date: Mon, 24 Nov 2025 22:29:30 +0100 Subject: [PATCH 2/3] availableupdates: Unify capitalization when comparing architectures Otherwise, the following condition in cincinnati.go in MultiArch clusters: ``` if desiredArch == string(configv1.ClusterVersionArchitectureMulti) && currentArch != desiredArch { return current, []configv1.Release{current}, nil, nil } ``` gets evaluated to: ``` if "Multi" == string(configv1.ClusterVersionArchitectureMulti) && "multi" != "Multi" { return current, []configv1.Release{current}, nil, nil } ``` This will cause MultiArch clusters with a set non-empty `ClusterVersion.Spec.DesiredUpdate.Architecture` field to indefinitely have available updates set to `[]configv1.Release{current}` because the `"multi" != "Multi"` logic will always match. --- pkg/cvo/availableupdates.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cvo/availableupdates.go b/pkg/cvo/availableupdates.go index 7a79c773b7..652467671b 100644 --- a/pkg/cvo/availableupdates.go +++ b/pkg/cvo/availableupdates.go @@ -52,7 +52,7 @@ func (optr *Operator) syncAvailableUpdates(ctx context.Context, config *configv1 currentArch := runtime.GOARCH if optr.release.Architecture == configv1.ClusterVersionArchitectureMulti { - currentArch = "multi" + currentArch = string(configv1.ClusterVersionArchitectureMulti) } if desiredArch == "" { From 1b8e2834f571a8946639dbc868df8ffa5f0f401d Mon Sep 17 00:00:00 2001 From: David Hurta Date: Thu, 30 Oct 2025 17:43:19 +0100 Subject: [PATCH 3/3] availableupdates: Refactor getting desired/current architecture The commit does change the default behaviour of the `getDesiredArchitecture` method. However, the method is only used once in the `syncAvailableUpdates` method and nowhere else. The commit adds the subsequent logic for evaluating a desired architecture to the method itself and implements the `getCurrentArchitecture` method. The goal is to introduces "getters" for such values where the unified capitalization is enforced, and their return values are of the same nature (e.g., "Multi", "amd64", ...). --- pkg/cvo/availableupdates.go | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/pkg/cvo/availableupdates.go b/pkg/cvo/availableupdates.go index 652467671b..73ebe078da 100644 --- a/pkg/cvo/availableupdates.go +++ b/pkg/cvo/availableupdates.go @@ -49,15 +49,7 @@ func (optr *Operator) syncAvailableUpdates(ctx context.Context, config *configv1 channel := config.Spec.Channel desiredArch := optr.getDesiredArchitecture(config.Spec.DesiredUpdate) - currentArch := runtime.GOARCH - - if optr.release.Architecture == configv1.ClusterVersionArchitectureMulti { - currentArch = string(configv1.ClusterVersionArchitectureMulti) - } - - if desiredArch == "" { - desiredArch = currentArch - } + currentArch := optr.getCurrentArchitecture() // updates are only checked at most once per minimumUpdateCheckInterval or if the generation changes optrAvailableUpdates := optr.getAvailableUpdates() @@ -327,10 +319,17 @@ func (optr *Operator) getAvailableUpdates() *availableUpdates { } func (optr *Operator) getDesiredArchitecture(update *configv1.Update) string { - if update != nil { + if update != nil && len(update.Architecture) > 0 { return string(update.Architecture) } - return "" + return optr.getCurrentArchitecture() +} + +func (optr *Operator) getCurrentArchitecture() string { + if optr.release.Architecture == configv1.ClusterVersionArchitectureMulti { + return string(configv1.ClusterVersionArchitectureMulti) + } + return runtime.GOARCH } func calculateAvailableUpdatesStatus(ctx context.Context, clusterID string, transport *http.Transport, userAgent, updateService, desiredArch,