Merge pull request #5382 from dkhater-redhat/mco-1940-safer-layered-image-serving

 MCO-1940: Enhance MCS layered image serving safety during node scale-up by requiring node validation

Author: openshift-merge-bot[bot]

pkg/server/bootstrap_server.go

@@ -134,7 +134,11 @@ func (bsc *bootstrapServer) GetConfig(cr poolRequest) (*runtime.RawExtension, er
 
 	addDataAndMaybeAppendToIgnition(caBundleFilePath, cc.Spec.KubeAPIServerServingCAData, &ignConf)
 	addDataAndMaybeAppendToIgnition(cloudProviderCAPath, cc.Spec.CloudProviderCAData, &ignConf)
-	appenders := getAppenders(currConf, nil, bsc.kubeconfigFunc, bsc.certs, bsc.serverBaseDir)
+
+	appenders := newAppendersBuilder(nil, bsc.kubeconfigFunc, bsc.certs, bsc.serverBaseDir).
+		WithNodeAnnotations(currConf, "").
+		build()
+
 	for _, a := range appenders {
 		if err := a(&ignConf, mc); err != nil {
 			return nil, err

pkg/server/cluster_server.go

@@ -185,24 +185,11 @@ func (cs *clusterServer) GetConfig(cr poolRequest) (*runtime.RawExtension, error
 	addDataAndMaybeAppendToIgnition(cloudProviderCAPath, cc.Spec.CloudProviderCAData, &ignConf)
 
 	desiredImage := cs.resolveDesiredImageForPool(mp)
-	klog.Infof("desiredImage is found to be %s", desiredImage)
 
-	appenders := []appenderFunc{
-		func(cfg *ign3types.Config, _ *mcfgv1.MachineConfig) error {
-			return appendNodeAnnotations(cfg, currConf, desiredImage)
-		},
-		func(cfg *ign3types.Config, _ *mcfgv1.MachineConfig) error {
-			return appendKubeConfig(cfg, cs.kubeconfigFunc)
-		},
-		appendInitialMachineConfig,
-		func(cfg *ign3types.Config, _ *mcfgv1.MachineConfig) error { return appendCerts(cfg, []string{}, "") },
-		// Inject desired image into the MC
-		appendDesiredOSImage(desiredImage),
-		// Must be last
-		func(cfg *ign3types.Config, mc *mcfgv1.MachineConfig) error {
-			return appendEncapsulated(cfg, mc, cr.version)
-		},
-	}
+	appenders := newAppendersBuilder(cr.version, cs.kubeconfigFunc, []string{}, "").
+		WithNodeAnnotations(currConf, desiredImage).
+		WithCustomAppender(appendDesiredOSImage(desiredImage)).
+		build()
 
 	for _, a := range appenders {
 		if err := a(&ignConf, mc); err != nil {
@@ -308,20 +295,6 @@ func (cs *clusterServer) resolveDesiredImageForPool(pool *mcfgv1.MachineConfigPo
 		return ""
 	}
 
-	// Also verify the corresponding MOSB is successful to ensure we don't serve an image that hasn't been built yet
-	//
-	// Note: We cannot directly use the MOSB reference from mosc.Status.MachineOSBuild here because
-	// there can be multiple MOSBs for a single MOSC (one per rendered MachineConfig). The MOSC status
-	// points to the latest successful build, but we need the MOSB matching the pool's rollout state.
-	// We use pool.Status.Configuration.Name (old config) when UpdatedMachineCount == 0, and
-	// pool.Spec.Configuration.Name (new config) when UpdatedMachineCount > 0. This matches the existing
-	// MCS behavior for non-layered nodes and ensures we serve the correct image during rollouts.
-	//
-	// TODO(dkhater): For added safety during node scale-up, we should only serve the new build if it's
-	// both successful AND at least one node has already updated to it. This would more closely match
-	// the existing MCS rollout behavior and provide better safety, though it would result in additional
-	// reboots when scaling nodes during an update.
-	// See https://issues.redhat.com/browse/MCO-1940 for tracking this improvement.
 	mosbList, err := cs.machineOSBuildLister.List(labels.Everything())
 	if err != nil {
 		klog.Infof("Could not list MachineOSBuilds for pool %s: %v", pool.Name, err)
@@ -356,6 +329,12 @@ func (cs *clusterServer) resolveDesiredImageForPool(pool *mcfgv1.MachineConfigPo
 		return ""
 	}
 
+	// Only serve layered images if at least one node has completed the update.
+	if pool.Status.UpdatedMachineCount == 0 {
+		klog.Infof("Pool %s has successful MOSB %s but no nodes have completed update yet (UpdatedMachineCount=0), will not serve layered image during bootstrap", pool.Name, mosb.Name)
+		return ""
+	}
+
 	imageSpec := moscState.GetOSImage()
 
 	// Don't serve internal registry images during bootstrap because cluster DNS is not available yet

pkg/server/server.go

@@ -46,23 +46,72 @@ type Server interface {
 	GetConfig(poolRequest) (*runtime.RawExtension, error)
 }
 
-func getAppenders(currMachineConfig string, version *semver.Version, f kubeconfigFunc, certs []string, serverDir string) []appenderFunc {
-	appenders := []appenderFunc{
-		// append machine annotations file.
+// appendersBuilder builds a slice of appenderFunc with guaranteed ordering.
+// The builder ensures that appendEncapsulated is always added last.
+type appendersBuilder struct {
+	version         *semver.Version
+	kubeconfigFn    kubeconfigFunc
+	certs           []string
+	serverDir       string
+	customAppenders []appenderFunc
+}
+
+// newAppendersBuilder creates a new appendersBuilder.
+// Common appenders (kubeconfig, initial machine config, certs) are added automatically in build().
+func newAppendersBuilder(version *semver.Version, kubeconfigFn kubeconfigFunc, certs []string, serverDir string) *appendersBuilder {
+	return &appendersBuilder{
+		version:         version,
+		kubeconfigFn:    kubeconfigFn,
+		certs:           certs,
+		serverDir:       serverDir,
+		customAppenders: make([]appenderFunc, 0),
+	}
+}
+
+// WithNodeAnnotations adds the node annotations appender with the specified config and image.
+func (ab *appendersBuilder) WithNodeAnnotations(currMachineConfig, image string) *appendersBuilder {
+	ab.customAppenders = append(ab.customAppenders, func(cfg *ign3types.Config, _ *mcfgv1.MachineConfig) error {
+		return appendNodeAnnotations(cfg, currMachineConfig, image)
+	})
+	return ab
+}
+
+// WithCustomAppender adds a custom appender function.
+// Custom appenders are inserted before the common appenders and appendEncapsulated.
+func (ab *appendersBuilder) WithCustomAppender(appender appenderFunc) *appendersBuilder {
+	ab.customAppenders = append(ab.customAppenders, appender)
+	return ab
+}
+
+// build creates the final slice of appenderFunc with the correct ordering:
+// 1. Custom appenders (node annotations, desired image, etc.)
+// 2. Common appenders (kubeconfig, initial machine config, certs)
+// 3. appendEncapsulated (always last)
+func (ab *appendersBuilder) build() []appenderFunc {
+	result := make([]appenderFunc, 0, len(ab.customAppenders)+4)
+
+	// Add custom appenders first
+	result = append(result, ab.customAppenders...)
+
+	// Add common appenders and appendEncapsulated
+	result = append(result,
+		// append kubeconfig
 		func(cfg *ign3types.Config, _ *mcfgv1.MachineConfig) error {
-			return appendNodeAnnotations(cfg, currMachineConfig, "")
+			return appendKubeConfig(cfg, ab.kubeconfigFn)
 		},
-		// append kubeconfig.
-		func(cfg *ign3types.Config, _ *mcfgv1.MachineConfig) error { return appendKubeConfig(cfg, f) },
 		// append the machineconfig content
 		appendInitialMachineConfig,
-		func(cfg *ign3types.Config, _ *mcfgv1.MachineConfig) error { return appendCerts(cfg, certs, serverDir) },
-		// This has to come last!!!
+		// append certs
+		func(cfg *ign3types.Config, _ *mcfgv1.MachineConfig) error {
+			return appendCerts(cfg, ab.certs, ab.serverDir)
+		},
+		// appendEncapsulated must always be last
 		func(cfg *ign3types.Config, mc *mcfgv1.MachineConfig) error {
-			return appendEncapsulated(cfg, mc, version)
+			return appendEncapsulated(cfg, mc, ab.version)
 		},
-	}
-	return appenders
+	)
+
+	return result
 }
 
 func appendCerts(cfg *ign3types.Config, certs []string, serverDir string) error {