diff --git a/cmd/machine-config-controller/start.go b/cmd/machine-config-controller/start.go index 4bc966992e..d62b54a767 100644 --- a/cmd/machine-config-controller/start.go +++ b/cmd/machine-config-controller/start.go @@ -153,6 +153,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) diff --git a/pkg/controller/bootimage/boot_image_controller.go b/pkg/controller/bootimage/boot_image_controller.go index ea9655bfeb..1a68e0c9f9 100644 --- a/pkg/controller/bootimage/boot_image_controller.go +++ b/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 } diff --git a/pkg/controller/bootimage/boot_image_controller_test.go b/pkg/controller/bootimage/boot_image_controller_test.go index dd5609ced2..2bf92a8956 100644 --- a/pkg/controller/bootimage/boot_image_controller_test.go +++ b/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) diff --git a/pkg/controller/bootimage/ms_helpers.go b/pkg/controller/bootimage/ms_helpers.go index 994040e72e..bbb00a3108 100644 --- a/pkg/controller/bootimage/ms_helpers.go +++ b/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) }