pkg: Propagate Release.Architecture

Pass the new property around in all clusters, but only propagate it
out to 'status' in the in-cluster ClusterVersion when the relevant
feature gate is enabled.

I'm dropping the "report an error condition when architecture isn't
set" test-case, because now that a non-Multi Operator.release is
always defaulted to runtime.GOARCH in syncAvailableUpdates, we no
longer have a way to trigger architecture-less Cincinnati requests.

Setting 'Architecture: runtime.GOARCH' in
TestOperator_availableUpdatesSync/if_last_successful_check_time_was_too_recent,_do_nothing
avoids triggering a new request because the CVO thinks the
architecture has changed.  Without that pivot:

  $ go test -run TestOperator_availableUpdatesSync/if_last_successful_check_time_was_too_recent,_do_nothing ./pkg/cvo/
  I1120 11:23:53.053380   24806 cvo.go:731] Started syncing available updates "test/default"
  I1120 11:23:53.057477   24806 availableupdates.go:79] Retrieving available updates again, because the architecture has changed from "" to "amd64"
  I1120 11:23:53.068889   24806 availableupdates.go:398] Update service http://127.0.0.1:46459 could not return available updates: ResponseFailed: unexpected HTTP status: 500 Internal Server Error
  I1120 11:23:53.070095   24806 cvo.go:733] Finished syncing available updates "test/default" (16.782722ms)
  --- FAIL: TestOperator_availableUpdatesSync (0.04s)
      --- FAIL: TestOperator_availableUpdatesSync/if_last_successful_check_time_was_too_recent,_do_nothing (0.04s)
          cvo_test.go:2784: unexpected:   (*cvo.availableUpdates)(
              -   nil,
              +   &{
              +           UpdateService: "http://localhost:8080/graph",
              +           Channel:       "fast",
              +           Architecture:  "amd64",
              +           Condition: v1.ClusterOperatorStatusCondition{
  ...

Author: wking

pkg/cincinnati/cincinnati.go

@@ -340,6 +340,14 @@ func convertRetrievedUpdateToRelease(update node) (configv1.Release, error) {
 		}
 		cvoUpdate.URL = configv1.URL(urlString)
 	}
+	if arch, ok := update.Metadata["release.openshift.io/architecture"]; ok {
+		switch arch {
+		case "multi":
+			cvoUpdate.Architecture = configv1.ClusterVersionArchitectureMulti
+		default:
+			klog.Warningf("Unrecognized release.openshift.io/architecture value %q", arch)
+		}
+	}
 	if channels, ok := update.Metadata["io.openshift.upgrades.graph.release.channels"]; ok {
 		cvoUpdate.Channels = strings.Split(channels, ",")
 		sort.Strings(cvoUpdate.Channels)

pkg/cvo/availableupdates.go

@@ -6,6 +6,7 @@ import (
 	"net/http"
 	"net/url"
 	"regexp"
+	"runtime"
 	"sort"
 	"strings"
 	"time"
@@ -48,7 +49,11 @@ func (optr *Operator) syncAvailableUpdates(ctx context.Context, config *configv1
 
 	channel := config.Spec.Channel
 	desiredArch := optr.getDesiredArchitecture(config.Spec.DesiredUpdate)
-	currentArch := optr.getArchitecture()
+	currentArch := runtime.GOARCH
+
+	if optr.release.Architecture == configv1.ClusterVersionArchitectureMulti {
+		currentArch = "multi"
+	}
 
 	if desiredArch == "" {
 		desiredArch = currentArch
@@ -205,24 +210,44 @@ func (u *availableUpdates) RecentlyChanged(interval time.Duration) bool {
 	return u.LastSyncOrConfigChange.After(time.Now().Add(-interval))
 }
 
-func (u *availableUpdates) NeedsUpdate(original *configv1.ClusterVersion) *configv1.ClusterVersion {
+func (u *availableUpdates) NeedsUpdate(original *configv1.ClusterVersion, statusReleaseArchitecture bool) *configv1.ClusterVersion {
 	if u == nil {
 		return nil
 	}
 	// Architecture could change but does not reside in ClusterVersion
 	if u.UpdateService != string(original.Spec.Upstream) || u.Channel != original.Spec.Channel {
 		return nil
 	}
-	if equality.Semantic.DeepEqual(u.Updates, original.Status.AvailableUpdates) &&
-		equality.Semantic.DeepEqual(u.ConditionalUpdates, original.Status.ConditionalUpdates) &&
+
+	var updates []configv1.Release
+	var conditionalUpdates []configv1.ConditionalUpdate
+
+	if statusReleaseArchitecture {
+		updates = u.Updates
+		conditionalUpdates = u.ConditionalUpdates
+	} else {
+		for _, update := range u.Updates {
+			c := update.DeepCopy()
+			c.Architecture = configv1.ClusterVersionArchitecture("")
+			updates = append(updates, *c)
+		}
+		for _, conditionalUpdate := range u.ConditionalUpdates {
+			c := conditionalUpdate.DeepCopy()
+			c.Release.Architecture = configv1.ClusterVersionArchitecture("")
+			conditionalUpdates = append(conditionalUpdates, *c)
+		}
+	}
+
+	if equality.Semantic.DeepEqual(updates, original.Status.AvailableUpdates) &&
+		equality.Semantic.DeepEqual(conditionalUpdates, original.Status.ConditionalUpdates) &&
 		equality.Semantic.DeepEqual(u.Condition, resourcemerge.FindOperatorStatusCondition(original.Status.Conditions, u.Condition.Type)) {
 		return nil
 	}
 
 	config := original.DeepCopy()
 	resourcemerge.SetOperatorStatusCondition(&config.Status.Conditions, u.Condition)
-	config.Status.AvailableUpdates = u.Updates
-	config.Status.ConditionalUpdates = u.ConditionalUpdates
+	config.Status.AvailableUpdates = updates
+	config.Status.ConditionalUpdates = conditionalUpdates
 	return config
 }
 
@@ -301,20 +326,6 @@ func (optr *Operator) getAvailableUpdates() *availableUpdates {
 	return u
 }
 
-// getArchitecture returns the currently determined cluster architecture.
-func (optr *Operator) getArchitecture() string {
-	optr.statusLock.Lock()
-	defer optr.statusLock.Unlock()
-	return optr.architecture
-}
-
-// SetArchitecture sets the cluster architecture.
-func (optr *Operator) SetArchitecture(architecture string) {
-	optr.statusLock.Lock()
-	defer optr.statusLock.Unlock()
-	optr.architecture = architecture
-}
-
 func (optr *Operator) getDesiredArchitecture(update *configv1.Update) string {
 	if update != nil {
 		return string(update.Architecture)

pkg/cvo/cvo.go

@@ -327,7 +327,6 @@ func (optr *Operator) InitializeFromPayload(update *payload.Update, requiredFeat
 	optr.enabledFeatureGates = cvoFlags
 	optr.release = update.Release
 	optr.releaseCreated = update.ImageRef.CreationTimestamp.Time
-	optr.SetArchitecture(update.Architecture)
 
 	// after the verifier has been loaded, initialize the sync worker with a payload retriever
 	// which will consume the verifier
@@ -676,7 +675,7 @@ func (optr *Operator) sync(ctx context.Context, key string) error {
 	config := validation.ClearInvalidFields(original, errs)
 
 	// identify the desired next version
-	desired, found := findUpdateFromConfig(config, optr.getArchitecture())
+	desired, found := findUpdateFromConfig(config, optr.release.Architecture)
 	initialized := optr.configSync.Initialized()
 	if found && initialized {
 		klog.V(2).Infof("Desired version from spec is %#v after initialization", desired)
@@ -721,8 +720,6 @@ func (optr *Operator) sync(ctx context.Context, key string) error {
 	// inform the config sync loop about our desired state
 	status := optr.configSync.Update(ctx, config.Generation, desired, config, state)
 
-	optr.SetArchitecture(status.Architecture)
-
 	// write cluster version status
 	return optr.syncStatus(ctx, original, config, status, errs)
 }
@@ -843,7 +840,7 @@ func (optr *Operator) currentVersion() configv1.Release {
 func mergeReleaseMetadata(release configv1.Release, getAvailableUpdates func() *availableUpdates) configv1.Release {
 	merged := *release.DeepCopy()
 
-	if merged.Version == "" || len(merged.URL) == 0 || merged.Channels == nil {
+	if merged.Version == "" || len(merged.URL) == 0 || len(merged.Architecture) == 0 || merged.Channels == nil {
 		// only fill in missing values from availableUpdates, to avoid clobbering data from payload.LoadUpdate.
 		availableUpdates := getAvailableUpdates()
 		if availableUpdates != nil {
@@ -865,6 +862,9 @@ func mergeReleaseMetadata(release configv1.Release, getAvailableUpdates func() *
 				if len(merged.URL) == 0 {
 					merged.URL = update.URL
 				}
+				if len(merged.Architecture) == 0 {
+					merged.Architecture = update.Architecture
+				}
 				if merged.Channels == nil {
 					merged.Channels = append(update.Channels[:0:0], update.Channels...) // copy
 				}

pkg/cvo/cvo_test.go

@@ -9,6 +9,7 @@ import (
 	"os"
 	"path/filepath"
 	"reflect"
+	"runtime"
 	"strconv"
 	"testing"
 	"time"
@@ -21,7 +22,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
-	"k8s.io/apimachinery/pkg/runtime"
+	apiruntime "k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/diff"
@@ -2365,47 +2366,6 @@ func TestOperator_availableUpdatesSync(t *testing.T) {
 				},
 			},
 		},
-		{
-			name: "report an error condition when architecture isn't set",
-			handler: func(w http.ResponseWriter, req *http.Request) {
-				http.Error(w, "bad things", http.StatusInternalServerError)
-			},
-			optr: &Operator{
-				updateService: "http://localhost:8080/graph",
-				release: configv1.Release{
-					Version: "v4.0.0",
-					Image:   "image/image:v4.0.1",
-				},
-				namespace: "test",
-				name:      "default",
-				client: fake.NewSimpleClientset(
-					&configv1.ClusterVersion{
-						ObjectMeta: metav1.ObjectMeta{
-							Name: "default",
-						},
-						Spec: configv1.ClusterVersionSpec{
-							ClusterID: configv1.ClusterID(id),
-							Channel:   "",
-						},
-						Status: configv1.ClusterVersionStatus{
-							History: []configv1.UpdateHistory{
-								{Image: "image/image:v4.0.1"},
-							},
-						},
-					},
-				),
-			},
-			wantUpdates: &availableUpdates{
-				UpdateService: "http://localhost:8080/graph",
-				Channel:       "",
-				Condition: configv1.ClusterOperatorStatusCondition{
-					Type:    configv1.RetrievedUpdates,
-					Status:  configv1.ConditionFalse,
-					Reason:  noArchitecture,
-					Message: "Architecture has not been configured.",
-				},
-			},
-		},
 		{
 			name: "report an error condition when channel isn't set",
 			handler: func(w http.ResponseWriter, req *http.Request) {
@@ -2685,6 +2645,7 @@ func TestOperator_availableUpdatesSync(t *testing.T) {
 				availableUpdates: &availableUpdates{
 					UpdateService:          "http://localhost:8080/graph",
 					Channel:                "fast",
+					Architecture:           runtime.GOARCH,
 					LastAttempt:            time.Now(),
 					LastSyncOrConfigChange: time.Now(),
 				},
@@ -4156,7 +4117,7 @@ func expectMutation(t *testing.T, a ktesting.Action, verb string, resource, subr
 	}
 	switch at := a.(type) {
 	case ktesting.CreateAction:
-		expect, actual := obj.(runtime.Object).DeepCopyObject(), at.GetObject()
+		expect, actual := obj.(apiruntime.Object).DeepCopyObject(), at.GetObject()
 		// default autogenerated cluster ID
 		if in, ok := expect.(*configv1.ClusterVersion); ok {
 			if in.Spec.ClusterID == "" {
@@ -4199,7 +4160,7 @@ func expectMutation(t *testing.T, a ktesting.Action, verb string, resource, subr
 
 func fakeClientsetWithUpdates(obj *configv1.ClusterVersion) *fake.Clientset {
 	client := &fake.Clientset{}
-	client.AddReactor("*", "*", func(action ktesting.Action) (handled bool, ret runtime.Object, err error) {
+	client.AddReactor("*", "*", func(action ktesting.Action) (handled bool, ret apiruntime.Object, err error) {
 		if action.GetVerb() == "get" {
 			return true, obj.DeepCopy(), nil
 		}

pkg/cvo/status.go

@@ -186,7 +186,7 @@ func (optr *Operator) syncStatus(ctx context.Context, original, config *configv1
 
 	cvUpdated := false
 	// update the config with the latest available updates
-	if updated := optr.getAvailableUpdates().NeedsUpdate(config); updated != nil {
+	if updated := optr.getAvailableUpdates().NeedsUpdate(config, optr.enabledFeatureGates.StatusReleaseArchitecture()); updated != nil {
 		cvUpdated = true
 		config = updated
 	}
@@ -229,11 +229,17 @@ func updateClusterVersionStatus(cvStatus *configv1.ClusterVersionStatus, status
 		if len(status.Actual.URL) == 0 {
 			status.Actual.URL = release.URL
 		}
+		if len(status.Actual.Architecture) == 0 {
+			status.Actual.Architecture = release.Architecture
+		}
 		if status.Actual.Channels == nil {
 			status.Actual.Channels = append(release.Channels[:0:0], release.Channels...) // copy
 		}
 	}
 	desired := mergeReleaseMetadata(status.Actual, getAvailableUpdates)
+	if !enabledGates.StatusReleaseArchitecture() {
+		desired.Architecture = configv1.ClusterVersionArchitecture("")
+	}
 
 	risksMsg := ""
 	if desired.Image == status.loadPayloadStatus.Update.Image {

pkg/cvo/status_test.go

@@ -201,6 +201,7 @@ func TestOperator_syncFailingStatus(t *testing.T) {
 type fakeRiFlags struct {
 	unknownVersion                bool
 	reconciliationIssuesCondition bool
+	statusReleaseArchitecture     bool
 }
 
 func (f fakeRiFlags) UnknownVersion() bool {
@@ -211,6 +212,10 @@ func (f fakeRiFlags) ReconciliationIssuesCondition() bool {
 	return f.reconciliationIssuesCondition
 }
 
+func (f fakeRiFlags) StatusReleaseArchitecture() bool {
+	return f.statusReleaseArchitecture
+}
+
 func TestUpdateClusterVersionStatus_UnknownVersionAndReconciliationIssues(t *testing.T) {
 	ignoreLastTransitionTime := cmpopts.IgnoreFields(configv1.ClusterOperatorStatusCondition{}, "LastTransitionTime")
 

pkg/cvo/updatepayload.go

@@ -343,7 +343,7 @@ func (r *payloadRetriever) pruneJobs(ctx context.Context, retain int) error {
 // the desired architecture is changed to multi it resolves the payload using the current
 // version since that's the only valid available update. Otherwise it attempts to resolve
 // the payload using the specified desired version.
-func findUpdateFromConfig(config *configv1.ClusterVersion, currentArch string) (configv1.Update, bool) {
+func findUpdateFromConfig(config *configv1.ClusterVersion, currentArch configv1.ClusterVersionArchitecture) (configv1.Update, bool) {
 	update := config.Spec.DesiredUpdate
 	if update == nil {
 		return configv1.Update{}, false
@@ -353,7 +353,7 @@ func findUpdateFromConfig(config *configv1.ClusterVersion, currentArch string) (
 
 		// Architecture changed to multi so only valid update is the multi arch version of current version
 		if update.Architecture == configv1.ClusterVersionArchitectureMulti &&
-			currentArch != string(configv1.ClusterVersionArchitectureMulti) {
+			currentArch != configv1.ClusterVersionArchitectureMulti {
 			version = config.Status.Desired.Version
 		}
 		return findUpdateFromConfigVersion(config, version, update.Force)

pkg/featuregates/featuregates.go

@@ -25,6 +25,10 @@ type CvoGateChecker interface {
 	// should never be relied upon by any production code. We may want to eventually turn this into
 	// some kind of "real" API.
 	ReconciliationIssuesCondition() bool
+
+	// StatusReleaseArchitecture controls whether CVO populates
+	// Release.Architecture in status properties like status.desired and status.history[].
+	StatusReleaseArchitecture() bool
 }
 
 type panicOnUsageBeforeInitializationFunc func()
@@ -42,6 +46,11 @@ func (p panicOnUsageBeforeInitializationFunc) ReconciliationIssuesCondition() bo
 	return false
 }
 
+func (p panicOnUsageBeforeInitializationFunc) StatusReleaseArchitecture() bool {
+	p()
+	return false
+}
+
 func (p panicOnUsageBeforeInitializationFunc) UnknownVersion() bool {
 	p()
 	return false
@@ -58,12 +67,17 @@ type CvoGates struct {
 	// individual flags mirror the CvoGateChecker interface
 	unknownVersion                bool
 	reconciliationIssuesCondition bool
+	statusReleaseArchitecture     bool
 }
 
 func (c CvoGates) ReconciliationIssuesCondition() bool {
 	return c.reconciliationIssuesCondition
 }
 
+func (c CvoGates) StatusReleaseArchitecture() bool {
+	return c.statusReleaseArchitecture
+}
+
 func (c CvoGates) UnknownVersion() bool {
 	return c.unknownVersion
 }
@@ -74,6 +88,7 @@ func DefaultCvoGates(version string) CvoGates {
 		desiredVersion:                version,
 		unknownVersion:                true,
 		reconciliationIssuesCondition: false,
+		statusReleaseArchitecture:     false,
 	}
 }
 
@@ -90,13 +105,19 @@ func CvoGatesFromFeatureGate(gate *configv1.FeatureGate, version string) CvoGate
 		// We found the matching version, so we do not need to run in the unknown version mode
 		enabledGates.unknownVersion = false
 		for _, enabled := range g.Enabled {
-			if enabled.Name == features.FeatureGateUpgradeStatus {
+			switch enabled.Name {
+			case features.FeatureGateUpgradeStatus:
 				enabledGates.reconciliationIssuesCondition = true
+			case features.FeatureGateImageStreamImportMode:
+				enabledGates.statusReleaseArchitecture = false
 			}
 		}
 		for _, disabled := range g.Disabled {
-			if disabled.Name == features.FeatureGateUpgradeStatus {
+			switch disabled.Name {
+			case features.FeatureGateUpgradeStatus:
 				enabledGates.reconciliationIssuesCondition = false
+			case features.FeatureGateImageStreamImportMode:
+				enabledGates.statusReleaseArchitecture = false
 			}
 		}
 	}

pkg/payload/payload.go

@@ -386,7 +386,8 @@ func loadReleaseFromMetadata(releaseDir string) (configv1.Release, string, error
 	if archInterface, ok := metadata.Metadata["release.openshift.io/architecture"]; ok {
 		if archString, ok := archInterface.(string); ok {
 			if archString == releaseMultiArchID {
-				arch = string(configv1.ClusterVersionArchitectureMulti)
+				release.Architecture = configv1.ClusterVersionArchitectureMulti
+				arch = string(release.Architecture)
 			} else {
 				return release, "", fmt.Errorf("Architecture from %s (%s) contains invalid value: %q. Valid value is %q.",
 					cincinnatiJSONFile, release.Version, archString, releaseMultiArchID)