Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 1 commit into from
Dec 24, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cmd/machine-config-controller/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
7 changes: 6 additions & 1 deletion pkg/controller/bootimage/boot_image_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand Down Expand Up @@ -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()
Expand All @@ -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,
Expand Down Expand Up @@ -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
}

Expand Down
117 changes: 95 additions & 22 deletions pkg/controller/bootimage/boot_image_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
}

Expand All @@ -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)
Expand Down
56 changes: 38 additions & 18 deletions pkg/controller/bootimage/ms_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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)
}