avoid reboots by recreating and reusing MOSB with updated rendered spec

Author: dkhater-redhat

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/reconciler.go

@@ -117,7 +117,7 @@ func (b *buildReconciler) updateMachineOSConfig(ctx context.Context, old, cur *m
 
 	// Whenever the MachineOSConfig spec has changed, create a new MachineOSBuild.
 	if !equality.Semantic.DeepEqual(old.Spec, cur.Spec) {
-		klog.Infof("Detected MachineOSConfig change for %s", cur.Name)
+		klog.Infof("[updateMachineOSConfig] Detected MachineOSConfig change and kicking off createNewMachineOSBuildOrReuseExisting for %s", cur.Name)
 		return b.createNewMachineOSBuildOrReuseExisting(ctx, cur, false)
 	}
 
@@ -331,7 +331,7 @@ func (b *buildReconciler) updateMachineOSConfigStatus(ctx context.Context, mosc
 
 	// skip the status update if the current image pullspec equals the digest image pushspec.
 	if mosc.Status.CurrentImagePullSpec == mosb.Status.DigestedImagePushSpec {
-		klog.Infof("MachineOSConfig %q already has final image pushspec for MachineOSBuild %q", mosc.Name, mosb.Name)
+		klog.Infof("[updateMachineOSConfig] MachineOSConfig %q already has final image pushspec for MachineOSBuild %q", mosc.Name, mosb.Name)
 		return nil
 	}
 
@@ -371,16 +371,56 @@ func (b *buildReconciler) UpdateMachineConfigPool(ctx context.Context, oldMCP, c
 // Sepcifically, whenever a new rendered MachineConfig is applied, it will
 // create a new MachineOSBuild in response.
 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 {
-			return fmt.Errorf("could not create or reuse existing MachineOSBuild for MachineConfigPool %q change: %w", curMCP.Name, err)
+
+	// Did the rendered-MC actually change?
+	needsImageRebuild, err := b.reconcilePoolChange(oldMCP, curMCP)
+	if err != nil {
+		return err
+	}
+
+	/////////// REMOVE JUST FOR LOGS ///////////
+	// Get current and desired MachineConfigs
+	curMC, err := b.machineConfigLister.Get(oldMCP.Spec.Configuration.Name)
+	if err != nil {
+		return fmt.Errorf("error getting current MachineConfig %s: %w", oldMCP.Spec.Configuration.Name, err)
+	}
+
+	desMC, err := b.machineConfigLister.Get(curMCP.Spec.Configuration.Name)
+	if err != nil {
+		return fmt.Errorf("error getting desired MachineConfig %s: %w", curMCP.Spec.Configuration.Name, err)
+	}
+
+	klog.Infof("desMC is %s", desMC.Name)
+	klog.Infof("currMC is %s", curMC.Name)
+	/////////// REMOVE JUST FOR LOGS ///////////
+
+	mosc, err := utils.GetMachineOSConfigForMachineConfigPool(curMCP, b.utilListers())
+	if err != nil {
+		return fmt.Errorf("could not find MachineOSConfig for pool %q: %w", curMCP.Name, err)
+	}
+	existingName := mosc.Annotations[constants.CurrentMachineOSBuildAnnotationKey]
+	if existingName == "" {
+		return fmt.Errorf("no current build annotation on MachineOSConfig %q", mosc.Name)
+	}
+	existingMosb, err := b.machineOSBuildLister.Get(existingName)
+	if err != nil {
+		return fmt.Errorf("could not fetch existing MachineOSBuild %q: %w", existingName, err)
+	}
+	if !needsImageRebuild && existingMosb != nil {
+		if err := b.deleteMachineOSBuild(ctx, existingMosb); err != nil {
+			return err
 		}
+		return b.createNewMachineOSBuildOrReuseExisting(ctx, mosc, true)
+	} else {
+		return b.createNewMachineOSBuildOrReuseExistingForPoolChange(ctx, curMCP)
 	}
+}
 
-	// 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)
+func (b *buildReconciler) patchMOSBLabels(ctx context.Context, mosb *mcfgv1.MachineOSBuild, newMCName string) error {
+	mosbCopy := mosb.DeepCopy()
+	mosbCopy.Labels[constants.RenderedMachineConfigLabelKey] = newMCName
+	_, err := b.mcfgclient.MachineconfigurationV1().MachineOSBuilds().Update(ctx, mosbCopy, metav1.UpdateOptions{})
+	return err
 }
 
 // Adds a MachineOSBuild.
@@ -540,8 +580,7 @@ func (b *buildReconciler) createNewMachineOSBuildOrReuseExisting(ctx context.Con
 	// previously built image pullspec and adjust the MachineOSConfig to use that
 	// image instead.
 	if err == nil && existingMosb != nil {
-		imageNeedsRebuild, err := b.reuseExistingMachineOSBuildIfPossible(ctx, mosc, existingMosb)
-		if err != nil {
+		if err := b.reuseExistingMachineOSBuildIfPossible(ctx, mosc, existingMosb, mcp); err != nil {
 			return fmt.Errorf("could not reuse existing MachineOSBuild %q for MachineOSConfig %q: %w", existingMosb.Name, mosc.Name, err)
 		}
 
@@ -556,8 +595,8 @@ 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{})
-		if err != nil && !k8serrors.IsAlreadyExists(err) {
+		mosb, err := b.mcfgclient.MachineconfigurationV1().MachineOSBuilds().Create(ctx, mosb, metav1.CreateOptions{})
+		if err != nil {
 			return fmt.Errorf("could not create new MachineOSBuild %q: %w", mosb.Name, err)
 		}
 		klog.Infof("New MachineOSBuild created: %s", mosb.Name)
@@ -607,7 +646,7 @@ func (b *buildReconciler) getCerts() error {
 }
 
 // Determines if a preexising MachineOSBuild can be reused and if possible, does it.
-func (b *buildReconciler) reuseExistingMachineOSBuildIfPossible(ctx context.Context, mosc *mcfgv1.MachineOSConfig, existingMosb *mcfgv1.MachineOSBuild) (bool, error) {
+func (b *buildReconciler) reuseExistingMachineOSBuildIfPossible(ctx context.Context, mosc *mcfgv1.MachineOSConfig, existingMosb *mcfgv1.MachineOSBuild, mcp *mcfgv1.MachineConfigPool) error {
 	existingMosbState := ctrlcommon.NewMachineOSBuildState(existingMosb)
 
 	canBeReused := false
@@ -661,7 +700,11 @@ func (b *buildReconciler) reuseExistingMachineOSBuildIfPossible(ctx context.Cont
 
 		// Update the MachineOSConfig to use the preexisting MachineOSBuild.
 		if err := b.updateMachineOSConfigStatus(ctx, mosc, existingMosb); err != nil {
-			return canBeReused, fmt.Errorf("could not update MachineOSConfig %q status to reuse preexisting MachineOSBuild %q: %w", mosc.Name, existingMosb.Name, err)
+			return fmt.Errorf("could not update MachineOSConfig %q status to reuse preexisting MachineOSBuild %q: %w", mosc.Name, existingMosb.Name, err)
+		}
+
+		if err := b.patchMOSBLabels(ctx, existingMosb, mcp.Spec.Configuration.Name); err != nil {
+			return fmt.Errorf("could not patch MOSB labels")
 		}
 	}
 
@@ -1188,12 +1231,62 @@ 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
 		}
 
+		// desiredMC, err := b.machineConfigLister.Get(mcp.Spec.Configuration.Name)
+		// if err != nil {
+		// 	return fmt.Errorf("error getting desired MC: %w", err)
+		// }
+
+		// // existing MC labels
+		// currentWithImage := mosb.Labels[constants.RenderedMCWithImageLabel]
+		// currMC := mosb.Labels[constants.RenderedMachineConfigLabelKey]
+
+		// oldMC, err := b.machineConfigLister.Get(currMC)
+		// if err != nil {
+		// 	return fmt.Errorf("could not fetch old MachineConfig %q: %w", currMC, err)
+		// }
+
+		// // new MC labels
+		// desMC := desiredMC.Name
+		// newWithImage := currentWithImage
+
+		// mosbCopy := mosb.DeepCopy()
+		// updateLabels := false
+
+		// if currMC != desMC {
+		// 	klog.Info("Current MC does not equal the new MC, we need to update our labels to reflect that")
+		// 	mosbCopy.Labels[constants.RenderedMachineConfigLabelKey] = desMC
+		// 	updateLabels = true
+		// }
+
+		// if ctrlcommon.RequiresRebuild(oldMC, desiredMC) && currentWithImage != newWithImage {
+		// 	mosbCopy.Labels[constants.RenderedMCWithImageLabel] = newWithImage
+		// 	updateLabels = true
+		// }
+
+		// if updateLabels {
+		// 	klog.Infof("syncMachineOSBuild: Updating labels for MOSB %q (currentNoImage=%s, desiredMCName=%s, desiredWithImage=%s, currentWithImage=%s)",
+		// 		mosb.Name, currMC, desMC, newWithImage, currentWithImage)
+
+		// 	mosbCopy.Labels[constants.RenderedMachineConfigLabelKey] = desMC
+		// 	mosbCopy.Labels[constants.RenderedMCWithImageLabel] = newWithImage
+
+		// 	// Update the MOSB object
+		// 	_, err := b.mcfgclient.MachineconfigurationV1().MachineOSBuilds().Update(ctx, mosbCopy, metav1.UpdateOptions{})
+		// 	if err != nil {
+		// 		klog.Errorf("syncMachineOSBuild: Failed to update MOSB %q labels: %v", mosb.Name, err)
+
+		// 		return fmt.Errorf("failed to sync labels: %v", err)
+		// 	}
+		// 	klog.Infof("syncMachineOSBuild: Updated MOSB %q labels to RenderedMCNoImageLabel=%s, RenderedMCWithImageLabel=%s",
+		// 		mosb.Name, desiredMC.Name, newWithImage)
+		// 	return nil
+		// }
+
 		mosbState := ctrlcommon.NewMachineOSBuildState(mosb)
 
 		if mosbState.IsInTerminalState() {
@@ -1290,7 +1383,7 @@ func (b *buildReconciler) syncMachineOSConfig(ctx context.Context, mosc *mcfgv1.
 			}
 		}
 
-		klog.Infof("No matching MachineOSBuild found for MachineOSConfig %q, will create one", mosc.Name)
+		klog.Infof("syncMachineOSConfig: No matching MachineOSBuild found for MachineOSConfig %q, will create one by kicking off createNewMachineOSBuildOrReuseExisting", mosc.Name)
 		if err := b.createNewMachineOSBuildOrReuseExisting(ctx, mosc, false); err != nil {
 			return fmt.Errorf("could not create new or reuse existing MachineOSBuild for MachineOSConfig %q: %w", mosc.Name, err)
 		}
@@ -1327,7 +1420,67 @@ func (b *buildReconciler) syncMachineConfigPools(ctx context.Context) error {
 // MachineOSConfigs and MachineOSBuilds, which will create a new MachineOSBuild,
 // 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)
-	})
+	mosc, err := utils.GetMachineOSConfigForMachineConfigPool(mcp, b.utilListers())
+	if err != nil {
+		// Not opted in yet, so nothing for us to do.
+		klog.V(4).Infof("buildReconciler: no MachineOSConfig for pool %q, skipping", mcp.Name)
+		return nil
+	}
+	// old pool
+	old := mcp.DeepCopy()
+	old.Spec.Configuration.Name = mcp.Status.Configuration.Name
+
+	// decide if we annotate or reuse
+	needsImageRebuild, err := b.reconcilePoolChange(old, mcp)
+	if err != nil {
+		return err
+	}
+
+	existingName := mosc.Annotations[constants.CurrentMachineOSBuildAnnotationKey]
+	if existingName == "" {
+		return fmt.Errorf("no current build annotation on MachineOSConfig %q", mosc.Name)
+	}
+
+	existingMosb, err := b.machineOSBuildLister.Get(existingName)
+	if err != nil {
+		return fmt.Errorf("could not fetch existing MachineOSBuild %q: %w", existingName, err)
+	}
+	klog.Infof("existing MOSB is %s", existingMosb.Name)
+
+	if !needsImageRebuild && existingMosb != nil {
+		if err := b.deleteMachineOSBuild(ctx, existingMosb); err != nil {
+			return err
+		}
+		return b.createNewMachineOSBuildOrReuseExisting(ctx, mosc, true)
+	}
+	return b.createNewMachineOSBuildOrReuseExistingForPoolChange(ctx, mcp)
+
+}
+
+// reconcilePoolChange returns (annotateRebuild bool, reuseOnly bool, err error)
+func (b *buildReconciler) reconcilePoolChange(oldMCP, curMCP *mcfgv1.MachineConfigPool) (bool, error) {
+
+	// // if the old mc is the same as the new mc, no updates needed
+	// if oldMCP.Spec.Configuration.Name == curMCP.Spec.Configuration.Name {
+	// 	klog.Info("old mc is the same as the new mc")
+	// 	return false, true, nil
+	// }
+
+	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 {
+		klog.Info("needsimage rebuild is true, signalling a rebuild")
+		return true, nil
+	}
+
+	klog.Info("reconciled pool change, no rebuild needed")
+	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)
+}