Merge pull request #5428 from djoshy/implement-skew-enforcement

MCO-1877: MCO-1879: MCO-1882: MCO-1884: Implement boot image skew enforcement MVP

Author: openshift-merge-bot[bot]

pkg/apihelpers/apihelpers.go

@@ -554,3 +554,31 @@ func MergeMachineManager(status *opv1.MachineConfigurationStatus, manager opv1.M
 
 	status.ManagedBootImagesStatus = result
 }
+
+// GetSkewEnforcementStatusAutomaticWithOCPVersion returns a BootImageSkewEnforcementStatus with Automatic mode and the given OCP version.
+func GetSkewEnforcementStatusAutomaticWithOCPVersion(ocpVersion string) opv1.BootImageSkewEnforcementStatus {
+	return opv1.BootImageSkewEnforcementStatus{
+		Mode: opv1.BootImageSkewEnforcementModeStatusAutomatic,
+		Automatic: opv1.ClusterBootImageAutomatic{
+			OCPVersion: ocpVersion,
+		},
+	}
+}
+
+// GetSkewEnforcementStatusManualWithOCPVersion returns a BootImageSkewEnforcementStatus with Manual mode and the given OCP version.
+func GetSkewEnforcementStatusManualWithOCPVersion(ocpVersion string) opv1.BootImageSkewEnforcementStatus {
+	return opv1.BootImageSkewEnforcementStatus{
+		Mode: opv1.BootImageSkewEnforcementModeStatusManual,
+		Manual: opv1.ClusterBootImageManual{
+			Mode:       opv1.ClusterBootImageSpecModeOCPVersion,
+			OCPVersion: ocpVersion,
+		},
+	}
+}
+
+// GetSkewEnforcementStatusNone returns a BootImageSkewEnforcementStatus with None mode.
+func GetSkewEnforcementStatusNone() opv1.BootImageSkewEnforcementStatus {
+	return opv1.BootImageSkewEnforcementStatus{
+		Mode: opv1.BootImageSkewEnforcementModeStatusNone,
+	}
+}

pkg/controller/bootimage/boot_image_controller.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"reflect"
+	"strings"
 	"time"
 
 	features "github.com/openshift/api/features"
@@ -14,6 +15,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
+	k8sversion "k8s.io/apimachinery/pkg/util/version"
 	"k8s.io/apimachinery/pkg/util/wait"
 	coreinformersv1 "k8s.io/client-go/informers/core/v1"
 	clientset "k8s.io/client-go/kubernetes"
@@ -77,6 +79,7 @@ type Controller struct {
 // Stats structure for local bookkeeping of machine resources
 type MachineResourceStats struct {
 	inProgress   int
+	skippedCount int
 	erroredCount int
 	totalCount   int
 }
@@ -94,6 +97,17 @@ func (mrs MachineResourceStats) isFinished() bool {
 	return mrs.totalCount == (mrs.inProgress + mrs.erroredCount)
 }
 
+func (mrs MachineResourceStats) getProgressingStatusMessage(name string) string {
+	if mrs.skippedCount > 0 {
+		return fmt.Sprintf("Reconciled %d of %d %s (%d skipped)", mrs.inProgress-mrs.skippedCount, mrs.totalCount, name, mrs.skippedCount)
+	}
+	return fmt.Sprintf("Reconciled %d of %d %s", mrs.inProgress, mrs.totalCount, name)
+}
+
+func (mrs MachineResourceStats) getDegradedStatusMessage(name string) string {
+	return fmt.Sprintf("%d Degraded %s", mrs.erroredCount, name)
+}
+
 const (
 	// Name of machine api namespace
 	MachineAPINamespace = "openshift-machine-api"
@@ -446,8 +460,11 @@ func (ctrl *Controller) updateMachineConfiguration(oldMC, newMC interface{}) {
 		return
 	}
 
-	// Only take action if the there is an actual change in the MachineConfiguration's ManagedBootImagesStatus
-	if reflect.DeepEqual(oldMachineConfiguration.Status.ManagedBootImagesStatus, newMachineConfiguration.Status.ManagedBootImagesStatus) {
+	// Skip reconciliation if neither ManagedBootImagesStatus nor BootImageSkewEnforcementStatus has changed.
+	// BootImageSkewEnforcementStatus is only checked when the BootImageSkewEnforcement feature gate is enabled.
+	if reflect.DeepEqual(oldMachineConfiguration.Status.ManagedBootImagesStatus, newMachineConfiguration.Status.ManagedBootImagesStatus) &&
+		(!ctrl.fgHandler.Enabled(features.FeatureGateBootImageSkewEnforcement) ||
+			reflect.DeepEqual(oldMachineConfiguration.Status.BootImageSkewEnforcementStatus, newMachineConfiguration.Status.BootImageSkewEnforcementStatus)) {
 		return
 	}
 
@@ -493,7 +510,13 @@ func (ctrl *Controller) updateConditions(newReason string, syncError error, targ
 	for i, condition := range newConditions {
 		if condition.Type == targetConditionType {
 			if condition.Type == opv1.MachineConfigurationBootImageUpdateProgressing {
-				newConditions[i].Message = fmt.Sprintf("Reconciled %d of %d MAPI MachineSets | Reconciled %d of %d ControlPlaneMachineSets | Reconciled %d of %d CAPI MachineSets | Reconciled %d of %d CAPI MachineDeployments", ctrl.mapiStats.inProgress, ctrl.mapiStats.totalCount, ctrl.cpmsStats.inProgress, ctrl.cpmsStats.totalCount, ctrl.capiMachineSetStats.inProgress, ctrl.capiMachineSetStats.totalCount, ctrl.capiMachineDeploymentStats.inProgress, ctrl.capiMachineDeploymentStats.totalCount)
+				messages := []string{
+					ctrl.mapiStats.getProgressingStatusMessage("MAPI MachineSets"),
+					ctrl.cpmsStats.getProgressingStatusMessage("ControlPlaneMachineSets"),
+					ctrl.capiMachineSetStats.getProgressingStatusMessage("CAPI MachineSets"),
+					ctrl.capiMachineDeploymentStats.getProgressingStatusMessage("CAPI MachineDeployments"),
+				}
+				newConditions[i].Message = strings.Join(messages, " | ")
 				newConditions[i].Reason = newReason
 				// If all machine resources have been processed, then the controller is no longer progressing.
 				if ctrl.mapiStats.isFinished() && ctrl.cpmsStats.isFinished() && ctrl.capiMachineSetStats.isFinished() && ctrl.capiMachineDeploymentStats.isFinished() {
@@ -502,10 +525,16 @@ func (ctrl *Controller) updateConditions(newReason string, syncError error, targ
 					newConditions[i].Status = metav1.ConditionTrue
 				}
 			} else if condition.Type == opv1.MachineConfigurationBootImageUpdateDegraded {
+				messages := []string{
+					ctrl.mapiStats.getDegradedStatusMessage("MAPI MachineSets"),
+					ctrl.cpmsStats.getDegradedStatusMessage("ControlPlaneMachineSets"),
+					ctrl.capiMachineSetStats.getDegradedStatusMessage("CAPI MachineSets"),
+					ctrl.capiMachineDeploymentStats.getDegradedStatusMessage("CAPI MachineDeployments"),
+				}
 				if syncError == nil {
-					newConditions[i].Message = fmt.Sprintf("%d Degraded MAPI MachineSets | %d Degraded ControlPlaneMachineSets | %d Degraded CAPI MachineSets | %d CAPI MachineDeployments", ctrl.mapiStats.erroredCount, ctrl.cpmsStats.erroredCount, ctrl.capiMachineSetStats.erroredCount, ctrl.capiMachineDeploymentStats.erroredCount)
+					newConditions[i].Message = strings.Join(messages, " | ")
 				} else {
-					newConditions[i].Message = fmt.Sprintf("%d Degraded MAPI MachineSets | %d Degraded ControlPlaneMachineSets | %d Degraded CAPI MachineSets | %d CAPI MachineDeployments | Error(s): %s", ctrl.mapiStats.erroredCount, ctrl.cpmsStats.erroredCount, ctrl.capiMachineSetStats.erroredCount, ctrl.capiMachineDeploymentStats.erroredCount, syncError.Error())
+					newConditions[i].Message = fmt.Sprintf("%s | Error(s): %s", strings.Join(messages, " | "), syncError.Error())
 				}
 				newConditions[i].Reason = newReason
 				if syncError != nil {
@@ -523,33 +552,77 @@ func (ctrl *Controller) updateConditions(newReason string, syncError error, targ
 	}
 	// Only make an API call if there is an update to the Conditions field
 	if !reflect.DeepEqual(newConditions, mcop.Status.Conditions) {
-		ctrl.updateMachineConfigurationStatus(mcop, newConditions)
+		mcop.Status.Conditions = newConditions
+		ctrl.updateMachineConfigurationStatus(mcop.Status)
 	}
 }
 
-// updateMachineConfigurationStatus updates the MachineConfiguration status with new conditions
-// using retry logic to handle concurrent updates.
-func (ctrl *Controller) updateMachineConfigurationStatus(mcop *opv1.MachineConfiguration, newConditions []metav1.Condition) {
+// updateClusterBootImage updates the cluster boot image record if the skew enforcement is set to Automatic mode.
+func (ctrl *Controller) updateClusterBootImage() {
+
+	mcop, err := ctrl.mcopClient.OperatorV1().MachineConfigurations().Get(context.TODO(), ctrlcommon.MCOOperatorKnobsObjectName, metav1.GetOptions{})
+	if err != nil {
+		klog.Errorf("error updating cluster boot image record: %s", err)
+		return
+	}
+	// No action to take if not in automatic mode
+	if mcop.Status.BootImageSkewEnforcementStatus.Mode != opv1.BootImageSkewEnforcementModeStatusAutomatic {
+		return
+	}
+
+	// Get OCP version of last boot image update from configmap
+	configMap, err := ctrl.mcoCmLister.ConfigMaps(ctrlcommon.MCONamespace).Get(ctrlcommon.BootImagesConfigMapName)
+	if err != nil {
+		klog.Warningf("Failed to get boot images configmap: %v, skipping cluster boot image record update", err)
+		return
+	}
+
+	releaseVersion, found := configMap.Data[ctrlcommon.OCPReleaseVersionKey]
+	if !found {
+		klog.Warningf("OCP release version not found in boot images configmap, skipping cluster boot image record update")
+		return
+	}
 
+	// Parse and extract semantic version (major.minor.patch) for API validation
+	parsedVersion, err := k8sversion.ParseGeneric(releaseVersion)
+	if err != nil {
+		klog.Warningf("Failed to parse release version %q from configmap: %v, skipping cluster boot image record update", releaseVersion, err)
+		return
+	}
+	ocpVersion := fmt.Sprintf("%d.%d.%d", parsedVersion.Major(), parsedVersion.Minor(), parsedVersion.Patch())
+
+	newBootImageSkewEnforcementStatus := mcop.Status.BootImageSkewEnforcementStatus.DeepCopy()
+	newBootImageSkewEnforcementStatus.Automatic = opv1.ClusterBootImageAutomatic{
+		OCPVersion: ocpVersion,
+	}
+
+	// Only make an API call if there is an update to the skew enforcement status
+	if !reflect.DeepEqual(mcop.Status.BootImageSkewEnforcementStatus, newBootImageSkewEnforcementStatus) {
+		mcop.Status.BootImageSkewEnforcementStatus = *newBootImageSkewEnforcementStatus
+		ctrl.updateMachineConfigurationStatus(mcop.Status)
+	}
+}
+
+// updateMachineConfigurationStatus updates the MachineConfiguration status using retry logic to handle concurrent updates.
+func (ctrl *Controller) updateMachineConfigurationStatus(mcopStatus opv1.MachineConfigurationStatus) {
 	// Using a retry here as there may be concurrent reconiliation loops updating conditions for multiple
 	// resources at the same time and their local stores may be out of date
-	if !reflect.DeepEqual(mcop.Status.Conditions, newConditions) {
-		klog.V(4).Infof("%v", newConditions)
-		if err := retry.RetryOnConflict(retry.DefaultBackoff, func() error {
-			mcop, err := ctrl.mcopClient.OperatorV1().MachineConfigurations().Get(context.TODO(), ctrlcommon.MCOOperatorKnobsObjectName, metav1.GetOptions{})
-			if err != nil {
-				return err
-			}
-			mcop.Status.Conditions = newConditions
-			_, err = ctrl.mcopClient.OperatorV1().MachineConfigurations().UpdateStatus(context.TODO(), mcop, metav1.UpdateOptions{})
-			if err != nil {
-				return err
-			}
-			return nil
-		}); err != nil {
-			klog.Errorf("error updating MachineConfiguration status: %v", err)
+	klog.V(4).Infof("MachineConfiguration status update: %v", mcopStatus)
+	if err := retry.RetryOnConflict(retry.DefaultBackoff, func() error {
+		mcop, err := ctrl.mcopClient.OperatorV1().MachineConfigurations().Get(context.TODO(), ctrlcommon.MCOOperatorKnobsObjectName, metav1.GetOptions{})
+		if err != nil {
+			return err
 		}
+		mcop.Status = mcopStatus
+		_, err = ctrl.mcopClient.OperatorV1().MachineConfigurations().UpdateStatus(context.TODO(), mcop, metav1.UpdateOptions{})
+		if err != nil {
+			return err
+		}
+		return nil
+	}); err != nil {
+		klog.Errorf("error updating MachineConfiguration status: %v", err)
 	}
+
 }
 
 // getDefaultConditions returns the default boot image update conditions when no

pkg/controller/bootimage/boot_image_controller_test.go

@@ -813,7 +813,7 @@ func TestReconcileAzureProviderSpec(t *testing.T) {
 				testStreamData = tt.streamData
 			}
 
-			patchRequired, updatedProviderSpec, err := reconcileAzureProviderSpec(
+			patchRequired, _, updatedProviderSpec, err := reconcileAzureProviderSpec(
 				testStreamData,
 				tt.arch,
 				infra,

pkg/controller/bootimage/cpms_helpers.go

@@ -257,7 +257,7 @@ func reconcilePlatformCPMS[T any](
 	configMap *corev1.ConfigMap,
 	arch string,
 	secretClient clientset.Interface,
-	reconcileProviderSpec func(*stream.Stream, string, *osconfigv1.Infrastructure, *T, string, clientset.Interface) (bool, *T, error),
+	reconcileProviderSpec func(*stream.Stream, string, *osconfigv1.Infrastructure, *T, string, clientset.Interface) (bool, bool, *T, error),
 ) (patchRequired bool, newCPMS *machinev1.ControlPlaneMachineSet, err error) {
 	klog.Infof("Reconciling controlplanemachineset %s on %s, with arch %s", cpms.Name, string(infra.Status.PlatformStatus.Type), arch)
 
@@ -274,7 +274,7 @@ func reconcilePlatformCPMS[T any](
 	}
 
 	// Reconcile the provider spec
-	patchRequired, newProviderSpec, err := reconcileProviderSpec(streamData, arch, infra, providerSpec, cpms.Name, secretClient)
+	patchRequired, _, newProviderSpec, err := reconcileProviderSpec(streamData, arch, infra, providerSpec, cpms.Name, secretClient)
 	if err != nil {
 		return false, nil, err
 	}

pkg/controller/bootimage/ms_helpers.go

@@ -9,6 +9,7 @@ import (
 	"time"
 
 	osconfigv1 "github.com/openshift/api/config/v1"
+	"github.com/openshift/api/features"
 	machinev1beta1 "github.com/openshift/api/machine/v1beta1"
 	opv1 "github.com/openshift/api/operator/v1"
 	ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common"
@@ -69,30 +70,40 @@ func (ctrl *Controller) syncMAPIMachineSets(reason string) {
 	// Reset stats before initiating reconciliation loop
 	ctrl.mapiStats.inProgress = 0
 	ctrl.mapiStats.totalCount = len(mapiMachineSets)
+	ctrl.mapiStats.skippedCount = 0
 	ctrl.mapiStats.erroredCount = 0
 
 	// Signal start of reconciliation process, by setting progressing to true
 	var syncErrors []error
 	ctrl.updateConditions(reason, nil, opv1.MachineConfigurationBootImageUpdateProgressing)
 
 	for _, machineSet := range mapiMachineSets {
-		err := ctrl.syncMAPIMachineSet(machineSet)
+		patchSkipped, err := ctrl.syncMAPIMachineSet(machineSet)
 		if err == nil {
 			ctrl.mapiStats.inProgress++
 		} else {
 			klog.Errorf("Error syncing MAPI MachineSet %v", err)
 			syncErrors = append(syncErrors, fmt.Errorf("error syncing MAPI MachineSet %s: %v", machineSet.Name, err))
 			ctrl.mapiStats.erroredCount++
 		}
+		if patchSkipped {
+			ctrl.mapiStats.skippedCount++
+		}
 		// Update progressing conditions every step of the loop
 		ctrl.updateConditions(reason, nil, opv1.MachineConfigurationBootImageUpdateProgressing)
 	}
 	// Update/Clear degrade conditions based on errors from this loop
 	ctrl.updateConditions(reason, kubeErrs.NewAggregate(syncErrors), opv1.MachineConfigurationBootImageUpdateDegraded)
+	// If no machinesets were skipped, update the cluster boot image record
+	if ctrl.fgHandler.Enabled(features.FeatureGateBootImageSkewEnforcement) {
+		if ctrl.mapiStats.skippedCount == 0 {
+			ctrl.updateClusterBootImage()
+		}
+	}
 }
 
 // syncMAPIMachineSet will attempt to reconcile the provided machineset
-func (ctrl *Controller) syncMAPIMachineSet(machineSet *machinev1beta1.MachineSet) error {
+func (ctrl *Controller) syncMAPIMachineSet(machineSet *machinev1beta1.MachineSet) (bool, error) {
 
 	startTime := time.Now()
 	klog.V(4).Infof("Started syncing MAPI machineset %q (%v)", machineSet.Name, startTime)
@@ -104,20 +115,20 @@ func (ctrl *Controller) syncMAPIMachineSet(machineSet *machinev1beta1.MachineSet
 	// that the machineset may be managed by another workflow and should not be reconciled.
 	if len(machineSet.GetOwnerReferences()) != 0 {
 		klog.Infof("machineset %s has OwnerReference: %v, skipping boot image update", machineSet.GetOwnerReferences()[0].Kind+"/"+machineSet.GetOwnerReferences()[0].Name, machineSet.Name)
-		return nil
+		return true, nil
 	}
 
 	if os, ok := machineSet.Spec.Template.Labels[OSLabelKey]; ok {
 		if os == "Windows" {
 			klog.Infof("machineset %s has a windows os label, skipping boot image update", machineSet.Name)
-			return nil
+			return false, nil
 		}
 	}
 
 	// 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)
+		return false, fmt.Errorf("failed to fetch clusterversion during machineset sync: %v, defaulting to single-arch behavior", err)
 	}
 
 	// Fetch the architecture type of this machineset
@@ -126,15 +137,15 @@ func (ctrl *Controller) syncMAPIMachineSet(machineSet *machinev1beta1.MachineSet
 		// 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 true, nil
 		}
-		return fmt.Errorf("failed to fetch arch during machineset sync: %w", err)
+		return false, fmt.Errorf("failed to fetch arch during machineset sync: %w", err)
 	}
 
 	// Fetch the infra object to determine the platform type
 	infra, err := ctrl.infraLister.Get("cluster")
 	if err != nil {
-		return fmt.Errorf("failed to fetch infra object during machineset sync: %w", err)
+		return false, fmt.Errorf("failed to fetch infra object during machineset sync: %w", err)
 	}
 
 	// Fetch the bootimage configmap & ensure it has been stamped by the operator. This is done by
@@ -143,44 +154,44 @@ func (ctrl *Controller) syncMAPIMachineSet(machineSet *machinev1beta1.MachineSet
 	// If it hasn't been updated, exit and wait for a resync.
 	configMap, err := ctrl.mcoCmLister.ConfigMaps(ctrlcommon.MCONamespace).Get(ctrlcommon.BootImagesConfigMapName)
 	if err != nil {
-		return fmt.Errorf("failed to fetch coreos-bootimages config map during machineset sync: %w", err)
+		return false, fmt.Errorf("failed to fetch coreos-bootimages config map during machineset sync: %w", err)
 	}
 	versionHashFromCM, versionHashFound := configMap.Data[ctrlcommon.MCOVersionHashKey]
 	if !versionHashFound {
 		klog.Infof("failed to find mco version hash in %s configmap, sync will exit to wait for the MCO upgrade to complete", ctrlcommon.BootImagesConfigMapName)
-		return nil
+		return true, nil
 	}
 	if versionHashFromCM != operatorversion.Hash {
 		klog.Infof("mismatch between MCO hash version stored in configmap and current MCO version; sync will exit to wait for the MCO upgrade to complete")
-		return nil
+		return true, nil
 	}
 	releaseVersionFromCM, releaseVersionFound := configMap.Data[ctrlcommon.OCPReleaseVersionKey]
 	if !releaseVersionFound {
 		klog.Infof("failed to find OCP release version in %s configmap, sync will exit to wait for the MCO upgrade to complete", ctrlcommon.BootImagesConfigMapName)
-		return nil
+		return true, nil
 	}
 	if releaseVersionFromCM != operatorversion.ReleaseVersion {
 		klog.Infof("mismatch between OCP release version stored in configmap and current MCO release version; sync will exit to wait for the MCO upgrade to complete")
-		return nil
+		return true, nil
 	}
 
 	// Check if the this MachineSet requires an update
-	patchRequired, newMachineSet, err := checkMachineSet(infra, machineSet, configMap, arch, ctrl.kubeClient)
+	patchRequired, patchSkipped, newMachineSet, err := checkMachineSet(infra, machineSet, configMap, arch, ctrl.kubeClient)
 	if err != nil {
-		return fmt.Errorf("failed to reconcile machineset %s, err: %w", machineSet.Name, err)
+		return false, fmt.Errorf("failed to reconcile machineset %s, err: %w", machineSet.Name, err)
 	}
 
 	// Patch the machineset if required
 	if patchRequired {
 		// First, check if we're hot looping
 		if ctrl.checkMAPIMachineSetHotLoop(newMachineSet) {
-			return fmt.Errorf("refusing to reconcile machineset %s, hot loop detected. Please opt-out of boot image updates, adjust your machine provisioning workflow to prevent hot loops and opt back in to resume boot image updates", machineSet.Name)
+			return false, fmt.Errorf("refusing to reconcile machineset %s, hot loop detected. Please opt-out of boot image updates, adjust your machine provisioning workflow to prevent hot loops and opt back in to resume boot image updates", machineSet.Name)
 		}
 		klog.Infof("Patching MAPI machineset %s", machineSet.Name)
-		return ctrl.patchMachineSet(machineSet, newMachineSet)
+		return false, ctrl.patchMachineSet(machineSet, newMachineSet)
 	}
 	klog.Infof("No patching required for MAPI machineset %s", machineSet.Name)
-	return nil
+	return patchSkipped, nil
 }
 
 // Checks against a local store of boot image updates to detect hot looping