unify update paths for non-OCL and OCL and avoid reboots by recreating and reusing MOSB's with updated rendered spec

Author: cheesesashimi

pkg/controller/build/clients.go

@@ -25,6 +25,7 @@ import (
 type informers struct {
 	controllerConfigInformer  mcfginformersv1.ControllerConfigInformer
 	machineConfigPoolInformer mcfginformersv1.MachineConfigPoolInformer
+	machineConfigInformer     mcfginformersv1.MachineConfigInformer
 	jobInformer               batchinformersv1.JobInformer
 	machineOSBuildInformer    mcfginformersv1.MachineOSBuildInformer
 	machineOSConfigInformer   mcfginformersv1.MachineOSConfigInformer
@@ -46,6 +47,7 @@ func (i *informers) listers() *listers {
 		machineOSBuildLister:    i.machineOSBuildInformer.Lister(),
 		machineOSConfigLister:   i.machineOSConfigInformer.Lister(),
 		machineConfigPoolLister: i.machineConfigPoolInformer.Lister(),
+		machineConfigLister:     i.machineConfigInformer.Lister(),
 		jobLister:               i.jobInformer.Lister(),
 		controllerConfigLister:  i.controllerConfigInformer.Lister(),
 		nodeLister:              i.nodeInformer.Lister(),
@@ -57,6 +59,7 @@ type listers struct {
 	machineOSBuildLister    mcfglistersv1.MachineOSBuildLister
 	machineOSConfigLister   mcfglistersv1.MachineOSConfigLister
 	machineConfigPoolLister mcfglistersv1.MachineConfigPoolLister
+	machineConfigLister     mcfglistersv1.MachineConfigLister
 	jobLister               batchlisterv1.JobLister
 	controllerConfigLister  mcfglistersv1.ControllerConfigLister
 	nodeLister              corelistersv1.NodeLister
@@ -91,6 +94,7 @@ func newInformers(mcfgclient mcfgclientset.Interface, kubeclient clientset.Inter
 	controllerConfigInformer := mcoInformerFactory.Machineconfiguration().V1().ControllerConfigs()
 	machineConfigPoolInformer := mcoInformerFactory.Machineconfiguration().V1().MachineConfigPools()
 	machineOSBuildInformer := mcoInformerFactory.Machineconfiguration().V1().MachineOSBuilds()
+	machineConfigInformer := mcoInformerFactory.Machineconfiguration().V1().MachineConfigs()
 	machineOSConfigInformer := mcoInformerFactory.Machineconfiguration().V1().MachineOSConfigs()
 	jobInformer := coreInformerFactory.Batch().V1().Jobs()
 	nodeInformer := coreInformerFactoryNodes.Core().V1().Nodes()
@@ -100,6 +104,7 @@ func newInformers(mcfgclient mcfgclientset.Interface, kubeclient clientset.Inter
 		machineConfigPoolInformer: machineConfigPoolInformer,
 		machineOSBuildInformer:    machineOSBuildInformer,
 		machineOSConfigInformer:   machineOSConfigInformer,
+		machineConfigInformer:     machineConfigInformer,
 		jobInformer:               jobInformer,
 		nodeInformer:              nodeInformer,
 		toStart: []interface{ Start(<-chan struct{}) }{

pkg/controller/build/osbuildcontroller_test.go

@@ -756,3 +756,36 @@ func assertMachineOSConfigGetsCurrentBuildAnnotation(ctx context.Context, t *tes
 
 	require.NoError(t, err)
 }
+
+// Test that when the MCP’s rendered-MC name changes but the two MCs only differ
+// by on-cluster layering, no Build Job is created.
+func TestOSBuildControllerSkipsBuildForLayerOnlyChanges(t *testing.T) {
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	t.Cleanup(cancel)
+	poolName := "worker"
+
+	_, mcfgclient, _, _, mosc, firstMosb, mcp, kubeassert := setupOSBuildControllerForTestWithSuccessfulBuild(ctx, t, poolName)
+	isMachineOSBuildReachedExpectedCount(ctx, t, mcfgclient, mosc, 1)
+
+	assertMachineOSConfigGetsCurrentBuildAnnotation(ctx, t, mcfgclient, mosc, firstMosb)
+
+	isMachineOSBuildReachedExpectedCount(ctx, t, mcfgclient, mosc, 1)
+
+	insertNewRenderedMachineConfigAndUpdatePool(ctx, t, mcfgclient, mcp.Name, "rendered-worker-layer-only")
+
+	// Give the controller a moment to (not) kick off a build
+	time.Sleep(200 * time.Millisecond)
+
+	mosbList, err := mcfgclient.MachineconfigurationV1().
+		MachineOSBuilds().
+		List(ctx, metav1.ListOptions{LabelSelector: utils.MachineOSBuildForPoolSelector(mosc).String()})
+	require.NoError(t, err)
+	require.Len(t, mosbList.Items, 2, "expected a new MOSB to be created for layering-only change")
+	assert.Equal(t, firstMosb.Name, mosbList.Items[0].Name, "first MOSB should remain unchanged")
+
+	layerOnlyMosb := mosbList.Items[1]
+
+	jobName := utils.GetBuildJobName(&layerOnlyMosb)
+
+	kubeassert.JobDoesNotExist(jobName, "layering-only MOSB should not spawn a build Job")
+}

pkg/controller/build/reconciler.go

@@ -13,6 +13,7 @@ import (
 	imagev1clientset "github.com/openshift/client-go/image/clientset/versioned"
 	mcfgclientset "github.com/openshift/client-go/machineconfiguration/clientset/versioned"
 	routeclientset "github.com/openshift/client-go/route/clientset/versioned"
+	"github.com/openshift/machine-config-operator/pkg/apihelpers"
 	"github.com/openshift/machine-config-operator/pkg/controller/build/buildrequest"
 	"github.com/openshift/machine-config-operator/pkg/controller/build/constants"
 	"github.com/openshift/machine-config-operator/pkg/controller/build/imagebuilder"
@@ -373,13 +374,11 @@ func (b *buildReconciler) UpdateMachineConfigPool(ctx context.Context, oldMCP, c
 func (b *buildReconciler) updateMachineConfigPool(ctx context.Context, oldMCP, curMCP *mcfgv1.MachineConfigPool) error {
 	if oldMCP.Spec.Configuration.Name != curMCP.Spec.Configuration.Name {
 		klog.Infof("Rendered config for pool %s changed from %s to %s", curMCP.Name, oldMCP.Spec.Configuration.Name, curMCP.Spec.Configuration.Name)
-		if err := b.createNewMachineOSBuildOrReuseExistingForPoolChange(ctx, curMCP); err != nil {
+		if err := b.reconcilePoolChange(ctx, curMCP); err != nil {
 			return fmt.Errorf("could not create or reuse existing MachineOSBuild for MachineConfigPool %q change: %w", curMCP.Name, err)
 		}
 	}
 
-	// Not sure if we need to do this here yet or not.
-	// TODO: Determine if we should call b.syncMachineConfigPools() here or not.
 	return b.syncAll(ctx)
 }
 
@@ -556,7 +555,7 @@ func (b *buildReconciler) createNewMachineOSBuildOrReuseExisting(ctx context.Con
 	// In this situation, we've determined that the MachineOSBuild does not
 	// exist, so we need to create it.
 	if k8serrors.IsNotFound(err) {
-		_, err := b.mcfgclient.MachineconfigurationV1().MachineOSBuilds().Create(ctx, mosb, metav1.CreateOptions{})
+		mosb, err := b.mcfgclient.MachineconfigurationV1().MachineOSBuilds().Create(ctx, mosb, metav1.CreateOptions{})
 		if err != nil && !k8serrors.IsAlreadyExists(err) {
 			return fmt.Errorf("could not create new MachineOSBuild %q: %w", mosb.Name, err)
 		}
@@ -1188,12 +1187,35 @@ func (b *buildReconciler) syncMachineOSBuild(ctx context.Context, mosb *mcfgv1.M
 			return fmt.Errorf("could not get MachineConfigPool from MachineOSBuild %q: %w", mosb.Name, err)
 		}
 
-		// An mosb which had previously been forgotten by the queue and is no longer desired by the mcp should not build
 		if mosb.ObjectMeta.Labels[constants.RenderedMachineConfigLabelKey] != mcp.Spec.Configuration.Name {
 			klog.Infof("The MachineOSBuild %q which builds the rendered Machine Config %q is no longer desired by the MCP %q", mosb.Name, mosb.ObjectMeta.Labels[constants.RenderedMachineConfigLabelKey], mosb.ObjectMeta.Labels[constants.TargetMachineConfigPoolLabelKey])
 			return nil
 		}
 
+		oldMCName := mcp.Status.Configuration.Name
+		newMCName := mcp.Spec.Configuration.Name
+
+		oldRendered, err := b.machineConfigLister.Get(oldMCName)
+		if err != nil {
+			return err
+		}
+		newRendered, err := b.machineConfigLister.Get(newMCName)
+		if err != nil {
+			return err
+		}
+
+		old := mcp.DeepCopy()
+		old.Spec.Configuration.Name = mcp.Status.Configuration.Name
+
+		needsImageRebuild, err := b.reconcileImageRebuild(old, mcp)
+		if err != nil {
+			return err
+		}
+		if oldRendered != newRendered && !needsImageRebuild {
+			klog.Infof("MachineOSBuild %q: only layering changes → skipping image build", mosb.Name)
+			return nil
+		}
+
 		mosbState := ctrlcommon.NewMachineOSBuildState(mosb)
 
 		if mosbState.IsInTerminalState() {
@@ -1328,6 +1350,128 @@ func (b *buildReconciler) syncMachineConfigPools(ctx context.Context) error {
 // if needed.
 func (b *buildReconciler) syncMachineConfigPool(ctx context.Context, mcp *mcfgv1.MachineConfigPool) error {
 	return b.timeObjectOperation(mcp, syncingVerb, func() error {
-		return b.createNewMachineOSBuildOrReuseExistingForPoolChange(ctx, mcp)
+		return b.reconcilePoolChange(ctx, mcp)
+	})
+}
+
+func (b *buildReconciler) reconcilePoolChange(ctx context.Context, mcp *mcfgv1.MachineConfigPool) error {
+	mosc, err := utils.GetMachineOSConfigForMachineConfigPool(mcp, b.utilListers())
+	if err != nil {
+		// Not opted in yet, so nothing for us to do.
+		klog.Infof("buildReconciler: no MachineOSConfig for pool %q, skipping", mcp.Name)
+		return nil
+	}
+
+	oldRendered := mcp.Status.Configuration.Name
+	newRendered := mcp.Spec.Configuration.Name
+
+	// old pool
+	old := mcp.DeepCopy()
+	old.Spec.Configuration.Name = mcp.Status.Configuration.Name
+	firstOptIn := mosc.Annotations[constants.CurrentMachineOSBuildAnnotationKey]
+	if firstOptIn == "" {
+		return fmt.Errorf("no current build annotation on MachineOSConfig %q", mosc.Name)
+	}
+
+	needsImageRebuild, err := b.reconcileImageRebuild(old, mcp)
+	if err != nil {
+		return err
+	}
+
+	if (oldRendered != newRendered && needsImageRebuild) || firstOptIn == "" {
+		klog.Infof("pool %q: detected extension/kernel/OSImageURL change → will rebuild image", mcp.Name)
+	} else if oldRendered != newRendered && !needsImageRebuild {
+		klog.Infof("pool %q: only layering changes → reusing existing image", mcp.Name)
+		prevPullSpec := mosc.Status.CurrentImagePullSpec
+		oldMOSB, err := utils.GetMachineOSBuildForImagePullspec(string(prevPullSpec), b.utilListers())
+		if err != nil {
+			return fmt.Errorf("failed to look up MachineOSBuild %q: %w", oldMOSB.Name, err)
+		}
+		return b.reuseImageForNewMOSB(ctx, mosc, oldMOSB)
+	}
+
+	return b.createNewMachineOSBuildOrReuseExisting(ctx, mosc, needsImageRebuild)
+
+}
+
+// reuseImageForNewMOSB creates a new MOSB (for the new rendered-MC name)
+// but populates its status from oldMosb so that no build actually runs.
+func (b *buildReconciler) reuseImageForNewMOSB(ctx context.Context, mosc *mcfgv1.MachineOSConfig, oldMosb *mcfgv1.MachineOSBuild,
+) error {
+	// Look up the MCP
+	mcp, err := b.machineConfigPoolLister.Get(mosc.Spec.MachineConfigPool.Name)
+	if err != nil {
+		return err
+	}
+
+	// Build the new MOSB object
+	osImageURLs, err := ctrlcommon.GetOSImageURLConfig(ctx, b.kubeclient)
+	if err != nil {
+		return err
+	}
+	newMosb, err := buildrequest.NewMachineOSBuild(
+		buildrequest.MachineOSBuildOpts{
+			MachineOSConfig:   mosc,
+			MachineConfigPool: mcp,
+			OSImageURLConfig:  osImageURLs,
+		})
+	if err != nil {
+		return err
+	}
+	newMosb.SetOwnerReferences([]metav1.OwnerReference{
+		*metav1.NewControllerRef(mosc, mcfgv1.SchemeGroupVersion.WithKind("MachineOSConfig")),
 	})
+
+	// Create it if not already there
+	_, err = b.machineOSBuildLister.Get(newMosb.Name)
+	if k8serrors.IsNotFound(err) {
+		if newMosb, err = b.mcfgclient.
+			MachineconfigurationV1().
+			MachineOSBuilds().
+			Create(ctx, newMosb, metav1.CreateOptions{}); err != nil {
+			return err
+		}
+	} else if err != nil {
+		return err
+	}
+
+	// Change its status so it’s “Succeeded” with the old image
+	toUpdate, err := b.getMachineOSBuildForUpdate(newMosb)
+	if err != nil {
+		return err
+	}
+	oldStatus := toUpdate.Status
+
+	toUpdate.Status.DigestedImagePushSpec = oldMosb.Status.DigestedImagePushSpec
+
+	for _, c := range apihelpers.MachineOSBuildSucceededConditions() {
+		apihelpers.SetMachineOSBuildCondition(&toUpdate.Status, c)
+	}
+
+	if err := b.setStatusOnMachineOSBuildIfNeeded(ctx, toUpdate, oldStatus, toUpdate.Status); err != nil {
+		return err
+	}
+
+	// re-point the MC to the NEW build
+	return b.updateMachineOSConfigStatus(ctx, mosc, toUpdate)
+}
+
+// reconcilePoolChange returns (annotateRebuild bool, reuseOnly bool, err error)
+func (b *buildReconciler) reconcileImageRebuild(oldMCP, curMCP *mcfgv1.MachineConfigPool) (bool, error) {
+
+	curr, err := b.machineConfigLister.Get(oldMCP.Spec.Configuration.Name)
+	if err != nil {
+		return false, err
+	}
+	des, err := b.machineConfigLister.Get(curMCP.Spec.Configuration.Name)
+	if err != nil {
+		return false, err
+	}
+
+	needsImageRebuild := ctrlcommon.RequiresRebuild(curr, des)
+	if needsImageRebuild {
+		return true, nil
+	}
+
+	return false, nil
 }

pkg/controller/common/helpers.go

@@ -1462,3 +1462,11 @@ func GetCAsFromConfigMap(cm *corev1.ConfigMap, key string) ([]byte, error) {
 	}
 	return nil, fmt.Errorf("%s not found in %s/%s", key, cm.Namespace, cm.Name)
 }
+
+// Determines if an on-cluster layering image rollout and rebuild is required for the changes applied on the new MC
+func RequiresRebuild(oldMC, newMC *mcfgv1.MachineConfig) bool {
+	return oldMC.Spec.OSImageURL != newMC.Spec.OSImageURL ||
+		oldMC.Spec.KernelType != newMC.Spec.KernelType ||
+		!reflect.DeepEqual(oldMC.Spec.Extensions, newMC.Spec.Extensions) ||
+		!reflect.DeepEqual(oldMC.Spec.KernelArguments, newMC.Spec.KernelArguments)
+}

pkg/controller/node/node_controller.go

@@ -901,7 +901,7 @@ func (ctrl *Controller) getConfigAndBuildAndLayeredStatus(pool *mcfgv1.MachineCo
 
 	isLayered, err := ctrl.isLayeredPool(mosc, mosb)
 	if err != nil {
-		return nil, nil, false, fmt.Errorf("Failed to determine whether pool %s opts in to OCL due to an error: %s", pool.Name, err)
+		return nil, nil, false, fmt.Errorf("failed to check layered status: %w", err)
 	}
 
 	return mosc, mosb, isLayered, nil
@@ -948,9 +948,8 @@ func (ctrl *Controller) getConfigAndBuild(pool *mcfgv1.MachineConfigPool) (*mcfg
 }
 
 func (ctrl *Controller) canLayeredContinue(mosc *mcfgv1.MachineOSConfig, mosb *mcfgv1.MachineOSBuild) (string, bool, error) {
-	// This is an edgecase which we should ideally never hit. However, it is
-	// better to anticipate it and have an error message ready vs. the
-	// alternative.
+
+	klog.Infof("Can Layered continue?")
 	if mosc == nil && mosb != nil {
 		msg := fmt.Sprintf("orphaned MachineOSBuild %q found, but MachineOSConfig %q not found", mosb.Name, mosb.Labels[buildconstants.MachineOSConfigNameLabelKey])
 		return msg, false, fmt.Errorf("%s", msg)
@@ -968,6 +967,8 @@ func (ctrl *Controller) canLayeredContinue(mosc *mcfgv1.MachineOSConfig, mosb *m
 
 	if !hasImage {
 		return "Desired image not set in MachineOSConfig", false, nil
+	} else if hasImage {
+		klog.Infof("pullspec is %s", pullspec)
 	}
 
 	switch {
@@ -1050,6 +1051,7 @@ func (ctrl *Controller) syncMachineConfigPool(key string) error {
 	}
 
 	if layered {
+		klog.Infof("Continuing updates for layered pool %s", pool.Name)
 		reason, canApplyUpdates, err := ctrl.canLayeredContinue(mosc, mosb)
 		if err != nil {
 			klog.Infof("Layered pool %s encountered an error: %s", pool.Name, err)
@@ -1062,11 +1064,10 @@ func (ctrl *Controller) syncMachineConfigPool(key string) error {
 			return ctrl.syncStatusOnly(pool)
 		}
 
-		klog.V(4).Infof("Continuing updates for layered pool %s", pool.Name)
-	} else {
-		klog.V(4).Infof("Pool %s is not layered", pool.Name)
+		klog.Infof("Continuing updates for layered pool %s", pool.Name)
 	}
 
+	klog.Info("getting nodes for pool")
 	nodes, err := ctrl.getNodesForPool(pool)
 	if err != nil {
 		if syncErr := ctrl.syncStatusOnly(pool); syncErr != nil {
@@ -1076,6 +1077,7 @@ func (ctrl *Controller) syncMachineConfigPool(key string) error {
 		return err
 	}
 
+	klog.Info("maxunavailable check")
 	maxunavail, err := maxUnavailable(pool, nodes)
 	if err != nil {
 		if syncErr := ctrl.syncStatusOnly(pool); syncErr != nil {
@@ -1113,6 +1115,7 @@ func (ctrl *Controller) syncMachineConfigPool(key string) error {
 			}
 		}
 	}
+	klog.Info("get all candidate machines")
 	candidates, capacity := getAllCandidateMachines(layered, mosc, mosb, pool, nodes, maxunavail)
 	if len(candidates) > 0 {
 		zones := make(map[string]bool)
@@ -1130,6 +1133,7 @@ func (ctrl *Controller) syncMachineConfigPool(key string) error {
 			}
 			return err
 		}
+		klog.Info("updating state metric")
 		ctrlcommon.UpdateStateMetric(ctrlcommon.MCCSubControllerState, "machine-config-controller-node", "Sync Machine Config Pool", pool.Name)
 	}
 	return ctrl.syncStatusOnly(pool)
@@ -1282,16 +1286,16 @@ func getAllCandidateMachines(layered bool, config *mcfgv1.MachineOSConfig, build
 		} else {
 			if lns.IsDesiredEqualToBuild(config, build) {
 				// If the node's desired annotations match the pool, return directly without updating the node.
-				klog.V(4).Infof("Pool %s: layered node %s: no update is needed", pool.Name, node.Name)
+				klog.Infof("Pool %s: layered node %s: no update is needed", pool.Name, node.Name)
 				continue
 			}
 		}
 		// Ignore nodes that are currently mid-update or unscheduled
 		if !isNodeReady(node) {
-			klog.V(4).Infof("node %s skipped during candidate selection as it is currently unscheduled", node.Name)
+			klog.Infof("node %s skipped during candidate selection as it is currently unscheduled", node.Name)
 			continue
 		}
-		klog.V(4).Infof("Pool %s: selected candidate node %s", pool.Name, node.Name)
+		klog.Infof("Pool %s: selected candidate node %s", pool.Name, node.Name)
 		nodes = append(nodes, node)
 	}
 	// Nodes which are failing to target this config also count against
@@ -1388,9 +1392,11 @@ func (ctrl *Controller) setDesiredAnnotations(layered bool, mosc *mcfgv1.Machine
 
 	if layered {
 		eventName = "SetDesiredConfigAndOSImage"
-		updateName = fmt.Sprintf("%s / Image: %s", updateName, ctrlcommon.NewMachineOSConfigState(mosc).GetOSImage())
-		klog.Infof("Continuing to sync layered MachineConfigPool %s", pool.Name)
+		moscImage := ctrlcommon.NewMachineOSConfigState(mosc).GetOSImage()
+		updateName = fmt.Sprintf("%s / Image: %s", updateName, moscImage)
+		klog.Infof("Continuing to sync layered MachineConfigPool %s with %s", pool.Name, moscImage)
 	}
+
 	for _, node := range candidates {
 		if err := ctrl.updateCandidateNode(mosc, mosb, node.Name, pool); err != nil {
 			return fmt.Errorf("setting desired %s for node %s: %w", pool.Spec.Configuration.Name, node.Name, err)