MCO-1961: Integrate OSImageStream into MCP and sync logic #5476

pablintino · 2025-12-09T14:12:30Z

Moved earlier cause I need to check if the FeatureGateOSStreams is active at each controller instantiation.

pablintino · 2025-12-09T14:25:58Z

Same as https://github.com/openshift/machine-config-operator/pull/5476/files#r2602846144.

pablintino · 2025-12-09T14:26:41Z

Needed to be able to read ImageStreams in https://github.com/openshift/machine-config-operator/pull/5476/files#diff-ee4889f36bd8b3bb13f51dd15721cbc3f0eae1cf4d6bb8635dec6de843e78c4dR184-R194

yuqi-zhang · 2025-12-11T02:08:11Z

out of curiosity when would this be possible? I was under the impression each payload would only have one tagged with MCO?

It's just a defensive measure, in theory, we are only copying the payload ImageStream. I added the check to be 100% we load the one we expected to load in case a change in the installer makes another IS to sneak in.

pablintino · 2025-12-09T14:28:01Z

Try to read the Payload ImageStream the installer copies into our manifests dir. The content is used by the OSImageStream logic to fetch the CoreOS images, get the labels and group the images by stream label.

pablintino · 2025-12-09T14:30:11Z

If the feature is enabled we collect the streams and the bootstrap-time OSImageStream CR is created and passed to all the bootstrapping logic in each controller to properly generate the MCs pointing to the default stream and to make the CC urls point to it.
If the feature is not enabled nil is passed everywhere and the regular logic we use to have that uses the CC image urls is used instead.

yuqi-zhang · 2025-12-11T02:19:45Z

On the same vein as the comment below, this code is fine for setting the default from the stream, but the override logic just assumes any override is for both pools, since both rhel9 and rhel10 are still "workers". Wouldn't that cause an issue if someone provides a worker osImageURL since both rhel9 and rhel10 would be using it?

Hmm, although looking at the must-gather, we do have that by default in 00-master and 00-worker, but it seems that testing this does work with RHEL 10, so maybe I'm misunderstanding that somewhere?

pablintino · 2025-12-09T14:31:41Z

If an OSImageStream is given it takes precedence to the regular "cluster-wide image urls"

-Original file line number
+Diff line change
@@ Expand Up / @@ -73,6 +73,15 @@ func runStartCmd(_ *cobra.Command, _ []string) { @@
     		ctrlctx := ctrlcommon.CreateControllerContext(ctx, cb)
+    		// Early start the config informer because feature gate depends on it
+    		ctrlctx.ConfigInformerFactory.Start(ctrlctx.Stop)
+    		if fgErr := ctrlctx.FeatureGatesHandler.Connect(ctx); fgErr != nil {
+    			klog.Error(fmt.Errorf("failed to connect to feature gates %w", fgErr))
+    			runCancel()
+    			<-ctx.Done()
+    			return
+    		}
     		go ctrlcommon.StartMetricsListener(startOpts.promMetricsListenAddress, ctrlctx.Stop, ctrlcommon.RegisterMCCMetrics)
     		controllers := createControllers(ctrlctx)
@@ Expand Down Expand Up / @@ -111,10 +120,6 @@ func runStartCmd(_ *cobra.Command, _ []string) { @@
     		close(ctrlctx.InformersStarted)
-    		if fgErr := ctrlctx.FeatureGatesHandler.Connect(ctx); fgErr != nil {
-    			klog.Fatal(fmt.Errorf("failed to connect to feature gates %w", fgErr))
-    		}
     		if ctrlctx.FeatureGatesHandler.Enabled(features.FeatureGatePinnedImages) && ctrlctx.FeatureGatesHandler.Enabled(features.FeatureGateMachineConfigNodes) {
     			pinnedImageSet := pinnedimageset.New(
     				ctrlctx.InformerFactory.Machineconfiguration().V1().PinnedImageSets(),
@@ Expand Down Expand Up @@
     			ctx.InformerFactory.Machineconfiguration().V1().ContainerRuntimeConfigs(),
     			ctx.InformerFactory.Machineconfiguration().V1().KubeletConfigs(),
     			ctx.OperatorInformerFactory.Operator().V1().MachineConfigurations(),
+    			ctx.InformerFactory.Machineconfiguration().V1alpha1().OSImageStreams(),
     			ctx.ClientBuilder.KubeClientOrDie("render-controller"),
     			ctx.ClientBuilder.MachineConfigClientOrDie("render-controller"),
     			ctx.FeatureGatesHandler,
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -67,6 +67,12 @@ func runStartCmd(_ *cobra.Command, _ []string) { @@
     		go common.SignalHandler(runCancel)
     		ctrlctx := ctrlcommon.CreateControllerContext(ctx, cb)
+    		// Early start the config informer because feature gate depends on it
+    		ctrlctx.ConfigInformerFactory.Start(ctrlctx.Stop)
+    		if fgErr := ctrlctx.FeatureGatesHandler.Connect(ctx); fgErr != nil {
+    			klog.Fatal(fmt.Errorf("failed to connect to feature gates %w", fgErr))
+    		}
     		controller := operator.New(
     			ctrlcommon.MCONamespace, componentName,
     			startOpts.imagesFile,
@@ Expand Down Expand Up / @@ -107,6 +113,8 @@ func runStartCmd(_ *cobra.Command, _ []string) { @@
     			ctrlctx.ConfigInformerFactory.Config().V1().Nodes(),
     			ctrlctx.ConfigInformerFactory.Config().V1().APIServers(),
     			ctrlctx.NamespacedInformerFactory.Machineconfiguration().V1().MachineOSConfigs(),
+    			ctrlctx.ConfigInformerFactory.Config().V1().ClusterVersions(),
+    			ctrlctx.InformerFactory.Machineconfiguration().V1alpha1().OSImageStreams(),
     			ctrlctx,
     		)
@@ Expand All / @@ -124,10 +132,6 @@ func runStartCmd(_ *cobra.Command, _ []string) { @@
     		close(ctrlctx.InformersStarted)
-    		if fgErr := ctrlctx.FeatureGatesHandler.Connect(ctx); fgErr != nil {
-    			klog.Fatal(fmt.Errorf("failed to connect to feature gates %w", fgErr))
-    		}
     		go controller.Run(2, ctrlctx.Stop)
     		// wait here in this function until the context gets cancelled (which tells us whe were being shut down)
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -2,12 +2,15 @@ package bootstrap @@
     import (
     	"bytes"
+    	"context"
     	"errors"
     	"fmt"
     	"io"
     	"os"
     	"path/filepath"
+    	"time"
+    	"github.com/openshift/machine-config-operator/pkg/osimagestream"
     	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
     	corev1 "k8s.io/api/core/v1"
@@ Expand All / @@ -23,6 +26,7 @@ import ( @@
     	apicfgv1 "github.com/openshift/api/config/v1"
     	apicfgv1alpha1 "github.com/openshift/api/config/v1alpha1"
     	"github.com/openshift/api/features"
+    	imagev1 "github.com/openshift/api/image/v1"
     	mcfgv1 "github.com/openshift/api/machineconfiguration/v1"
     	mcfgv1alpha1 "github.com/openshift/api/machineconfiguration/v1alpha1"
     	apioperatorsv1alpha1 "github.com/openshift/api/operator/v1alpha1"
@@ Expand Down Expand Up / @@ -70,7 +74,7 @@ func (b *Bootstrap) Run(destDir string) error { @@
     		return err
     	}
-    	psraw, err := getPullSecretFromSecret(psfraw)
+    	pullSecret, err := getValidPullSecretFromBytes(psfraw)
     	if err != nil {
     		return err
     	}
@@ Expand All / @@ -82,8 +86,13 @@ func (b *Bootstrap) Run(destDir string) error { @@
     	apicfgv1.Install(scheme)
     	apicfgv1alpha1.Install(scheme)
     	corev1.AddToScheme(scheme)
+    	imagev1.AddToScheme(scheme)
     	codecFactory := serializer.NewCodecFactory(scheme)
-    	decoder := codecFactory.UniversalDecoder(mcfgv1.GroupVersion, apioperatorsv1alpha1.GroupVersion, apicfgv1.GroupVersion, apicfgv1alpha1.GroupVersion, corev1.SchemeGroupVersion, mcfgv1alpha1.GroupVersion)
+    	decoder := codecFactory.UniversalDecoder(
+    		mcfgv1.GroupVersion, apioperatorsv1alpha1.GroupVersion,
+    		apicfgv1.GroupVersion, apicfgv1alpha1.GroupVersion,
+    		corev1.SchemeGroupVersion, mcfgv1alpha1.GroupVersion,
+    		imagev1.SchemeGroupVersion)
     	var (
     		cconfig              *mcfgv1.ControllerConfig
@@ Expand All / @@ -101,6 +110,7 @@ func (b *Bootstrap) Run(destDir string) error { @@
     		imagePolicies        []*apicfgv1.ImagePolicy
     		imgCfg               *apicfgv1.Image
     		apiServer            *apicfgv1.APIServer
+    		imageStream          *imagev1.ImageStream
     		iri                  *mcfgv1alpha1.InternalReleaseImage
     		iriTLSCert           *corev1.Secret
     	)
@@ Expand Down Expand Up / @@ -172,6 +182,17 @@ func (b *Bootstrap) Run(destDir string) error { @@
     				if obj.GetName() == ctrlcommon.InternalReleaseImageInstanceName {
     					iri = obj
     				}
+    			case *imagev1.ImageStream:
+    				for _, tag := range obj.Spec.Tags {
+    					if tag.Name == "machine-config-operator" {
+    						if imageStream != nil {
+    							klog.Infof("multiple ImageStream found. Previous ImageStream %s replaced by %s", imageStream.Name, obj.Name)
+    						}
+    						imageStream = obj
+    					}
+    				}
+    				// It's an ImageStream that doesn't look like the Release one (doesn't have our tag)
     			case *corev1.Secret:
     				if obj.GetName() == ctrlcommon.InternalReleaseImageTLSSecretName {
     					iriTLSCert = obj
@@ Expand All / @@ -196,7 +217,40 @@ func (b *Bootstrap) Run(destDir string) error { @@
     		return fmt.Errorf("error creating feature gates handler: %w", err)
     	}
-    	iconfigs, err := template.RunBootstrap(b.templatesDir, cconfig, psraw, apiServer)
+    	var osImageStream *mcfgv1alpha1.OSImageStream
+    	// Enable OSImageStreams if the FeatureGate is active and the deployment is not OKD
+    	if osimagestream.IsFeatureEnabled(fgHandler) {
+    		ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
+    		defer cancel()
+    		osImageStream, err = osimagestream.BuildOsImageStreamBootstrap(ctx,
+    			pullSecret,
+    			cconfig,
+    			imageStream,
+    			&osimagestream.OSImageTuple{
+    				OSImage:           cconfig.Spec.BaseOSContainerImage,
+    				OSExtensionsImage: cconfig.Spec.BaseOSExtensionsContainerImage,
+    			},
+    			osimagestream.NewDefaultStreamSourceFactory(nil, &osimagestream.DefaultImagesInspectorFactory{}),
+    		)
+    		if err != nil {
+    			return fmt.Errorf("error inspecting available OSImageStreams: %w", err)
+    		}
+    		// If no error happened override the ControllerConfig URLs with the default stream ones
+    		if err == nil {
+    			defaultStreamSet, err := osimagestream.GetOSImageStreamSetByName(osImageStream, "")
+    			if err != nil {
+    				// Should never happen
+    				return fmt.Errorf("error getting default OSImageStreamSet: %w", err)
+    			}
+    			cconfig.Spec.BaseOSContainerImage = string(defaultStreamSet.OSImage)
+    			cconfig.Spec.BaseOSExtensionsContainerImage = string(defaultStreamSet.OSExtensionsImage)
+    		}
+    	}
+    	pullSecretBytes := pullSecret.Data[corev1.DockerConfigJsonKey]
+    	iconfigs, err := template.RunBootstrap(b.templatesDir, cconfig, pullSecretBytes, apiServer)
     	if err != nil {
     		return err
     	}
@@ Expand Down Expand Up / @@ -279,7 +333,7 @@ func (b *Bootstrap) Run(destDir string) error { @@
     		klog.Infof("Successfully created %d pre-built image component MachineConfigs for hybrid OCL.", len(preBuiltImageMCs))
     	}
-    	fpools, gconfigs, err := render.RunBootstrap(pools, configs, cconfig)
+    	fpools, gconfigs, err := render.RunBootstrap(pools, configs, cconfig, osImageStream)
     	if err != nil {
     		return err
     	}
@@ Expand Down Expand Up / @@ -362,7 +416,7 @@ func (b *Bootstrap) Run(destDir string) error { @@
     }
-    func getPullSecretFromSecret(sData []byte) ([]byte, error) {
+    func getValidPullSecretFromBytes(sData []byte) (*corev1.Secret, error) {
     	obji, err := runtime.Decode(kscheme.Codecs.UniversalDecoder(corev1.SchemeGroupVersion), sData)
     	if err != nil {
     		return nil, err
@@ Expand All @@
     	if s.Type != corev1.SecretTypeDockerConfigJson {
     		return nil, fmt.Errorf("expected secret type %s found %s", corev1.SecretTypeDockerConfigJson, s.Type)
     	}
-    	return s.Data[corev1.DockerConfigJsonKey], nil
+    	return s, nil
     }
     type manifest struct {
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -28,6 +28,7 @@ import ( @@
     	ign2types "github.com/coreos/ignition/config/v2_2/types"
     	validate2 "github.com/coreos/ignition/config/validate"
     	ign3error "github.com/coreos/ignition/v2/config/shared/errors"
+    	"github.com/openshift/api/machineconfiguration/v1alpha1"
     	ign3 "github.com/coreos/ignition/v2/config/v3_5"
     	ign3types "github.com/coreos/ignition/v2/config/v3_5/types"
@@ Expand Down Expand Up / @@ -68,7 +69,7 @@ func boolToPtr(b bool) *bool { @@
     // It uses the Ignition config from first object as base and appends all the rest.
     // Kernel arguments are concatenated.
     // It defaults to the OSImageURL provided by the CVO but allows a MC provided OSImageURL to take precedence.
-    func MergeMachineConfigs(configs []*mcfgv1.MachineConfig, cconfig *mcfgv1.ControllerConfig) (*mcfgv1.MachineConfig, error) {
+    func MergeMachineConfigs(configs []*mcfgv1.MachineConfig, cconfig *mcfgv1.ControllerConfig, imageStream *v1alpha1.OSImageStreamSet) (*mcfgv1.MachineConfig, error) {
     	if len(configs) == 0 {
     		return nil, nil
     	}
@@ Expand Down Expand Up @@
     	// For layering, we want to let the user override OSImageURL again
     	// The template configs always match what's in controllerconfig because they get rendered from there,
     	// so the only way we get an override here is if the user adds something different
-    	osImageURL := GetDefaultBaseImageContainer(&cconfig.Spec)
+    	osImageURL := GetBaseImageContainer(&cconfig.Spec, imageStream)
     	for _, cfg := range configs {
     		// Ignore generated MCs, only the rendered MC or a user provided MC can set this field
     		if cfg.Annotations[GeneratedByControllerVersionAnnotationKey] != "" {
@@ Expand All @@
     	}
     	// Allow overriding the extensions container
-    	baseOSExtensionsContainerImage := cconfig.Spec.BaseOSExtensionsContainerImage
+    	baseOSExtensionsContainerImage := GetBaseExtensionsImageContainer(&cconfig.Spec, imageStream)
     	for _, cfg := range configs {
     		// Ignore generated MCs, only the rendered MC or a user provided MC can set this field
     		if cfg.Annotations[GeneratedByControllerVersionAnnotationKey] != "" {
@@ Expand Down Expand Up @@
     	return nil, nil
     }
-    // GetDefaultBaseImageContainer returns the default bootable host base image.
-    func GetDefaultBaseImageContainer(cconfigspec *mcfgv1.ControllerConfigSpec) string {
-    	return cconfigspec.BaseOSContainerImage
+    // GetBaseImageContainer returns the default bootable host base image.
+    func GetBaseImageContainer(cconfigspec *mcfgv1.ControllerConfigSpec, imageStream *v1alpha1.OSImageStreamSet) string {
+    	if imageStream == nil {
+    		return cconfigspec.BaseOSContainerImage
+    	}
+    	return string(imageStream.OSImage)
+    }
+    // GetBaseExtensionsImageContainer returns the default bootable host base image.
+    func GetBaseExtensionsImageContainer(cconfigspec *mcfgv1.ControllerConfigSpec, imageStream *v1alpha1.OSImageStreamSet) string {
+    	if imageStream == nil {
+    		return cconfigspec.BaseOSExtensionsContainerImage
+    	}
+    	return string(imageStream.OSExtensionsImage)
     }
     // Configures common template FuncMaps used across all renderers.
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -383,7 +383,7 @@ func TestMergeMachineConfigs(t *testing.T) { @@
     		},
     	}
     	inMachineConfigs := []*mcfgv1.MachineConfig{machineConfigFIPS}
-    	mergedMachineConfig, err := MergeMachineConfigs(inMachineConfigs, cconfig)
+    	mergedMachineConfig, err := MergeMachineConfigs(inMachineConfigs, cconfig, nil)
     	require.Nil(t, err)
     	// check that the outgoing config does have the version string set,
@@ Expand All / @@ -397,7 +397,7 @@ func TestMergeMachineConfigs(t *testing.T) { @@
     	require.Nil(t, err)
     	expectedMachineConfig := &mcfgv1.MachineConfig{
     		Spec: mcfgv1.MachineConfigSpec{
-    			OSImageURL:      GetDefaultBaseImageContainer(&cconfig.Spec),
+    			OSImageURL:      GetBaseImageContainer(&cconfig.Spec, nil),
     			KernelArguments: []string{},
     			Config: runtime.RawExtension{
     				Raw: rawOutIgn,
@@ Expand Down Expand Up / @@ -504,7 +504,7 @@ func TestMergeMachineConfigs(t *testing.T) { @@
     		machineConfigIgnV2Merge,
     	}
-    	mergedMachineConfig, err = MergeMachineConfigs(inMachineConfigs, cconfig)
+    	mergedMachineConfig, err = MergeMachineConfigs(inMachineConfigs, cconfig, nil)
     	require.Nil(t, err)
     	expectedMachineConfig = &mcfgv1.MachineConfig{
@@ Expand Down Expand Up / @@ -588,7 +588,7 @@ func TestMergeMachineConfigs(t *testing.T) { @@
     	}
     	cconfig = &mcfgv1.ControllerConfig{}
-    	mergedMachineConfig, err = MergeMachineConfigs(inMachineConfigs, cconfig)
+    	mergedMachineConfig, err = MergeMachineConfigs(inMachineConfigs, cconfig, nil)
     	require.Nil(t, err)
     	// The expectation here is that the merged config contains the MCs with name bbb (overrides aaa due to name) and ccc (overrides ddd due to pool)
@@ Expand Down Expand Up / @@ -795,7 +795,7 @@ func TestSetDefaultFileOverwrite(t *testing.T) { @@
     	require.Nil(t, err)
     	cconfig := &mcfgv1.ControllerConfig{}
-    	mergedMachineConfig, err := MergeMachineConfigs([]*mcfgv1.MachineConfig{machineConfigPreMerge}, cconfig)
+    	mergedMachineConfig, err := MergeMachineConfigs([]*mcfgv1.MachineConfig{machineConfigPreMerge}, cconfig, nil)
     	require.Nil(t, err)
     	// Convert and create the expected post-merge config
@@ Expand Down @@

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

MCO-1961: Integrate OSImageStream into MCP and sync logic #5476

Uh oh!

Diff view

Diff view

There are no files selected for viewing

pablintino Dec 9, 2025

Uh oh!

pablintino Dec 9, 2025

Uh oh!

pablintino Dec 9, 2025

Uh oh!

yuqi-zhang Dec 11, 2025

Uh oh!

pablintino Dec 11, 2025

Uh oh!

pablintino Dec 9, 2025

Uh oh!

pablintino Dec 9, 2025

Uh oh!

yuqi-zhang Dec 11, 2025

Uh oh!

pablintino Dec 9, 2025

Uh oh!

Uh oh!

-Original file line number
+Diff line change
@@ Expand Up / @@ -5,6 +5,7 @@ import ( @@
     	"fmt"
     	"strings"
+    	"github.com/openshift/machine-config-operator/pkg/osimagestream"
     	corev1 "k8s.io/api/core/v1"
     	"k8s.io/apimachinery/pkg/api/equality"
     	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ Expand Down Expand Up @@
     		unavailableMachineCount == 0 &&
     		!isLayeredPoolBuilding(isLayeredPool, mosc, mosb)
     	if allUpdated {
+    		// When the pool is fully updated & the `OSStreams` FeatureGate is enabled, set the
+    		// `OSImageStream` reference in the MCP status to be consistent to what is defined in the
+    		// MCP spec
+    		if osimagestream.IsFeatureEnabled(ctrl.fgHandler) {
+    			status.OSImageStream = pool.Spec.OSImageStream
+    		}
     		//TODO: update api to only have one condition regarding status of update.
     		updatedMsg := fmt.Sprintf("All nodes are updated with %s", getPoolUpdateLine(pool, mosc, isLayeredPool))
     		supdated := apihelpers.NewMachineConfigPoolCondition(mcfgv1.MachineConfigPoolUpdated, corev1.ConditionTrue, "", updatedMsg)
@@ Expand Down @@

MCO-1961: Integrate OSImageStream into MCP and sync logic #5476

Uh oh!

MCO-1961: Integrate OSImageStream into MCP and sync logic #5476

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!