diff --git a/pkg/controller/common/helpers.go b/pkg/controller/common/helpers.go index 54bb1a4305..247b158210 100644 --- a/pkg/controller/common/helpers.go +++ b/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 +} diff --git a/pkg/controller/common/helpers_test.go b/pkg/controller/common/helpers_test.go index 6bb48c80a2..797fb19669 100644 --- a/pkg/controller/common/helpers_test.go +++ b/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) + } + }) + } +} diff --git a/pkg/controller/render/render_controller.go b/pkg/controller/render/render_controller.go index 1d78e369ea..681e73402f 100644 --- a/pkg/controller/render/render_controller.go +++ b/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) } diff --git a/pkg/controller/render/render_controller_test.go b/pkg/controller/render/render_controller_test.go index cc8f96b3ba..cc8277ca82 100644 --- a/pkg/controller/render/render_controller_test.go +++ b/pkg/controller/render/render_controller_test.go @@ -613,3 +613,298 @@ func TestGenerateMachineConfigValidation(t *testing.T) { assert.Error(t, err) assert.Nil(t, gmc) } + +func TestIsCustomPool(t *testing.T) { + tests := []struct { + name string + poolName string + expected bool + }{ + { + name: "master pool is not custom", + poolName: "master", + expected: false, + }, + { + name: "worker pool is not custom", + poolName: "worker", + expected: false, + }, + { + name: "arbiter pool is not custom", + poolName: "arbiter", + expected: false, + }, + { + name: "infra pool is custom", + poolName: "infra", + expected: true, + }, + { + name: "custom-role pool is custom", + poolName: "custom-role", + expected: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pool := helpers.NewMachineConfigPool(tt.poolName, helpers.WorkerSelector, nil, "") + result := ctrlcommon.IsCustomPool(pool) + assert.Equal(t, tt.expected, result) + }) + } +} + +func TestGetOSImageStreamNameForPool(t *testing.T) { + tests := []struct { + name string + pool *mcfgv1.MachineConfigPool + workerPool *mcfgv1.MachineConfigPool + expectedStream string + }{ + { + name: "custom pool with explicit stream uses 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", + }, + { + name: "custom pool without stream 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", + }, + { + name: "custom pool without stream, worker has no stream, uses default", + pool: helpers.NewMachineConfigPool("infra", helpers.InfraSelector, nil, ""), + workerPool: helpers.NewMachineConfigPool("worker", helpers.WorkerSelector, nil, ""), + expectedStream: "", + }, + { + name: "custom pool without stream, worker doesn't exist, uses default", + pool: helpers.NewMachineConfigPool("infra", helpers.InfraSelector, nil, ""), + workerPool: nil, + expectedStream: "", + }, + { + name: "standard pool (master) with explicit stream uses explicit stream", + pool: func() *mcfgv1.MachineConfigPool { + p := helpers.NewMachineConfigPool("master", helpers.MasterSelector, 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", + }, + { + name: "standard pool (worker) without stream uses default", + pool: helpers.NewMachineConfigPool("worker", helpers.WorkerSelector, nil, ""), + workerPool: nil, + expectedStream: "", + }, + { + name: "standard pool (arbiter) without stream uses default", + pool: helpers.NewMachineConfigPool("arbiter", &metav1.LabelSelector{ + MatchLabels: map[string]string{"node-role/arbiter": ""}, + }, nil, ""), + workerPool: nil, + expectedStream: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := newFixture(t) + + // Add worker pool to lister if provided + if tt.workerPool != nil { + f.mcpLister = append(f.mcpLister, tt.workerPool) + f.objects = append(f.objects, tt.workerPool) + } + + c := f.newController() + + result := c.getOSImageStreamNameForPool(tt.pool) + assert.Equal(t, tt.expectedStream, result, "Expected stream name %q, got %q", tt.expectedStream, result) + }) + } +} + +func TestGetOSImageStreamNameForPoolBootstrap(t *testing.T) { + tests := []struct { + name string + pool *mcfgv1.MachineConfigPool + pools []*mcfgv1.MachineConfigPool + expectedStream string + }{ + { + name: "custom pool with explicit stream uses explicit stream", + pool: func() *mcfgv1.MachineConfigPool { + p := helpers.NewMachineConfigPool("infra", helpers.InfraSelector, nil, "") + p.Spec.OSImageStream.Name = "rhel-10" + return p + }(), + pools: []*mcfgv1.MachineConfigPool{ + func() *mcfgv1.MachineConfigPool { + p := helpers.NewMachineConfigPool("worker", helpers.WorkerSelector, nil, "") + p.Spec.OSImageStream.Name = "rhel-9" + return p + }(), + }, + expectedStream: "rhel-10", + }, + { + name: "custom pool without stream inherits from worker", + pool: helpers.NewMachineConfigPool("infra", helpers.InfraSelector, nil, ""), + pools: []*mcfgv1.MachineConfigPool{ + helpers.NewMachineConfigPool("master", helpers.MasterSelector, nil, ""), + func() *mcfgv1.MachineConfigPool { + p := helpers.NewMachineConfigPool("worker", helpers.WorkerSelector, nil, "") + p.Spec.OSImageStream.Name = "rhel-9" + return p + }(), + }, + expectedStream: "rhel-9", + }, + { + name: "custom pool without stream, worker has no stream, uses default", + pool: helpers.NewMachineConfigPool("infra", helpers.InfraSelector, nil, ""), + pools: []*mcfgv1.MachineConfigPool{ + helpers.NewMachineConfigPool("worker", helpers.WorkerSelector, nil, ""), + }, + expectedStream: "", + }, + { + name: "custom pool without stream, no worker pool exists, uses default", + pool: helpers.NewMachineConfigPool("infra", helpers.InfraSelector, nil, ""), + pools: []*mcfgv1.MachineConfigPool{ + helpers.NewMachineConfigPool("master", helpers.MasterSelector, nil, ""), + }, + expectedStream: "", + }, + { + name: "standard pool (master) with explicit stream uses explicit stream", + pool: func() *mcfgv1.MachineConfigPool { + p := helpers.NewMachineConfigPool("master", helpers.MasterSelector, nil, "") + p.Spec.OSImageStream.Name = "rhel-10" + return p + }(), + pools: []*mcfgv1.MachineConfigPool{ + func() *mcfgv1.MachineConfigPool { + p := helpers.NewMachineConfigPool("worker", helpers.WorkerSelector, nil, "") + p.Spec.OSImageStream.Name = "rhel-9" + return p + }(), + }, + expectedStream: "rhel-10", + }, + { + name: "standard pool (worker) without stream uses default", + pool: helpers.NewMachineConfigPool("worker", helpers.WorkerSelector, nil, ""), + pools: []*mcfgv1.MachineConfigPool{ + helpers.NewMachineConfigPool("master", helpers.MasterSelector, nil, ""), + }, + expectedStream: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := getOSImageStreamNameForPoolBootstrap(tt.pool, tt.pools) + assert.Equal(t, tt.expectedStream, result, "Expected stream name %q, got %q", tt.expectedStream, result) + }) + } +} + +func TestWorkerPoolOSImageStreamChangeEnqueuesCustomPools(t *testing.T) { + f := newFixture(t) + + // Create worker pool with osImageStream + workerPool := helpers.NewMachineConfigPool("worker", helpers.WorkerSelector, nil, "") + workerPool.Spec.OSImageStream.Name = "rhel-9" + + // Create custom pools + infraPool := helpers.NewMachineConfigPool("infra", helpers.InfraSelector, nil, "") + customPool := helpers.NewMachineConfigPool("custom", helpers.InfraSelector, nil, "") + + // Add pools to lister + f.mcpLister = append(f.mcpLister, workerPool, infraPool, customPool) + f.objects = append(f.objects, workerPool, infraPool, customPool) + + c := f.newController() + + // Track which pools get enqueued + enqueuedPools := []string{} + c.enqueueMachineConfigPool = func(pool *mcfgv1.MachineConfigPool) { + enqueuedPools = append(enqueuedPools, pool.Name) + } + + // Update worker pool with new osImageStream + oldWorkerPool := workerPool.DeepCopy() + newWorkerPool := workerPool.DeepCopy() + newWorkerPool.Spec.OSImageStream.Name = "rhel-10" + + c.updateMachineConfigPool(oldWorkerPool, newWorkerPool) + + // Verify worker pool was enqueued (normal behavior) + assert.Contains(t, enqueuedPools, "worker", "Worker pool should be enqueued") + + // Verify custom pools were also enqueued + assert.Contains(t, enqueuedPools, "infra", "Infra pool should be enqueued when worker osImageStream changes") + assert.Contains(t, enqueuedPools, "custom", "Custom pool should be enqueued when worker osImageStream changes") + + // Verify master pool is NOT enqueued (we only have worker and custom pools) + assert.NotContains(t, enqueuedPools, "master", "Master pool should not be enqueued") +} + +func TestWorkerPoolOtherChangeDoesNotEnqueueCustomPools(t *testing.T) { + f := newFixture(t) + + // Create worker pool + workerPool := helpers.NewMachineConfigPool("worker", helpers.WorkerSelector, nil, "") + workerPool.Spec.OSImageStream.Name = "rhel-9" + + // Create custom pool + infraPool := helpers.NewMachineConfigPool("infra", helpers.InfraSelector, nil, "") + + // Add pools to lister + f.mcpLister = append(f.mcpLister, workerPool, infraPool) + f.objects = append(f.objects, workerPool, infraPool) + + c := f.newController() + + // Track which pools get enqueued + enqueuedPools := []string{} + c.enqueueMachineConfigPool = func(pool *mcfgv1.MachineConfigPool) { + enqueuedPools = append(enqueuedPools, pool.Name) + } + + // Update worker pool with SAME osImageStream but different field (e.g., paused) + oldWorkerPool := workerPool.DeepCopy() + newWorkerPool := workerPool.DeepCopy() + newWorkerPool.Spec.Paused = true + + c.updateMachineConfigPool(oldWorkerPool, newWorkerPool) + + // Verify only worker pool was enqueued, NOT custom pools + assert.Contains(t, enqueuedPools, "worker", "Worker pool should be enqueued") + assert.NotContains(t, enqueuedPools, "infra", "Infra pool should NOT be enqueued when osImageStream unchanged") + assert.Len(t, enqueuedPools, 1, "Only worker pool should be enqueued") +} diff --git a/pkg/operator/sync.go b/pkg/operator/sync.go index 5fa418d28e..56526810ef 100644 --- a/pkg/operator/sync.go +++ b/pkg/operator/sync.go @@ -1789,7 +1789,12 @@ func (optr *Operator) syncRequiredMachineConfigPools(config *renderConfig, co *c _, hasRequiredPoolLabel := pool.Labels[requiredForUpgradeMachineConfigPoolLabelKey] if hasRequiredPoolLabel { - opURL, _, err := optr.getOsImageURLs(optr.namespace, pool.Spec.OSImageStream.Name) + streamName, err := ctrlcommon.GetEffectiveOSImageStreamName(pool, optr.mcpLister) + if err != nil { + klog.Errorf("Error getting effective osImageStream name for pool %s: %q", pool.Name, err) + return false, nil + } + opURL, _, err := optr.getOsImageURLs(optr.namespace, streamName) if err != nil { klog.Errorf("Error getting OS images: %q", err) return false, nil