Merge pull request #5508 from djoshy/skip-unknown-arch

OCPBUGS-69674: Skip boot image updates if arch annotation is not found

Author: openshift-merge-bot[bot]

cmd/machine-config-controller/start.go

@@ -158,6 +158,7 @@ func runStartCmd(_ *cobra.Command, _ []string) {
 				ctrlctx.ConfigInformerFactory.Config().V1().Infrastructures(),
 				ctrlctx.ClientBuilder.OperatorClientOrDie(componentName),
 				ctrlctx.OperatorInformerFactory.Operator().V1().MachineConfigurations(),
+				ctrlctx.ConfigInformerFactory.Config().V1().ClusterVersions(),
 				ctrlctx.FeatureGatesHandler,
 			)
 			go bootImageController.Run(ctrlctx.Stop)

pkg/controller/bootimage/boot_image_controller.go

@@ -53,12 +53,14 @@ type Controller struct {
 	cpmsLister           machinelistersv1.ControlPlaneMachineSetLister
 	infraLister          configlistersv1.InfrastructureLister
 	mcopLister           mcoplistersv1.MachineConfigurationLister
+	clusterVersionLister configlistersv1.ClusterVersionLister
 
 	mcoCmListerSynced          cache.InformerSynced
 	mapiMachineSetListerSynced cache.InformerSynced
 	cpmsListerSynced           cache.InformerSynced
 	infraListerSynced          cache.InformerSynced
 	mcopListerSynced           cache.InformerSynced
+	clusterVersionListerSynced cache.InformerSynced
 
 	queue workqueue.TypedRateLimitingInterface[string]
 
@@ -122,6 +124,7 @@ func New(
 	infraInformer configinformersv1.InfrastructureInformer,
 	mcopClient mcopclientset.Interface,
 	mcopInformer mcopinformersv1.MachineConfigurationInformer,
+	clusterVersionInformer configinformersv1.ClusterVersionInformer,
 	fgHandler ctrlcommon.FeatureGatesHandler,
 ) *Controller {
 	eventBroadcaster := record.NewBroadcaster()
@@ -145,12 +148,14 @@ func New(
 	ctrl.cpmsLister = cpmsInformer.Lister()
 	ctrl.infraLister = infraInformer.Lister()
 	ctrl.mcopLister = mcopInformer.Lister()
+	ctrl.clusterVersionLister = clusterVersionInformer.Lister()
 
 	ctrl.mcoCmListerSynced = mcoCmInfomer.Informer().HasSynced
 	ctrl.mapiMachineSetListerSynced = mapiMachineSetInformer.Informer().HasSynced
 	ctrl.cpmsListerSynced = cpmsInformer.Informer().HasSynced
 	ctrl.infraListerSynced = infraInformer.Informer().HasSynced
 	ctrl.mcopListerSynced = mcopInformer.Informer().HasSynced
+	ctrl.clusterVersionListerSynced = clusterVersionInformer.Informer().HasSynced
 
 	mapiMachineSetInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
 		AddFunc:    ctrl.addMAPIMachineSet,
@@ -192,7 +197,7 @@ func (ctrl *Controller) Run(stopCh <-chan struct{}) {
 	defer utilruntime.HandleCrash()
 	defer ctrl.queue.ShutDown()
 
-	if !cache.WaitForCacheSync(stopCh, ctrl.mcoCmListerSynced, ctrl.mapiMachineSetListerSynced, ctrl.infraListerSynced, ctrl.mcopListerSynced) {
+	if !cache.WaitForCacheSync(stopCh, ctrl.mcoCmListerSynced, ctrl.mapiMachineSetListerSynced, ctrl.infraListerSynced, ctrl.mcopListerSynced, ctrl.clusterVersionListerSynced) {
 		return
 	}
 

pkg/controller/bootimage/boot_image_controller_test.go

@@ -97,71 +97,144 @@ func setMachineSetBootImage(machineset *machinev1beta1.MachineSet, generateBootI
 }
 
 func TestGetArchFromMachineSet(t *testing.T) {
+	// Helper to create a single-arch cluster version
+	singleArchCV := &osconfigv1.ClusterVersion{
+		Status: osconfigv1.ClusterVersionStatus{
+			Desired: osconfigv1.Release{
+				Architecture: "", // Empty means single-arch
+			},
+		},
+	}
+	// Helper to create a multi-arch cluster version
+	multiArchCV := &osconfigv1.ClusterVersion{
+		Status: osconfigv1.ClusterVersionStatus{
+			Desired: osconfigv1.Release{
+				Architecture: osconfigv1.ClusterVersionArchitectureMulti,
+			},
+		},
+	}
+
 	cases := []struct {
-		name         string
-		annotations  map[string]string
-		expectedArch string
-		expectError  bool
+		name           string
+		annotations    map[string]string
+		clusterVersion *osconfigv1.ClusterVersion
+		expectedArch   string
+		expectError    bool
 	}{
 		{
-			name: "Single architecture label",
+			name: "Single architecture label in single-arch cluster",
 			annotations: map[string]string{
 				MachineSetArchAnnotationKey: "kubernetes.io/arch=amd64",
 			},
-			expectedArch: "x86_64",
-			expectError:  false,
+			clusterVersion: singleArchCV,
+			expectedArch:   "x86_64",
+			expectError:    false,
+		},
+		{
+			name: "Single architecture label in multi-arch cluster",
+			annotations: map[string]string{
+				MachineSetArchAnnotationKey: "kubernetes.io/arch=amd64",
+			},
+			clusterVersion: multiArchCV,
+			expectedArch:   "x86_64",
+			expectError:    false,
 		},
 		{
 			name: "Multiple labels with architecture first",
 			annotations: map[string]string{
 				MachineSetArchAnnotationKey: "kubernetes.io/arch=amd64,topology.ebs.csi.aws.com/zone=eu-central-1a",
 			},
-			expectedArch: "x86_64",
-			expectError:  false,
+			clusterVersion: singleArchCV,
+			expectedArch:   "x86_64",
+			expectError:    false,
 		},
 		{
 			name: "Multiple labels with architecture last",
 			annotations: map[string]string{
 				MachineSetArchAnnotationKey: "topology.ebs.csi.aws.com/zone=eu-central-1a,kubernetes.io/arch=arm64",
 			},
-			expectedArch: "aarch64",
-			expectError:  false,
+			clusterVersion: singleArchCV,
+			expectedArch:   "aarch64",
+			expectError:    false,
 		},
 		{
 			name: "Multiple labels with architecture in middle",
 			annotations: map[string]string{
 				MachineSetArchAnnotationKey: "topology.ebs.csi.aws.com/zone=eu-central-1a,kubernetes.io/arch=s390x,node.kubernetes.io/instance-type=m5.large",
 			},
-			expectedArch: "s390x",
-			expectError:  false,
+			clusterVersion: singleArchCV,
+			expectedArch:   "s390x",
+			expectError:    false,
 		},
 		{
 			name: "Multiple labels with spaces",
 			annotations: map[string]string{
 				MachineSetArchAnnotationKey: " topology.ebs.csi.aws.com/zone=eu-central-1a , kubernetes.io/arch=ppc64le , node.kubernetes.io/instance-type=m5.large ",
 			},
-			expectedArch: "ppc64le",
-			expectError:  false,
+			clusterVersion: singleArchCV,
+			expectedArch:   "ppc64le",
+			expectError:    false,
 		},
 		{
 			name: "Invalid architecture",
 			annotations: map[string]string{
 				MachineSetArchAnnotationKey: "kubernetes.io/arch=invalid-arch",
 			},
-			expectError: true,
+			clusterVersion: singleArchCV,
+			expectError:    true,
 		},
 		{
-			name: "No architecture label",
+			name: "No architecture label in annotation",
 			annotations: map[string]string{
 				MachineSetArchAnnotationKey: "topology.ebs.csi.aws.com/zone=eu-central-1a,node.kubernetes.io/instance-type=m5.large",
 			},
+			clusterVersion: singleArchCV,
+			expectError:    true,
+		},
+		{
+			name:           "No annotation in single-arch cluster defaults to control plane arch",
+			annotations:    map[string]string{},
+			clusterVersion: singleArchCV,
+			expectError:    false, // Should default to control plane arch
+		},
+		{
+			name:        "No annotation in multi-arch cluster returns error",
+			annotations: map[string]string{},
+			clusterVersion: &osconfigv1.ClusterVersion{
+				Status: osconfigv1.ClusterVersionStatus{
+					Desired: osconfigv1.Release{
+						Architecture: osconfigv1.ClusterVersionArchitectureMulti,
+					},
+				},
+			},
 			expectError: true,
 		},
 		{
-			name:         "No annotation",
-			annotations:  map[string]string{},
-			expectedArch: "", // Will default to control plane arch, but we can't test that easily
-			expectError:  false,
+			name: "ARM64 architecture in multi-arch cluster",
+			annotations: map[string]string{
+				MachineSetArchAnnotationKey: "kubernetes.io/arch=arm64",
+			},
+			clusterVersion: multiArchCV,
+			expectedArch:   "aarch64",
+			expectError:    false,
+		},
+		{
+			name: "PPC64LE architecture in single-arch cluster",
+			annotations: map[string]string{
+				MachineSetArchAnnotationKey: "kubernetes.io/arch=ppc64le",
+			},
+			clusterVersion: singleArchCV,
+			expectedArch:   "ppc64le",
+			expectError:    false,
+		},
+		{
+			name: "S390X architecture in multi-arch cluster",
+			annotations: map[string]string{
+				MachineSetArchAnnotationKey: "kubernetes.io/arch=s390x",
+			},
+			clusterVersion: multiArchCV,
+			expectedArch:   "s390x",
+			expectError:    false,
 		},
 	}
 
@@ -174,7 +247,7 @@ func TestGetArchFromMachineSet(t *testing.T) {
 				},
 			}
 
-			arch, err := getArchFromMachineSet(machineSet)
+			arch, err := getArchFromMachineSet(machineSet, tc.clusterVersion)
 
 			if tc.expectError {
 				assert.Error(t, err, "Expected error for test case: %s", tc.name)

pkg/controller/bootimage/ms_helpers.go

@@ -8,6 +8,7 @@ import (
 	"strings"
 	"time"
 
+	osconfigv1 "github.com/openshift/api/config/v1"
 	machinev1beta1 "github.com/openshift/api/machine/v1beta1"
 	opv1 "github.com/openshift/api/operator/v1"
 	ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common"
@@ -113,9 +114,20 @@ func (ctrl *Controller) syncMAPIMachineSet(machineSet *machinev1beta1.MachineSet
 		}
 	}
 
+	// Fetch the ClusterVersion to determine if this is a multi-arch cluster
+	clusterVersion, err := ctrl.clusterVersionLister.Get("version")
+	if err != nil {
+		return fmt.Errorf("failed to fetch clusterversion during machineset sync: %v, defaulting to single-arch behavior", err)
+	}
+
 	// Fetch the architecture type of this machineset
-	arch, err := getArchFromMachineSet(machineSet)
+	arch, err := getArchFromMachineSet(machineSet, clusterVersion)
 	if err != nil {
+		// If no architecture annotation was found, skip this machineset without erroring
+		// A later sync loop will pick it up once the annotation is added
+		if strings.Contains(err.Error(), "no architecture annotation found") {
+			return nil
+		}
 		return fmt.Errorf("failed to fetch arch during machineset sync: %w", err)
 	}
 
@@ -222,29 +234,37 @@ func (ctrl *Controller) patchMachineSet(oldMachineSet, newMachineSet *machinev1b
 }
 
 // Returns architecture type for a given machineset
-func getArchFromMachineSet(machineset *machinev1beta1.MachineSet) (arch string, err error) {
+func getArchFromMachineSet(machineset *machinev1beta1.MachineSet, clusterVersion *osconfigv1.ClusterVersion) (arch string, err error) {
 
 	// Valid set of machineset/node architectures
 	validArchSet := sets.New("arm64", "s390x", "amd64", "ppc64le")
 	// Check if the annotation enclosing arch label is present on this machineset
 	archLabel, archLabelMatch := machineset.Annotations[MachineSetArchAnnotationKey]
-	if archLabelMatch {
-		// Parse the annotation value which may contain multiple comma-separated labels
-		// Example: kubernetes.io/arch=amd64,topology.ebs.csi.aws.com/zone=eu-central-1a
-		for label := range strings.SplitSeq(archLabel, ",") {
-			label = strings.TrimSpace(label)
-			if archLabelValue, found := strings.CutPrefix(label, ArchLabelKey); found {
-				// Extract just the architecture value after "kubernetes.io/arch="
-				if validArchSet.Has(archLabelValue) {
-					return archtranslater.RpmArch(archLabelValue), nil
-				}
-				return "", fmt.Errorf("invalid architecture value found in annotation: %s", archLabelValue)
+
+	if !archLabelMatch {
+		// Check if this is a multi-arch cluster
+		// clusterVersion should never be nil as it's validated by the caller
+		if clusterVersion.Status.Desired.Architecture == osconfigv1.ClusterVersionArchitectureMulti {
+			// For multi-arch clusters, we require the architecture annotation
+			klog.Errorf("No architecture annotation found on machineset %s in multi-arch cluster, skipping boot image update", machineset.Name)
+			return "", fmt.Errorf("no architecture annotation found on machineset %s", machineset.Name)
+		}
+		// For single-arch clusters, default to control plane architecture
+		klog.Infof("No architecture annotation found on machineset %s, defaulting to control plane architecture", machineset.Name)
+		return archtranslater.CurrentRpmArch(), nil
+	}
+
+	// Parse the annotation value which may contain multiple comma-separated labels
+	// Example: kubernetes.io/arch=amd64,topology.ebs.csi.aws.com/zone=eu-central-1a
+	for label := range strings.SplitSeq(archLabel, ",") {
+		label = strings.TrimSpace(label)
+		if archLabelValue, found := strings.CutPrefix(label, ArchLabelKey); found {
+			// Extract just the architecture value after "kubernetes.io/arch="
+			if validArchSet.Has(archLabelValue) {
+				return archtranslater.RpmArch(archLabelValue), nil
 			}
+			return "", fmt.Errorf("invalid architecture value found in annotation: %s", archLabelValue)
 		}
-		return "", fmt.Errorf("kubernetes.io/arch label not found in annotation: %s", archLabel)
 	}
-	// If no arch annotation was found on the machineset, default to the control plane arch.
-	// return the architecture of the node running this pod, which will always be a control plane node.
-	klog.Infof("Defaulting to control plane architecture")
-	return archtranslater.CurrentRpmArch(), nil
+	return "", fmt.Errorf("kubernetes.io/arch label not found in annotation: %s", archLabel)
 }