Merge pull request #5697 from umohnani8/osimgstream-spec

MCO-2136: Implement osImageStream inheritance for custom MCPs

Author: openshift-merge-bot[bot]

pkg/controller/common/helpers.go

@@ -49,6 +49,7 @@ import (
 	opv1 "github.com/openshift/api/operator/v1"
 	mcfgclientset "github.com/openshift/client-go/machineconfiguration/clientset/versioned"
 	"github.com/openshift/client-go/machineconfiguration/clientset/versioned/scheme"
+	mcfglistersv1 "github.com/openshift/client-go/machineconfiguration/listers/machineconfiguration/v1"
 	"github.com/openshift/library-go/pkg/crypto"
 	buildconstants "github.com/openshift/machine-config-operator/pkg/controller/build/constants"
 )
@@ -1430,3 +1431,43 @@ func GetPreBuiltImageMachineConfigsFromList(mcs []*mcfgv1.MachineConfig) map[str
 
 	return preBuiltMCs
 }
+
+// IsCustomPool returns true if the pool is a custom pool (not master, worker, or arbiter)
+func IsCustomPool(pool *mcfgv1.MachineConfigPool) bool {
+	return pool.Name != MachineConfigPoolMaster &&
+		pool.Name != MachineConfigPoolWorker &&
+		pool.Name != MachineConfigPoolArbiter
+}
+
+// GetEffectiveOSImageStreamName returns the effective osImageStream name for a pool,
+// taking inheritance from the worker pool into account for custom pools.
+// Returns:
+// - The pool's explicit osImageStream.Name if set
+// - For custom pools: the worker pool's osImageStream.Name if the custom pool has none
+// - Empty string (which will resolve to the default stream)
+//
+// This function can return an error if the worker pool cannot be fetched.
+func GetEffectiveOSImageStreamName(pool *mcfgv1.MachineConfigPool, mcpLister mcfglistersv1.MachineConfigPoolLister) (string, error) {
+	// If the pool explicitly sets an osImageStream, use it
+	if pool.Spec.OSImageStream.Name != "" {
+		return pool.Spec.OSImageStream.Name, nil
+	}
+
+	// Only custom pools inherit from worker; standard pools use default directly
+	if !IsCustomPool(pool) {
+		return "", nil
+	}
+
+	// For custom pools with no explicit stream, try to inherit from worker
+	workerPool, err := mcpLister.Get(MachineConfigPoolWorker)
+	if err != nil {
+		// If worker pool doesn't exist or can't be fetched, use default stream
+		if !kerr.IsNotFound(err) {
+			return "", fmt.Errorf("failed to get worker pool for osImageStream inheritance: %w", err)
+		}
+		return "", nil
+	}
+
+	// Return worker pool's stream (which may be empty, indicating default)
+	return workerPool.Spec.OSImageStream.Name, nil
+}

pkg/controller/common/helpers_test.go

@@ -20,6 +20,8 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 
 	mcfgv1 "github.com/openshift/api/machineconfiguration/v1"
+	"github.com/openshift/client-go/machineconfiguration/clientset/versioned/fake"
+	informers "github.com/openshift/client-go/machineconfiguration/informers/externalversions"
 	"github.com/openshift/machine-config-operator/pkg/controller/common/fixtures"
 	"github.com/openshift/machine-config-operator/test/helpers"
 )
@@ -1830,3 +1832,92 @@ func assertExpectedNodes(t *testing.T, expected []string, actual []*corev1.Node)
 	t.Helper()
 	assert.Equal(t, expected, helpers.GetNamesFromNodes(actual))
 }
+
+func TestGetEffectiveOSImageStreamName(t *testing.T) {
+	tests := []struct {
+		name           string
+		pool           *mcfgv1.MachineConfigPool
+		workerPool     *mcfgv1.MachineConfigPool
+		expectedStream string
+		expectError    bool
+	}{
+		{
+			name: "custom pool with explicit stream",
+			pool: func() *mcfgv1.MachineConfigPool {
+				p := helpers.NewMachineConfigPool("infra", helpers.InfraSelector, nil, "")
+				p.Spec.OSImageStream.Name = "rhel-10"
+				return p
+			}(),
+			workerPool: func() *mcfgv1.MachineConfigPool {
+				p := helpers.NewMachineConfigPool("worker", helpers.WorkerSelector, nil, "")
+				p.Spec.OSImageStream.Name = "rhel-9"
+				return p
+			}(),
+			expectedStream: "rhel-10",
+			expectError:    false,
+		},
+		{
+			name: "custom pool inherits from worker",
+			pool: helpers.NewMachineConfigPool("infra", helpers.InfraSelector, nil, ""),
+			workerPool: func() *mcfgv1.MachineConfigPool {
+				p := helpers.NewMachineConfigPool("worker", helpers.WorkerSelector, nil, "")
+				p.Spec.OSImageStream.Name = "rhel-9"
+				return p
+			}(),
+			expectedStream: "rhel-9",
+			expectError:    false,
+		},
+		{
+			name:           "custom pool without worker pool uses default",
+			pool:           helpers.NewMachineConfigPool("infra", helpers.InfraSelector, nil, ""),
+			workerPool:     nil,
+			expectedStream: "",
+			expectError:    false,
+		},
+		{
+			name:           "standard pool (worker) uses default",
+			pool:           helpers.NewMachineConfigPool("worker", helpers.WorkerSelector, nil, ""),
+			workerPool:     nil,
+			expectedStream: "",
+			expectError:    false,
+		},
+		{
+			name:           "standard pool (master) uses default",
+			pool:           helpers.NewMachineConfigPool("master", helpers.MasterSelector, nil, ""),
+			workerPool:     nil,
+			expectedStream: "",
+			expectError:    false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Create fake client and informer
+			objects := []runtime.Object{}
+			if tt.workerPool != nil {
+				objects = append(objects, tt.workerPool)
+			}
+
+			fakeClient := fake.NewSimpleClientset(objects...)
+			informerFactory := informers.NewSharedInformerFactory(fakeClient, 0)
+			mcpLister := informerFactory.Machineconfiguration().V1().MachineConfigPools().Lister()
+
+			// Start informer and wait for cache sync
+			stopCh := make(chan struct{})
+			defer close(stopCh)
+			informerFactory.Start(stopCh)
+			informerFactory.WaitForCacheSync(stopCh)
+
+			// Call the function
+			streamName, err := GetEffectiveOSImageStreamName(tt.pool, mcpLister)
+
+			// Check error
+			if tt.expectError {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, tt.expectedStream, streamName)
+			}
+		})
+	}
+}

pkg/controller/render/render_controller.go

@@ -192,6 +192,14 @@ func (ctrl *Controller) updateMachineConfigPool(old, cur interface{}) {
 
 	klog.V(4).Infof("Updating MachineConfigPool %s", oldPool.Name)
 	ctrl.enqueueMachineConfigPool(curPool)
+
+	// If the worker pool's osImageStream changed, enqueue all custom pools that might inherit from it
+	if curPool.Name == ctrlcommon.MachineConfigPoolWorker &&
+		oldPool.Spec.OSImageStream.Name != curPool.Spec.OSImageStream.Name {
+		klog.V(4).Infof("Worker pool osImageStream changed from %q to %q, enqueuing custom pools",
+			oldPool.Spec.OSImageStream.Name, curPool.Spec.OSImageStream.Name)
+		ctrl.enqueueCustomPools()
+	}
 }
 
 func (ctrl *Controller) deleteMachineConfigPool(obj interface{}) {
@@ -398,6 +406,23 @@ func (ctrl *Controller) enqueueDefault(pool *mcfgv1.MachineConfigPool) {
 	ctrl.enqueueAfter(pool, renderDelay)
 }
 
+// enqueueCustomPools enqueues all custom pools (pools that are not master, worker, or arbiter).
+// This is used when the worker pool changes in a way that might affect custom pools that inherit from it.
+func (ctrl *Controller) enqueueCustomPools() {
+	pools, err := ctrl.mcpLister.List(labels.Everything())
+	if err != nil {
+		klog.Warningf("Failed to list pools when enqueuing custom pools: %v", err)
+		return
+	}
+
+	for _, pool := range pools {
+		if ctrlcommon.IsCustomPool(pool) {
+			klog.V(4).Infof("Enqueuing custom pool %s due to worker pool change", pool.Name)
+			ctrl.enqueueMachineConfigPool(pool)
+		}
+	}
+}
+
 // worker runs a worker thread that just dequeues items, processes them, and marks them done.
 // It enforces that the syncHandler is never invoked concurrently with the same key.
 func (ctrl *Controller) worker() {
@@ -565,17 +590,51 @@ func (ctrl *Controller) getRenderedMachineConfig(pool *mcfgv1.MachineConfigPool,
 	return generateAndValidateRenderedMachineConfig(currentMC, pool, configs, cc, &mcop.Spec.IrreconcilableValidationOverrides, osImageStreamSet)
 }
 
+// getOSImageStreamNameForPool determines the OS image stream name for a pool,
+// implementing inheritance from the worker pool for custom pools.
+// Returns:
+// - The pool's explicit osImageStream.Name if set
+// - For custom pools: the worker pool's osImageStream.Name if the custom pool has none
+// - Empty string (which will resolve to the default stream)
+func (ctrl *Controller) getOSImageStreamNameForPool(pool *mcfgv1.MachineConfigPool) string {
+	streamName, err := ctrlcommon.GetEffectiveOSImageStreamName(pool, ctrl.mcpLister)
+	if err != nil {
+		klog.V(2).Infof("Failed to get effective osImageStream for pool %s: %v; using default stream", pool.Name, err)
+		return ""
+	}
+
+	// Log what stream is being used
+	if streamName != "" {
+		if pool.Spec.OSImageStream.Name != "" {
+			klog.V(4).Infof("Pool %s using explicit osImageStream: %s", pool.Name, streamName)
+		} else {
+			klog.V(4).Infof("Custom pool %s inheriting osImageStream from worker: %s", pool.Name, streamName)
+		}
+	} else {
+		if ctrlcommon.IsCustomPool(pool) {
+			klog.V(4).Infof("Custom pool %s using default osImageStream (worker also uses default)", pool.Name)
+		} else {
+			klog.V(4).Infof("Standard pool %s using default osImageStream", pool.Name)
+		}
+	}
+
+	return streamName
+}
+
 func (ctrl *Controller) getOSImageStreamForPool(pool *mcfgv1.MachineConfigPool) (*v1alpha1.OSImageStreamSet, error) {
 	if !osimagestream.IsFeatureEnabled(ctrl.fgHandler) || ctrl.osImageStreamLister == nil {
 		return nil, nil
 	}
 
+	// Determine which stream name to use based on inheritance logic
+	streamName := ctrl.getOSImageStreamNameForPool(pool)
+
 	imageStream, err := ctrl.osImageStreamLister.Get(ctrlcommon.ClusterInstanceNameOSImageStream)
 	if err != nil {
 		return nil, fmt.Errorf("could not get OSImageStream for pool %s: %w", pool.Name, err)
 	}
 
-	imageStreamSet, err := osimagestream.GetOSImageStreamSetByName(imageStream, pool.Spec.OSImageStream.Name)
+	imageStreamSet, err := osimagestream.GetOSImageStreamSetByName(imageStream, streamName)
 	if err != nil {
 		return nil, fmt.Errorf("could not get OSImageStreamSet for pool %s: %w", pool.Name, err)
 	}
@@ -778,6 +837,31 @@ func generateAndValidateRenderedMachineConfig(
 	return generated, nil
 }
 
+// getOSImageStreamNameForPoolBootstrap implements the same inheritance logic as
+// getOSImageStreamNameForPool but for the bootstrap scenario where we only have
+// a slice of pools, not a lister.
+func getOSImageStreamNameForPoolBootstrap(pool *mcfgv1.MachineConfigPool, pools []*mcfgv1.MachineConfigPool) string {
+	// If the pool explicitly sets an osImageStream, use it
+	if pool.Spec.OSImageStream.Name != "" {
+		return pool.Spec.OSImageStream.Name
+	}
+
+	// Only custom pools inherit from worker
+	if !ctrlcommon.IsCustomPool(pool) {
+		return ""
+	}
+
+	// Find worker pool in the provided slice
+	for _, p := range pools {
+		if p.Name == ctrlcommon.MachineConfigPoolWorker {
+			return p.Spec.OSImageStream.Name
+		}
+	}
+
+	// Worker not found, use default
+	return ""
+}
+
 // RunBootstrap runs the render controller in bootstrap mode.
 // For each pool, it matches the machineconfigs based on label selector and
 // returns the generated machineconfigs and pool with CurrentMachineConfig status field set.
@@ -793,7 +877,9 @@ func RunBootstrap(pools []*mcfgv1.MachineConfigPool, configs []*mcfgv1.MachineCo
 		}
 		var osImageStreamSet *v1alpha1.OSImageStreamSet
 		if osImageStream != nil {
-			osImageStreamSet, err = osimagestream.GetOSImageStreamSetByName(osImageStream, pool.Spec.OSImageStream.Name)
+			// Determine which stream name to use based on inheritance logic
+			streamName := getOSImageStreamNameForPoolBootstrap(pool, pools)
+			osImageStreamSet, err = osimagestream.GetOSImageStreamSetByName(osImageStream, streamName)
 			if err != nil {
 				return nil, nil, fmt.Errorf("couldn't get the OSImageStream for pool %s %w", pool.Name, err)
 			}