From 0cf18d594f46a990703cbeb45627ec0e7a165653 Mon Sep 17 00:00:00 2001 From: Pablo Rodriguez Nava Date: Thu, 11 Dec 2025 12:34:49 +0100 Subject: [PATCH 1/4] MCO-1961: Rework MC's OSImageURL merge logic Template-generated MachineConfigs (e.g., 01-master-kubelet, 01-master-container-runtime) were setting OSImageURL on the final rendered MachineConfig, even though only user-provided MachineConfigs should be able to override this field. The root cause was that template generation explicitly sets OSImageURL on all generated MCs, and the merge logic (MergeMachineConfigs) was treating all MCs equally when determining the final OSImageURL value. This meant template-generated MCs would always propagate the base OS image URL to the rendered MC, making it impossible for the system to distinguish between a default value and an intentional override. This commit fixes the issue by modifying MergeMachineConfigs to skip any MachineConfig with the machineconfiguration.openshift.io/generated-by-controller-version annotation when evaluating OSImageURL and BaseOSExtensionsContainerImage overrides. This ensures that only user-provided MachineConfigs can override these fields, while still allowing template-generated MCs to have the field populated (which is necessary due to resourcemerge not blanking out previously-set values during upgrades). The same logic is applied to BaseOSExtensionsContainerImage for consistency. --- pkg/controller/common/helpers.go | 8 ++++++++ pkg/controller/template/render.go | 12 +++++------- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/pkg/controller/common/helpers.go b/pkg/controller/common/helpers.go index c8ed0ac473..acac5425a1 100644 --- a/pkg/controller/common/helpers.go +++ b/pkg/controller/common/helpers.go @@ -189,6 +189,10 @@ func MergeMachineConfigs(configs []*mcfgv1.MachineConfig, cconfig *mcfgv1.Contro // so the only way we get an override here is if the user adds something different osImageURL := GetDefaultBaseImageContainer(&cconfig.Spec) for _, cfg := range configs { + // Ignore generated MCs, only the rendered MC or a user provided MC can set this field + if cfg.Annotations[GeneratedByControllerVersionAnnotationKey] != "" { + continue + } if cfg.Spec.OSImageURL != "" { osImageURL = cfg.Spec.OSImageURL } @@ -197,6 +201,10 @@ func MergeMachineConfigs(configs []*mcfgv1.MachineConfig, cconfig *mcfgv1.Contro // Allow overriding the extensions container baseOSExtensionsContainerImage := cconfig.Spec.BaseOSExtensionsContainerImage for _, cfg := range configs { + // Ignore generated MCs, only the rendered MC or a user provided MC can set this field + if cfg.Annotations[GeneratedByControllerVersionAnnotationKey] != "" { + continue + } if cfg.Spec.BaseOSExtensionsContainerImage != "" { baseOSExtensionsContainerImage = cfg.Spec.BaseOSExtensionsContainerImage } diff --git a/pkg/controller/template/render.go b/pkg/controller/template/render.go index 35268a8d01..fbb6b67395 100644 --- a/pkg/controller/template/render.go +++ b/pkg/controller/template/render.go @@ -361,13 +361,11 @@ func generateMachineConfigForName(config *RenderConfig, role, name, templateDir, mcfg.Spec.Extensions = append(mcfg.Spec.Extensions, slices.Sorted(maps.Keys(extensions))...) - // TODO(jkyros): you might think you can remove this since we override later when we merge - // config, but resourcemerge doesn't blank this field out once it's populated - // so if you end up on a cluster where it was ever populated in this machineconfig, it - // will keep that last value forever once you upgrade...which is a problen now that we allow OSImageURL overrides - // because it will look like an override when it shouldn't be. So don't take this out until you've solved that. - // And inject the osimageurl here - mcfg.Spec.OSImageURL = ctrlcommon.GetDefaultBaseImageContainer(config.ControllerConfigSpec) + // Note: Previously, the OSImageURL was set here (as well as in the rendered MC) to facilitate + // overrides. Now, all the OSImageURL's from generated MCs are ignored and only user provided + // MCs with the OSImageURL set are considered during the merge process. + // Now the image URL is explicitly cleared to avoid confusion. Its content is never consumed. + mcfg.Spec.OSImageURL = "" return mcfg, nil } From ddfaa03a8e69ac6a38e378533e67d5ff66514247 Mon Sep 17 00:00:00 2001 From: Pablo Rodriguez Nava Date: Fri, 28 Nov 2025 19:00:34 +0100 Subject: [PATCH 2/4] MCO-1961: Introduce the OSImageStream controller --- cmd/machine-config-controller/start.go | 14 +- cmd/machine-config-operator/start.go | 12 +- pkg/controller/bootstrap/bootstrap.go | 66 ++- pkg/controller/common/helpers.go | 24 +- pkg/controller/common/helpers_test.go | 10 +- .../osimagestream/osimagestream_controller.go | 206 --------- .../osimagestream_controller_test.go | 404 ------------------ pkg/controller/render/render_controller.go | 83 +++- .../render/render_controller_test.go | 35 +- pkg/controller/template/render.go | 6 +- pkg/imageutils/layer_reader.go | 8 +- pkg/operator/operator.go | 24 +- pkg/operator/sync.go | 297 ++++++++++--- .../osimagestream/clusterversion.go | 0 .../osimagestream/clusterversion_test.go | 0 pkg/{controller => }/osimagestream/helpers.go | 8 + .../osimagestream/helpers_test.go | 0 .../osimagestream/image_data.go | 0 .../osimagestream/image_data_test.go | 0 .../osimagestream/imagestream_provider.go | 0 .../imagestream_provider_test.go | 0 .../osimagestream/imagestream_source.go | 0 .../osimagestream/imagestream_source_test.go | 0 .../osimagestream/inspector.go | 46 +- .../osimagestream/mocks_test.go | 0 .../osimagestream/osimagestream.go | 3 +- .../osimagestream/osimagestream_test.go | 1 - .../osimagestream/urls_source.go | 0 .../osimagestream/urls_source_test.go | 0 test/e2e-2of2/imageutils_test.go | 2 +- test/e2e-2of2/osimagestream_test.go | 2 +- test/e2e-bootstrap/bootstrap_test.go | 6 + 32 files changed, 504 insertions(+), 753 deletions(-) delete mode 100644 pkg/controller/osimagestream/osimagestream_controller.go delete mode 100644 pkg/controller/osimagestream/osimagestream_controller_test.go rename pkg/{controller => }/osimagestream/clusterversion.go (100%) rename pkg/{controller => }/osimagestream/clusterversion_test.go (100%) rename pkg/{controller => }/osimagestream/helpers.go (84%) rename pkg/{controller => }/osimagestream/helpers_test.go (100%) rename pkg/{controller => }/osimagestream/image_data.go (100%) rename pkg/{controller => }/osimagestream/image_data_test.go (100%) rename pkg/{controller => }/osimagestream/imagestream_provider.go (100%) rename pkg/{controller => }/osimagestream/imagestream_provider_test.go (100%) rename pkg/{controller => }/osimagestream/imagestream_source.go (100%) rename pkg/{controller => }/osimagestream/imagestream_source_test.go (100%) rename pkg/{controller => }/osimagestream/inspector.go (65%) rename pkg/{controller => }/osimagestream/mocks_test.go (100%) rename pkg/{controller => }/osimagestream/osimagestream.go (98%) rename pkg/{controller => }/osimagestream/osimagestream_test.go (99%) rename pkg/{controller => }/osimagestream/urls_source.go (100%) rename pkg/{controller => }/osimagestream/urls_source_test.go (100%) diff --git a/cmd/machine-config-controller/start.go b/cmd/machine-config-controller/start.go index f22c4dfee0..cf26a206e6 100644 --- a/cmd/machine-config-controller/start.go +++ b/cmd/machine-config-controller/start.go @@ -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) @@ -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(), @@ -246,6 +251,7 @@ func createControllers(ctx *ctrlcommon.ControllerContext) []ctrlcommon.Controlle 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, diff --git a/cmd/machine-config-operator/start.go b/cmd/machine-config-operator/start.go index 22d7323dea..b9cbca3265 100644 --- a/cmd/machine-config-operator/start.go +++ b/cmd/machine-config-operator/start.go @@ -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, @@ -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, ) @@ -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) diff --git a/pkg/controller/bootstrap/bootstrap.go b/pkg/controller/bootstrap/bootstrap.go index b99ad49859..e31da52601 100644 --- a/pkg/controller/bootstrap/bootstrap.go +++ b/pkg/controller/bootstrap/bootstrap.go @@ -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" @@ -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" @@ -70,7 +74,7 @@ func (b *Bootstrap) Run(destDir string) error { return err } - psraw, err := getPullSecretFromSecret(psfraw) + pullSecret, err := getValidatePullSecretFromBytes(psfraw) if err != nil { return err } @@ -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 @@ -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 ) for _, info := range infos { @@ -171,6 +181,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) default: klog.Infof("skipping %q [%d] manifest because of unhandled %T", file.Name(), idx+1, obji) } @@ -191,7 +212,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 } @@ -274,7 +328,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 } @@ -357,7 +411,7 @@ func (b *Bootstrap) Run(destDir string) error { } -func getPullSecretFromSecret(sData []byte) ([]byte, error) { +func getValidatePullSecretFromBytes(sData []byte) (*corev1.Secret, error) { obji, err := runtime.Decode(kscheme.Codecs.UniversalDecoder(corev1.SchemeGroupVersion), sData) if err != nil { return nil, err @@ -369,7 +423,7 @@ func getPullSecretFromSecret(sData []byte) ([]byte, error) { 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 { diff --git a/pkg/controller/common/helpers.go b/pkg/controller/common/helpers.go index acac5425a1..1657c738fd 100644 --- a/pkg/controller/common/helpers.go +++ b/pkg/controller/common/helpers.go @@ -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" @@ -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 } @@ -187,7 +188,7 @@ func MergeMachineConfigs(configs []*mcfgv1.MachineConfig, cconfig *mcfgv1.Contro // 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] != "" { @@ -199,7 +200,7 @@ func MergeMachineConfigs(configs []*mcfgv1.MachineConfig, cconfig *mcfgv1.Contro } // 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] != "" { @@ -1043,9 +1044,20 @@ func GetIgnitionFileDataByPath(config *ign3types.Config, path string) ([]byte, e 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. diff --git a/pkg/controller/common/helpers_test.go b/pkg/controller/common/helpers_test.go index c1f2f9e103..a1ff947dff 100644 --- a/pkg/controller/common/helpers_test.go +++ b/pkg/controller/common/helpers_test.go @@ -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, @@ -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, @@ -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{ @@ -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) @@ -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 diff --git a/pkg/controller/osimagestream/osimagestream_controller.go b/pkg/controller/osimagestream/osimagestream_controller.go deleted file mode 100644 index abeeb9d530..0000000000 --- a/pkg/controller/osimagestream/osimagestream_controller.go +++ /dev/null @@ -1,206 +0,0 @@ -package osimagestream - -import ( - "context" - "fmt" - "time" - - mcfgv1 "github.com/openshift/api/machineconfiguration/v1" - "github.com/openshift/api/machineconfiguration/v1alpha1" - "github.com/openshift/machine-config-operator/pkg/version" - "k8s.io/apimachinery/pkg/api/errors" - corelisterv1 "k8s.io/client-go/listers/core/v1" - - configinformersv1 "github.com/openshift/client-go/config/informers/externalversions/config/v1" - configlisters "github.com/openshift/client-go/config/listers/config/v1" - mcfgclientset "github.com/openshift/client-go/machineconfiguration/clientset/versioned" - mcfginformersv1 "github.com/openshift/client-go/machineconfiguration/informers/externalversions/machineconfiguration/v1" - mcfginformersv1alpha1 "github.com/openshift/client-go/machineconfiguration/informers/externalversions/machineconfiguration/v1alpha1" - mcfglistersv1 "github.com/openshift/client-go/machineconfiguration/listers/machineconfiguration/v1" - mcfglistersv1alpha1 "github.com/openshift/client-go/machineconfiguration/listers/machineconfiguration/v1alpha1" - clientset "k8s.io/client-go/kubernetes" - - ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - - utilruntime "k8s.io/apimachinery/pkg/util/runtime" - - coreinformersv1 "k8s.io/client-go/informers/core/v1" - "k8s.io/client-go/tools/cache" - - "k8s.io/klog/v2" -) - -// Controller manages the OSImageStream singleton resource lifecycle. -type Controller struct { - client mcfgclientset.Interface - kubeClient clientset.Interface - - ccLister mcfglistersv1.ControllerConfigLister - clusterVersionLister configlisters.ClusterVersionLister - osImageStreamLister mcfglistersv1alpha1.OSImageStreamLister - cmLister corelisterv1.ConfigMapLister - - cachesToSync []cache.InformerSynced - bootedChan chan error - // osImageStream holds the OSImageStream resource after successful boot - osImageStream *v1alpha1.OSImageStream - - imageStreamFactory ImageStreamFactory -} - -// NewController creates a new OSImageStream controller. -func NewController( - kubeClient clientset.Interface, - mcfgClient mcfgclientset.Interface, - ccInformer mcfginformersv1.ControllerConfigInformer, - mcoCmInformer coreinformersv1.ConfigMapInformer, - osImageStreamInformer mcfginformersv1alpha1.OSImageStreamInformer, - clusterVersionInformer configinformersv1.ClusterVersionInformer, - imageStreamFactory ImageStreamFactory, -) *Controller { - ctrl := &Controller{ - client: mcfgClient, - kubeClient: kubeClient, - ccLister: ccInformer.Lister(), - clusterVersionLister: clusterVersionInformer.Lister(), - osImageStreamLister: osImageStreamInformer.Lister(), - cmLister: mcoCmInformer.Lister(), - cachesToSync: []cache.InformerSynced{ - ccInformer.Informer().HasSynced, - mcoCmInformer.Informer().HasSynced, - clusterVersionInformer.Informer().HasSynced, - osImageStreamInformer.Informer().HasSynced, - }, - bootedChan: make(chan error, 1), - imageStreamFactory: imageStreamFactory, - } - - // Default to the "full/real" implementation if not factory was provided - if imageStreamFactory == nil { - ctrl.imageStreamFactory = NewDefaultStreamSourceFactory(ctrl.cmLister, &DefaultImagesInspectorFactory{}) - } - - return ctrl -} - -// Run starts the controller and boots the OSImageStream resource. -func (ctrl *Controller) Run(stopCh <-chan struct{}) { - defer utilruntime.HandleCrash() - - if !cache.WaitForCacheSync(stopCh, ctrl.cachesToSync...) { - utilruntime.HandleError(fmt.Errorf("caches did not sync")) - return - } - - klog.Info("Starting MachineConfigController-OSImageStreamController") - defer klog.Info("Shutting down MachineConfigController-OSImageStreamController") - - go func() { - err := ctrl.boot() - if err != nil { - klog.Errorf("Error booting OSImageStreamController: %v", err) - } else { - klog.Infof( - "OSImageStreamController booted successfully. Available streams: %s. Default stream: %s", - GetStreamSetsNames(ctrl.osImageStream.Status.AvailableStreams), - ctrl.osImageStream.Status.DefaultStream, - ) - } - - ctrl.bootedChan <- err - }() - <-stopCh -} - -// WaitBoot blocks until the boot process completes and returns any error encountered. -func (ctrl *Controller) WaitBoot() error { - return <-ctrl.bootedChan -} - -// boot initializes or updates the OSImageStream resource. -// It checks if an update is needed, fetches release images, and creates or updates the resource accordingly. -func (ctrl *Controller) boot() error { - existingOSImageStream, err := ctrl.getExistingOSImageStream() - if err != nil { - return err - } - - if !osImageStreamRequiresUpdate(existingOSImageStream) { - klog.Info("Skipping OSImageStream boot: OSImageStream is already up-to-date") - ctrl.osImageStream = existingOSImageStream - return nil - } - - image, err := GetReleasePayloadImage(ctrl.clusterVersionLister) - if err != nil { - return fmt.Errorf("error getting the Release Image digest from the ClusterVersion for the initial OSImageStream load: %w", err) - } - - secret, cc, err := ctrl.getSysContextObjects() - if err != nil { - return fmt.Errorf("error getting the required dependencies for the initial OSImageStream load: %w", err) - } - - ctx, cancel := context.WithTimeout(context.Background(), 1*time.Minute) - defer cancel() - osImageStream, err := BuildOsImageStreamRuntime(ctx, secret, cc, image, ctrl.imageStreamFactory) - if err != nil { - return fmt.Errorf("error building the OSImageStream at runtime: %w", err) - } - - if existingOSImageStream == nil { - klog.V(4).Infof("Creating OSImageStream singleton instance as it doesn't exist") - if _, err = ctrl.client.MachineconfigurationV1alpha1().OSImageStreams().Create(context.TODO(), osImageStream, metav1.CreateOptions{}); err != nil { - return fmt.Errorf("error creating the OSImageStream at runtime: %w", err) - } - } else { - oldVersion := existingOSImageStream.Annotations[ctrlcommon.ReleaseImageVersionAnnotationKey] - klog.V(4).Infof("Updating the OSImageStream singleton as it was created by a previous version (%s). New version: %s", oldVersion, version.Hash) - if _, err = ctrl.client. - MachineconfigurationV1alpha1(). - OSImageStreams(). - UpdateStatus(context.TODO(), osImageStream, metav1.UpdateOptions{}); err != nil { - return fmt.Errorf("error updating the OSImageStream at runtime: %w", err) - } - } - ctrl.osImageStream = osImageStream - return nil -} - -// getExistingOSImageStream retrieves the existing OSImageStream from the lister. -// Returns nil if the OSImageStream does not exist. -func (ctrl *Controller) getExistingOSImageStream() (*v1alpha1.OSImageStream, error) { - osImageStream, err := ctrl.osImageStreamLister.Get(ctrlcommon.ClusterInstanceNameOSImageStream) - if err != nil { - if !errors.IsNotFound(err) { - return nil, fmt.Errorf("failed to retrieve existing OSImageStream: %v", err) - } - return nil, nil - } - return osImageStream, nil -} - -// osImageStreamRequiresUpdate checks if the OSImageStream needs to be created or updated. -// Returns true if osImageStream is nil or if its version annotation doesn't match the current version. -func osImageStreamRequiresUpdate(osImageStream *v1alpha1.OSImageStream) bool { - if osImageStream == nil { - return true - } - releaseVersion, ok := osImageStream.Annotations[ctrlcommon.ReleaseImageVersionAnnotationKey] - return !ok || releaseVersion != version.Hash -} - -func (ctrl *Controller) getSysContextObjects() (*corev1.Secret, *mcfgv1.ControllerConfig, error) { - cc, err := ctrl.ccLister.Get(ctrlcommon.ControllerConfigName) - if err != nil { - return nil, nil, fmt.Errorf("could not get ControllerConfig for OSImageStream initial load: %v", err) - } - - clusterPullSecret, err := ctrl.kubeClient.CoreV1().Secrets(cc.Spec.PullSecret.Namespace).Get(context.TODO(), cc.Spec.PullSecret.Name, metav1.GetOptions{}) - if err != nil { - return nil, nil, fmt.Errorf("could not get the cluster PullSecret for OSImageStream initial load: %v", err) - } - return clusterPullSecret, cc, nil -} diff --git a/pkg/controller/osimagestream/osimagestream_controller_test.go b/pkg/controller/osimagestream/osimagestream_controller_test.go deleted file mode 100644 index ddb6e3d274..0000000000 --- a/pkg/controller/osimagestream/osimagestream_controller_test.go +++ /dev/null @@ -1,404 +0,0 @@ -// Assisted-by: Claude -package osimagestream - -import ( - "context" - "testing" - "time" - - "github.com/containers/image/v5/types" - configv1 "github.com/openshift/api/config/v1" - imagev1 "github.com/openshift/api/image/v1" - mcfgv1 "github.com/openshift/api/machineconfiguration/v1" - "github.com/openshift/api/machineconfiguration/v1alpha1" - configfake "github.com/openshift/client-go/config/clientset/versioned/fake" - configinformers "github.com/openshift/client-go/config/informers/externalversions" - mcfgfake "github.com/openshift/client-go/machineconfiguration/clientset/versioned/fake" - mcfginformers "github.com/openshift/client-go/machineconfiguration/informers/externalversions" - ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common" - "github.com/openshift/machine-config-operator/pkg/version" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - kubeinformers "k8s.io/client-go/informers" - kubefake "k8s.io/client-go/kubernetes/fake" -) - -// mockImageStreamFactory is a test implementation of ImageStreamFactory -type mockImageStreamFactory struct { - runtimeStream *v1alpha1.OSImageStream - runtimeErr error - bootstrapStream *v1alpha1.OSImageStream - bootstrapErr error -} - -func (m *mockImageStreamFactory) CreateRuntimeSources(_ context.Context, _ string, _ *types.SystemContext) (*v1alpha1.OSImageStream, error) { - return m.runtimeStream, m.runtimeErr -} - -func (m *mockImageStreamFactory) CreateBootstrapSources(_ context.Context, _ *imagev1.ImageStream, _ *OSImageTuple, _ *types.SystemContext) (*v1alpha1.OSImageStream, error) { - return m.bootstrapStream, m.bootstrapErr -} - -func TestController_Run_BootSuccess(t *testing.T) { - // Create existing OSImageStream with current version (no update needed) - existingOSImageStream := &v1alpha1.OSImageStream{ - ObjectMeta: metav1.ObjectMeta{ - Name: ctrlcommon.ClusterInstanceNameOSImageStream, - Annotations: map[string]string{ - ctrlcommon.ReleaseImageVersionAnnotationKey: version.Hash, - ctrlcommon.GeneratedByControllerVersionAnnotationKey: version.Hash, - }, - }, - Status: v1alpha1.OSImageStreamStatus{ - DefaultStream: "rhel-9", - AvailableStreams: []v1alpha1.OSImageStreamSet{ - {Name: "rhel-9", OSImage: "image1", OSExtensionsImage: "ext1"}, - }, - }, - } - - // Create fake clients - mcfgObjs := []runtime.Object{existingOSImageStream} - fakeMcfgClient := mcfgfake.NewSimpleClientset(mcfgObjs...) - mcfgInformerFactory := mcfginformers.NewSharedInformerFactory(fakeMcfgClient, 0) - - configObjs := []runtime.Object{} - fakeConfigClient := configfake.NewSimpleClientset(configObjs...) - configInformerFactory := configinformers.NewSharedInformerFactory(fakeConfigClient, 0) - - fakeKubeClient := kubefake.NewSimpleClientset() - kubeInformerFactory := kubeinformers.NewSharedInformerFactory(fakeKubeClient, 0) - - // Setup informers - ccInformer := mcfgInformerFactory.Machineconfiguration().V1().ControllerConfigs() - cmInformer := kubeInformerFactory.Core().V1().ConfigMaps() - osImageStreamInformer := mcfgInformerFactory.Machineconfiguration().V1alpha1().OSImageStreams() - cvInformer := configInformerFactory.Config().V1().ClusterVersions() - - // Add objects to indexers - osImageStreamInformer.Informer().GetIndexer().Add(existingOSImageStream) - - // Mock factory (not used since no update is needed) - mockFactory := &mockImageStreamFactory{} - - // Create controller using constructor - ctrl := NewController( - fakeKubeClient, - fakeMcfgClient, - ccInformer, - cmInformer, - osImageStreamInformer, - cvInformer, - mockFactory, - ) - - // Start informers - stopCh := make(chan struct{}) - defer close(stopCh) - - mcfgInformerFactory.Start(stopCh) - configInformerFactory.Start(stopCh) - kubeInformerFactory.Start(stopCh) - - // Run controller in goroutine - go ctrl.Run(stopCh) - - // Wait for boot to complete using WaitBoot - done := make(chan error, 1) - go func() { - done <- ctrl.WaitBoot() - }() - - select { - case err := <-done: - require.NoError(t, err) - - // Verify the OSImageStream was not modified (remains as it was) - osImageStream, err := fakeMcfgClient.MachineconfigurationV1alpha1(). - OSImageStreams(). - Get(context.TODO(), ctrlcommon.ClusterInstanceNameOSImageStream, metav1.GetOptions{}) - require.NoError(t, err) - assert.Equal(t, existingOSImageStream, osImageStream) - case <-time.After(2 * time.Second): - t.Fatal("Boot did not complete in time") - } -} - -func TestController_Run_NoOSImageStream(t *testing.T) { - // No existing OSImageStream - controller should create one - - // New OSImageStream that will be returned by the mock factory and created - newOSImageStream := &v1alpha1.OSImageStream{ - ObjectMeta: metav1.ObjectMeta{ - Name: ctrlcommon.ClusterInstanceNameOSImageStream, - Annotations: map[string]string{ - ctrlcommon.ReleaseImageVersionAnnotationKey: version.Hash, - ctrlcommon.GeneratedByControllerVersionAnnotationKey: version.Hash, - }, - }, - Status: v1alpha1.OSImageStreamStatus{ - DefaultStream: "rhel-9", - AvailableStreams: []v1alpha1.OSImageStreamSet{ - {Name: "rhel-9", OSImage: "image1", OSExtensionsImage: "ext1"}, - }, - }, - } - - // Create fake clients - mcfgObjs := []runtime.Object{} - fakeMcfgClient := mcfgfake.NewSimpleClientset(mcfgObjs...) - mcfgInformerFactory := mcfginformers.NewSharedInformerFactory(fakeMcfgClient, 0) - - // Provide ClusterVersion - clusterVersion := &configv1.ClusterVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "version", - }, - Status: configv1.ClusterVersionStatus{ - Desired: configv1.Release{ - Image: "quay.io/openshift-release-dev/ocp-release@sha256:abc123", - }, - }, - } - configObjs := []runtime.Object{clusterVersion} - fakeConfigClient := configfake.NewSimpleClientset(configObjs...) - configInformerFactory := configinformers.NewSharedInformerFactory(fakeConfigClient, 0) - - // Provide ControllerConfig with PullSecret - controllerConfig := &mcfgv1.ControllerConfig{ - ObjectMeta: metav1.ObjectMeta{ - Name: ctrlcommon.ControllerConfigName, - }, - Spec: mcfgv1.ControllerConfigSpec{ - PullSecret: &corev1.ObjectReference{ - Name: "test-pull-secret", - Namespace: "openshift-config", - }, - }, - } - mcfgObjs = append(mcfgObjs, controllerConfig) - fakeMcfgClient = mcfgfake.NewSimpleClientset(mcfgObjs...) - mcfgInformerFactory = mcfginformers.NewSharedInformerFactory(fakeMcfgClient, 0) - - // Provide PullSecret - pullSecret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-pull-secret", - Namespace: "openshift-config", - }, - Type: corev1.SecretTypeDockerConfigJson, - Data: map[string][]byte{ - corev1.DockerConfigJsonKey: []byte(`{"auths":{}}`), - }, - } - fakeKubeClient := kubefake.NewSimpleClientset(pullSecret) - kubeInformerFactory := kubeinformers.NewSharedInformerFactory(fakeKubeClient, 0) - - // Setup informers - ccInformer := mcfgInformerFactory.Machineconfiguration().V1().ControllerConfigs() - cmInformer := kubeInformerFactory.Core().V1().ConfigMaps() - osImageStreamInformer := mcfgInformerFactory.Machineconfiguration().V1alpha1().OSImageStreams() - cvInformer := configInformerFactory.Config().V1().ClusterVersions() - - // Add objects to indexers - cvInformer.Informer().GetIndexer().Add(clusterVersion) - ccInformer.Informer().GetIndexer().Add(controllerConfig) - - // Mock factory that returns the new OSImageStream - mockFactory := &mockImageStreamFactory{ - runtimeStream: newOSImageStream, - } - - // Create controller using constructor - ctrl := NewController( - fakeKubeClient, - fakeMcfgClient, - ccInformer, - cmInformer, - osImageStreamInformer, - cvInformer, - mockFactory, - ) - - // Start informers - stopCh := make(chan struct{}) - defer close(stopCh) - - mcfgInformerFactory.Start(stopCh) - configInformerFactory.Start(stopCh) - kubeInformerFactory.Start(stopCh) - - // Run controller in goroutine - go ctrl.Run(stopCh) - - // Wait for boot to complete using WaitBoot - done := make(chan error, 1) - go func() { - done <- ctrl.WaitBoot() - }() - - select { - case err := <-done: - require.NoError(t, err) - - // Verify the OSImageStream was created - created, err := fakeMcfgClient.MachineconfigurationV1alpha1(). - OSImageStreams(). - Get(context.TODO(), ctrlcommon.ClusterInstanceNameOSImageStream, metav1.GetOptions{}) - require.NoError(t, err) - assert.Equal(t, version.Hash, created.Annotations[ctrlcommon.ReleaseImageVersionAnnotationKey]) - assert.Equal(t, "rhel-9", created.Status.DefaultStream) - assert.Len(t, created.Status.AvailableStreams, 1) - assert.Equal(t, v1alpha1.ImageDigestFormat("image1"), created.Status.AvailableStreams[0].OSImage) - case <-time.After(2 * time.Second): - t.Fatal("Boot did not complete in time") - } -} - -func TestController_Run_OldVersion(t *testing.T) { - // Create existing OSImageStream with old version (update needed) - existingOSImageStream := &v1alpha1.OSImageStream{ - ObjectMeta: metav1.ObjectMeta{ - Name: ctrlcommon.ClusterInstanceNameOSImageStream, - Annotations: map[string]string{ - ctrlcommon.ReleaseImageVersionAnnotationKey: "old-version-hash", - }, - }, - Status: v1alpha1.OSImageStreamStatus{ - DefaultStream: "rhel-9-old", - AvailableStreams: []v1alpha1.OSImageStreamSet{ - {Name: "rhel-9-old", OSImage: "old-image", OSExtensionsImage: "old-ext"}, - }, - }, - } - - // Updated OSImageStream that will be returned by the mock factory - updatedOSImageStream := &v1alpha1.OSImageStream{ - ObjectMeta: metav1.ObjectMeta{ - Name: ctrlcommon.ClusterInstanceNameOSImageStream, - Annotations: map[string]string{ - ctrlcommon.ReleaseImageVersionAnnotationKey: version.Hash, - ctrlcommon.GeneratedByControllerVersionAnnotationKey: version.Hash, - }, - }, - Status: v1alpha1.OSImageStreamStatus{ - DefaultStream: "rhel-9", - AvailableStreams: []v1alpha1.OSImageStreamSet{ - {Name: "rhel-9", OSImage: "new-image", OSExtensionsImage: "new-ext"}, - }, - }, - } - - // Create fake clients - mcfgObjs := []runtime.Object{existingOSImageStream} - fakeMcfgClient := mcfgfake.NewSimpleClientset(mcfgObjs...) - mcfgInformerFactory := mcfginformers.NewSharedInformerFactory(fakeMcfgClient, 0) - - // Provide ClusterVersion - clusterVersion := &configv1.ClusterVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "version", - }, - Status: configv1.ClusterVersionStatus{ - Desired: configv1.Release{ - Image: "quay.io/openshift-release-dev/ocp-release@sha256:abc123", - }, - }, - } - configObjs := []runtime.Object{clusterVersion} - fakeConfigClient := configfake.NewSimpleClientset(configObjs...) - configInformerFactory := configinformers.NewSharedInformerFactory(fakeConfigClient, 0) - - // Provide ControllerConfig with PullSecret - controllerConfig := &mcfgv1.ControllerConfig{ - ObjectMeta: metav1.ObjectMeta{ - Name: ctrlcommon.ControllerConfigName, - }, - Spec: mcfgv1.ControllerConfigSpec{ - PullSecret: &corev1.ObjectReference{ - Name: "test-pull-secret", - Namespace: "openshift-config", - }, - }, - } - mcfgObjs = append(mcfgObjs, controllerConfig) - fakeMcfgClient = mcfgfake.NewSimpleClientset(mcfgObjs...) - mcfgInformerFactory = mcfginformers.NewSharedInformerFactory(fakeMcfgClient, 0) - - // Provide PullSecret - pullSecret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-pull-secret", - Namespace: "openshift-config", - }, - Type: corev1.SecretTypeDockerConfigJson, - Data: map[string][]byte{ - corev1.DockerConfigJsonKey: []byte(`{"auths":{}}`), - }, - } - fakeKubeClient := kubefake.NewSimpleClientset(pullSecret) - kubeInformerFactory := kubeinformers.NewSharedInformerFactory(fakeKubeClient, 0) - - // Setup informers - ccInformer := mcfgInformerFactory.Machineconfiguration().V1().ControllerConfigs() - cmInformer := kubeInformerFactory.Core().V1().ConfigMaps() - osImageStreamInformer := mcfgInformerFactory.Machineconfiguration().V1alpha1().OSImageStreams() - cvInformer := configInformerFactory.Config().V1().ClusterVersions() - - // Add objects to indexers - osImageStreamInformer.Informer().GetIndexer().Add(existingOSImageStream) - cvInformer.Informer().GetIndexer().Add(clusterVersion) - ccInformer.Informer().GetIndexer().Add(controllerConfig) - - // Mock factory that returns the updated OSImageStream - mockFactory := &mockImageStreamFactory{ - runtimeStream: updatedOSImageStream, - } - - // Create controller using constructor - ctrl := NewController( - fakeKubeClient, - fakeMcfgClient, - ccInformer, - cmInformer, - osImageStreamInformer, - cvInformer, - mockFactory, - ) - - // Start informers - stopCh := make(chan struct{}) - defer close(stopCh) - - mcfgInformerFactory.Start(stopCh) - configInformerFactory.Start(stopCh) - kubeInformerFactory.Start(stopCh) - - // Run controller in goroutine - go ctrl.Run(stopCh) - - // Wait for boot to complete using WaitBoot - done := make(chan error, 1) - go func() { - done <- ctrl.WaitBoot() - }() - - select { - case err := <-done: - require.NoError(t, err) - - // Verify the OSImageStream was updated - updated, err := fakeMcfgClient.MachineconfigurationV1alpha1(). - OSImageStreams(). - Get(context.TODO(), ctrlcommon.ClusterInstanceNameOSImageStream, metav1.GetOptions{}) - require.NoError(t, err) - assert.Equal(t, version.Hash, updated.Annotations[ctrlcommon.ReleaseImageVersionAnnotationKey]) - assert.Equal(t, "rhel-9", updated.Status.DefaultStream) - assert.Equal(t, v1alpha1.ImageDigestFormat("new-image"), updated.Status.AvailableStreams[0].OSImage) - case <-time.After(2 * time.Second): - t.Fatal("Boot did not complete in time") - } -} diff --git a/pkg/controller/render/render_controller.go b/pkg/controller/render/render_controller.go index 055534a4a6..d219646e3f 100644 --- a/pkg/controller/render/render_controller.go +++ b/pkg/controller/render/render_controller.go @@ -10,17 +10,21 @@ import ( "github.com/openshift/api/features" mcfgv1 "github.com/openshift/api/machineconfiguration/v1" + "github.com/openshift/api/machineconfiguration/v1alpha1" 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" mcfginformersv1 "github.com/openshift/client-go/machineconfiguration/informers/externalversions/machineconfiguration/v1" + mcfginformersv1alpha1 "github.com/openshift/client-go/machineconfiguration/informers/externalversions/machineconfiguration/v1alpha1" mcfglistersv1 "github.com/openshift/client-go/machineconfiguration/listers/machineconfiguration/v1" + mcfglistersv1alpha1 "github.com/openshift/client-go/machineconfiguration/listers/machineconfiguration/v1alpha1" mcopinformersv1 "github.com/openshift/client-go/operator/informers/externalversions/operator/v1" mcoplistersv1 "github.com/openshift/client-go/operator/listers/operator/v1" mcoResourceApply "github.com/openshift/machine-config-operator/lib/resourceapply" "github.com/openshift/machine-config-operator/pkg/apihelpers" ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common" daemonconsts "github.com/openshift/machine-config-operator/pkg/daemon/constants" + "github.com/openshift/machine-config-operator/pkg/osimagestream" "github.com/openshift/machine-config-operator/pkg/version" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -65,11 +69,13 @@ type Controller struct { syncHandler func(mcp string) error enqueueMachineConfigPool func(*mcfgv1.MachineConfigPool) - mcpLister mcfglistersv1.MachineConfigPoolLister - mcLister mcfglistersv1.MachineConfigLister + mcpLister mcfglistersv1.MachineConfigPoolLister + mcLister mcfglistersv1.MachineConfigLister + osImageStreamLister mcfglistersv1alpha1.OSImageStreamLister - mcpListerSynced cache.InformerSynced - mcListerSynced cache.InformerSynced + mcpListerSynced cache.InformerSynced + mcListerSynced cache.InformerSynced + osImageStreamListerSynced cache.InformerSynced ccLister mcfglistersv1.ControllerConfigLister ccListerSynced cache.InformerSynced @@ -96,6 +102,7 @@ func New( crcInformer mcfginformersv1.ContainerRuntimeConfigInformer, mckInformer mcfginformersv1.KubeletConfigInformer, mcopInformer mcopinformersv1.MachineConfigurationInformer, + osImageStreamInformer mcfginformersv1alpha1.OSImageStreamInformer, kubeClient clientset.Interface, mcfgClient mcfgclientset.Interface, featureGatesHandler ctrlcommon.FeatureGatesHandler, @@ -140,6 +147,10 @@ func New( ctrl.mcopLister = mcopInformer.Lister() ctrl.mcopListerSynced = mcopInformer.Informer().HasSynced + if osImageStreamInformer != nil && osimagestream.IsFeatureEnabled(ctrl.fgHandler) { + ctrl.osImageStreamLister = osImageStreamInformer.Lister() + ctrl.osImageStreamListerSynced = osImageStreamInformer.Informer().HasSynced + } return ctrl } @@ -148,7 +159,13 @@ func (ctrl *Controller) Run(workers int, stopCh <-chan struct{}) { defer utilruntime.HandleCrash() defer ctrl.queue.ShutDown() - if !cache.WaitForCacheSync(stopCh, ctrl.mcpListerSynced, ctrl.mcListerSynced, ctrl.ccListerSynced) { + listerCaches := []cache.InformerSynced{ctrl.mcpListerSynced, ctrl.mcListerSynced, ctrl.ccListerSynced} + + // OSImageStreams and MCPs fetched only if FeatureGateOSStreams active + if ctrl.osImageStreamListerSynced != nil { + listerCaches = append(listerCaches, ctrl.osImageStreamListerSynced) + } + if !cache.WaitForCacheSync(stopCh, listerCaches...) { return } @@ -509,12 +526,12 @@ func (ctrl *Controller) garbageCollectRenderedConfigs(_ *mcfgv1.MachineConfigPoo return nil } -func (ctrl *Controller) getRenderedMachineConfig(pool *mcfgv1.MachineConfigPool, configs []*mcfgv1.MachineConfig, cc *mcfgv1.ControllerConfig) (*mcfgv1.MachineConfig, error) { +func (ctrl *Controller) getRenderedMachineConfig(pool *mcfgv1.MachineConfigPool, configs []*mcfgv1.MachineConfig, cc *mcfgv1.ControllerConfig, osImageStreamSet *v1alpha1.OSImageStreamSet) (*mcfgv1.MachineConfig, error) { // If we don't yet have a rendered MachineConfig on the pool, we cannot // perform reconciliation. So we must solely generate the rendered // MachineConfig. if pool.Spec.Configuration.Name == "" { - return generateRenderedMachineConfig(pool, configs, cc) + return generateRenderedMachineConfig(pool, configs, cc, osImageStreamSet) } // The pool has a rendered MachineConfig, so we can do more advanced @@ -523,7 +540,7 @@ func (ctrl *Controller) getRenderedMachineConfig(pool *mcfgv1.MachineConfigPool, // Degenerate case: When the renderedMC that the MCP is currently pointing to is deleted if apierrors.IsNotFound(err) { - generated, err := generateRenderedMachineConfig(pool, configs, cc) + generated, err := generateRenderedMachineConfig(pool, configs, cc, osImageStreamSet) if err != nil { return nil, err } @@ -542,7 +559,24 @@ func (ctrl *Controller) getRenderedMachineConfig(pool *mcfgv1.MachineConfigPool, mcop = *mcopPtr } - return generateAndValidateRenderedMachineConfig(currentMC, pool, configs, cc, &mcop.Spec.IrreconcilableValidationOverrides) + return generateAndValidateRenderedMachineConfig(currentMC, pool, configs, cc, &mcop.Spec.IrreconcilableValidationOverrides, osImageStreamSet) +} + +func (ctrl *Controller) getOSImageStreamForPool(pool *mcfgv1.MachineConfigPool) (*v1alpha1.OSImageStreamSet, error) { + if !osimagestream.IsFeatureEnabled(ctrl.fgHandler) || ctrl.osImageStreamLister == nil { + return nil, nil + } + + 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) + if err != nil { + return nil, fmt.Errorf("could not get OSImageStreamSet for pool %s: %w", pool.Name, err) + } + return imageStreamSet, nil } func (ctrl *Controller) syncGeneratedMachineConfig(pool *mcfgv1.MachineConfigPool, configs []*mcfgv1.MachineConfig) error { @@ -555,14 +589,19 @@ func (ctrl *Controller) syncGeneratedMachineConfig(pool *mcfgv1.MachineConfigPoo return err } - generated, err := ctrl.getRenderedMachineConfig(pool, configs, cc) + osImageStreamSet, err := ctrl.getOSImageStreamForPool(pool) + if err != nil { + return err + } + + generated, err := ctrl.getRenderedMachineConfig(pool, configs, cc, osImageStreamSet) if err != nil { return fmt.Errorf("could not generate rendered MachineConfig: %w", err) } // Collect metric when OSImageURL was overridden var isOSImageURLOverridden bool - if generated.Spec.OSImageURL != ctrlcommon.GetDefaultBaseImageContainer(&cc.Spec) { + if generated.Spec.OSImageURL != ctrlcommon.GetBaseImageContainer(&cc.Spec, osImageStreamSet) { ctrlcommon.OSImageURLOverride.WithLabelValues(pool.Name).Set(1) isOSImageURLOverridden = true } else { @@ -613,7 +652,7 @@ func (ctrl *Controller) syncGeneratedMachineConfig(pool *mcfgv1.MachineConfigPoo } // generateRenderedMachineConfig takes all MCs for a given pool and returns a single rendered MC. For ex master-XXXX or worker-XXXX -func generateRenderedMachineConfig(pool *mcfgv1.MachineConfigPool, configs []*mcfgv1.MachineConfig, cconfig *mcfgv1.ControllerConfig) (*mcfgv1.MachineConfig, error) { +func generateRenderedMachineConfig(pool *mcfgv1.MachineConfigPool, configs []*mcfgv1.MachineConfig, cconfig *mcfgv1.ControllerConfig, osImageStreamSet *v1alpha1.OSImageStreamSet) (*mcfgv1.MachineConfig, error) { // Suppress rendered config generation until a corresponding new controller can roll out too. // https://bugzilla.redhat.com/show_bug.cgi?id=1879099 if genver, ok := cconfig.Annotations[daemonconsts.GeneratedByVersionAnnotationKey]; ok { @@ -644,7 +683,7 @@ func generateRenderedMachineConfig(pool *mcfgv1.MachineConfigPool, configs []*mc } } - merged, err := ctrlcommon.MergeMachineConfigs(configs, cconfig) + merged, err := ctrlcommon.MergeMachineConfigs(configs, cconfig, osImageStreamSet) if err != nil { return nil, err @@ -666,7 +705,7 @@ func generateRenderedMachineConfig(pool *mcfgv1.MachineConfigPool, configs []*mc // The operator needs to know the user overrode this, so it knows if it needs to skip the // OSImageURL check during upgrade -- if the user took over managing OS upgrades this way, // the operator shouldn't stop the rest of the upgrade from progressing/completing. - if merged.Spec.OSImageURL != ctrlcommon.GetDefaultBaseImageContainer(&cconfig.Spec) { + if merged.Spec.OSImageURL != ctrlcommon.GetBaseImageContainer(&cconfig.Spec, osImageStreamSet) { merged.Annotations[ctrlcommon.OSImageURLOverriddenKey] = "true" // Log a warning if the osImageURL is set using a tag instead of a digest if !strings.Contains(merged.Spec.OSImageURL, "sha256:") { @@ -689,11 +728,12 @@ func generateAndValidateRenderedMachineConfig( pool *mcfgv1.MachineConfigPool, configs []*mcfgv1.MachineConfig, cconfig *mcfgv1.ControllerConfig, - validationOverrides *opv1.IrreconcilableValidationOverrides) (*mcfgv1.MachineConfig, error) { + validationOverrides *opv1.IrreconcilableValidationOverrides, + osImageStreamSet *v1alpha1.OSImageStreamSet) (*mcfgv1.MachineConfig, error) { source := getMachineConfigRefs(configs) klog.V(4).Infof("Considering %d configs %s for MachineConfig generation", len(source), source) - generated, err := generateRenderedMachineConfig(pool, configs, cconfig) + generated, err := generateRenderedMachineConfig(pool, configs, cconfig, osImageStreamSet) if err != nil { return nil, err } @@ -738,7 +778,7 @@ func generateAndValidateRenderedMachineConfig( // 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. -func RunBootstrap(pools []*mcfgv1.MachineConfigPool, configs []*mcfgv1.MachineConfig, cconfig *mcfgv1.ControllerConfig) ([]*mcfgv1.MachineConfigPool, []*mcfgv1.MachineConfig, error) { +func RunBootstrap(pools []*mcfgv1.MachineConfigPool, configs []*mcfgv1.MachineConfig, cconfig *mcfgv1.ControllerConfig, osImageStream *v1alpha1.OSImageStream) ([]*mcfgv1.MachineConfigPool, []*mcfgv1.MachineConfig, error) { var ( opools []*mcfgv1.MachineConfigPool oconfigs []*mcfgv1.MachineConfig @@ -748,8 +788,15 @@ func RunBootstrap(pools []*mcfgv1.MachineConfigPool, configs []*mcfgv1.MachineCo if err != nil { return nil, nil, err } + var osImageStreamSet *v1alpha1.OSImageStreamSet + if osImageStream != nil { + osImageStreamSet, err = osimagestream.GetOSImageStreamSetByName(osImageStream, pool.Spec.OSImageStream.Name) + if err != nil { + return nil, nil, fmt.Errorf("couldn't get the OSImageStream for pool %s %w", pool.Name, err) + } + } - generated, err := generateRenderedMachineConfig(pool, pcs, cconfig) + generated, err := generateRenderedMachineConfig(pool, pcs, cconfig, osImageStreamSet) if err != nil { return nil, nil, err } diff --git a/pkg/controller/render/render_controller_test.go b/pkg/controller/render/render_controller_test.go index 1d567da40e..b17c4a9575 100644 --- a/pkg/controller/render/render_controller_test.go +++ b/pkg/controller/render/render_controller_test.go @@ -8,6 +8,7 @@ import ( "github.com/clarketm/json" ign3types "github.com/coreos/ignition/v2/config/v3_5/types" + apicfgv1 "github.com/openshift/api/config/v1" configv1 "github.com/openshift/api/config/v1" mcopfake "github.com/openshift/client-go/operator/clientset/versioned/fake" operatorinformer "github.com/openshift/client-go/operator/informers/externalversions" @@ -64,6 +65,10 @@ func newFixture(t *testing.T) *fixture { f.t = t f.objects = []runtime.Object{} f.oObjects = []runtime.Object{} + f.fgHandler = ctrlcommon.NewFeatureGatesHardcodedHandler( + []apicfgv1.FeatureGateName{}, + []apicfgv1.FeatureGateName{}, + ) return f } @@ -76,7 +81,7 @@ func (f *fixture) newController() *Controller { c := New(i.Machineconfiguration().V1().MachineConfigPools(), i.Machineconfiguration().V1().MachineConfigs(), i.Machineconfiguration().V1().ControllerConfigs(), i.Machineconfiguration().V1().ContainerRuntimeConfigs(), i.Machineconfiguration().V1().KubeletConfigs(), oi.Operator().V1().MachineConfigurations(), - k8sfake.NewSimpleClientset(), f.client, f.fgHandler) + i.Machineconfiguration().V1alpha1().OSImageStreams(), k8sfake.NewSimpleClientset(), f.client, f.fgHandler) c.mcpListerSynced = alwaysReady c.mcListerSynced = alwaysReady @@ -299,7 +304,7 @@ func TestCreatesGeneratedMachineConfig(t *testing.T) { f.objects = append(f.objects, mcs[idx]) } - gmc, err := generateRenderedMachineConfig(mcp, mcs, cc) + gmc, err := generateRenderedMachineConfig(mcp, mcs, cc, nil) assert.NoError(t, err) mcpNew := mcp.DeepCopy() @@ -331,7 +336,7 @@ func TestIgnValidationGenerateRenderedMachineConfig(t *testing.T) { } cc := newControllerConfig(ctrlcommon.ControllerConfigName) - _, err := generateRenderedMachineConfig(mcp, mcs, cc) + _, err := generateRenderedMachineConfig(mcp, mcs, cc, nil) require.Nil(t, err) // verify that an invalid ignition config (here a config with content and an empty version, @@ -343,7 +348,7 @@ func TestIgnValidationGenerateRenderedMachineConfig(t *testing.T) { require.Nil(t, err) mcs[1].Spec.Config.Raw = rawIgnCfg - _, err = generateRenderedMachineConfig(mcp, mcs, cc) + _, err = generateRenderedMachineConfig(mcp, mcs, cc, nil) require.NotNil(t, err) // verify that a machine config with no ignition content will not fail validation @@ -352,7 +357,7 @@ func TestIgnValidationGenerateRenderedMachineConfig(t *testing.T) { require.Nil(t, err) mcs[1].Spec.Config.Raw = rawEmptyIgnCfg mcs[1].Spec.KernelArguments = append(mcs[1].Spec.KernelArguments, "test1") - _, err = generateRenderedMachineConfig(mcp, mcs, cc) + _, err = generateRenderedMachineConfig(mcp, mcs, cc, nil) require.Nil(t, err) } @@ -377,7 +382,7 @@ func TestUpdatesGeneratedMachineConfig(t *testing.T) { } cc := newControllerConfig(ctrlcommon.ControllerConfigName) - gmc, err := generateRenderedMachineConfig(mcp, mcs, cc) + gmc, err := generateRenderedMachineConfig(mcp, mcs, cc, nil) if err != nil { t.Fatal(err) } @@ -400,7 +405,7 @@ func TestUpdatesGeneratedMachineConfig(t *testing.T) { f.mcLister = append(f.mcLister, gmc) f.objects = append(f.objects, gmc) - expmc, err := generateRenderedMachineConfig(mcp, mcs, cc) + expmc, err := generateRenderedMachineConfig(mcp, mcs, cc, nil) if err != nil { t.Fatal(err) } @@ -424,7 +429,7 @@ func TestGenerateMachineConfigOverrideOSImageURL(t *testing.T) { cc := newControllerConfig(ctrlcommon.ControllerConfigName) - gmc, err := generateRenderedMachineConfig(mcp, mcs, cc) + gmc, err := generateRenderedMachineConfig(mcp, mcs, cc, nil) if err != nil { t.Fatal(err) } @@ -432,7 +437,7 @@ func TestGenerateMachineConfigOverrideOSImageURL(t *testing.T) { mcs = append(mcs, helpers.NewMachineConfig("00-test-cluster-master-1", map[string]string{"node-role/master": ""}, "dummy-change-2", []ign3types.File{})) - gmc, err = generateAndValidateRenderedMachineConfig(gmc, mcp, mcs, cc, nil) + gmc, err = generateAndValidateRenderedMachineConfig(gmc, mcp, mcs, cc, nil, nil) assert.NoError(t, err) assert.Equal(t, "dummy-change-2", gmc.Spec.OSImageURL) } @@ -446,12 +451,12 @@ func TestVersionSkew(t *testing.T) { cc := newControllerConfig(ctrlcommon.ControllerConfigName) cc.Annotations[daemonconsts.GeneratedByVersionAnnotationKey] = "different-version" - _, err := generateRenderedMachineConfig(mcp, mcs, cc) + _, err := generateRenderedMachineConfig(mcp, mcs, cc, nil) require.NotNil(t, err) // Now the same thing without overriding the version cc = newControllerConfig(ctrlcommon.ControllerConfigName) - gmc, err := generateRenderedMachineConfig(mcp, mcs, cc) + gmc, err := generateRenderedMachineConfig(mcp, mcs, cc, nil) require.Nil(t, err) require.NotNil(t, gmc) } @@ -464,14 +469,14 @@ func TestGenerateRenderedConfigOnLatestControllerVersionOnly(t *testing.T) { } version.Hash = "2" cc := newControllerConfig(ctrlcommon.ControllerConfigName) - _, err := generateRenderedMachineConfig(mcp, mcs, cc) + _, err := generateRenderedMachineConfig(mcp, mcs, cc, nil) require.NotNil(t, err) mcs = []*mcfgv1.MachineConfig{ helpers.NewMachineConfigWithAnnotation("00-updated-conf", map[string]string{"node-role/master": ""}, map[string]string{ctrlcommon.GeneratedByControllerVersionAnnotationKey: "2"}, "dummy-test-1", []ign3types.File{}), helpers.NewMachineConfigWithAnnotation("99-user-conf", map[string]string{"node-role/master": ""}, map[string]string{ctrlcommon.GeneratedByControllerVersionAnnotationKey: ""}, "user-data", []ign3types.File{}), } - _, err = generateRenderedMachineConfig(mcp, mcs, cc) + _, err = generateRenderedMachineConfig(mcp, mcs, cc, nil) require.Nil(t, err) } @@ -495,7 +500,7 @@ func TestDoNothing(t *testing.T) { } cc := newControllerConfig(ctrlcommon.ControllerConfigName) - gmc, err := generateRenderedMachineConfig(mcp, mcs, cc) + gmc, err := generateRenderedMachineConfig(mcp, mcs, cc, nil) if err != nil { t.Fatal(err) } @@ -604,7 +609,7 @@ func TestGenerateMachineConfigValidation(t *testing.T) { cc := newControllerConfig(ctrlcommon.ControllerConfigName) - gmc, err := generateAndValidateRenderedMachineConfig(currentMC, mcp, mcs, cc, nil) + gmc, err := generateAndValidateRenderedMachineConfig(currentMC, mcp, mcs, cc, nil, nil) assert.Error(t, err) assert.Nil(t, gmc) } diff --git a/pkg/controller/template/render.go b/pkg/controller/template/render.go index fbb6b67395..b8763e6418 100644 --- a/pkg/controller/template/render.go +++ b/pkg/controller/template/render.go @@ -101,7 +101,11 @@ func generateTemplateMachineConfigs(config *RenderConfig, templateDir string) ([ continue } - roleConfigs, err := GenerateMachineConfigsForRole(config, role, templateDir) + roleConfigs, err := GenerateMachineConfigsForRole( + config, + role, + templateDir, + ) if err != nil { return nil, fmt.Errorf("failed to create MachineConfig for role %s: %w", role, err) } diff --git a/pkg/imageutils/layer_reader.go b/pkg/imageutils/layer_reader.go index 6e2f334bde..d121dc5655 100644 --- a/pkg/imageutils/layer_reader.go +++ b/pkg/imageutils/layer_reader.go @@ -21,7 +21,7 @@ type ReadImageFileContentFn func(*tar.Header) bool // It iterates through the image layers (starting from the last) and uses matcherFn // to identify the target file. When found, the file content is returned as a byte slice. // If no matching file is found, (nil, nil) is returned. -func ReadImageFileContent(ctx context.Context, sysCtx *types.SystemContext, imageName string, matcherFn ReadImageFileContentFn) (content []byte, err error) { +func ReadImageFileContent(ctx context.Context, sysCtx *types.SystemContext, imageName string, matcherFn ReadImageFileContentFn, retryOpts *retry.Options) (content []byte, err error) { ref, err := ParseImageName(imageName) if err != nil { return nil, err @@ -36,7 +36,11 @@ func ReadImageFileContent(ctx context.Context, sysCtx *types.SystemContext, imag } }() - src, err := GetImageSourceFromReference(ctx, sysCtx, ref, &retry.Options{MaxRetry: 2}) + retries := retryOpts + if retries == nil { + retries = &retry.Options{MaxRetry: 2} + } + src, err := GetImageSourceFromReference(ctx, sysCtx, ref, retries) if err != nil { return nil, err } diff --git a/pkg/operator/operator.go b/pkg/operator/operator.go index da0e9cacb2..b859220b98 100644 --- a/pkg/operator/operator.go +++ b/pkg/operator/operator.go @@ -6,6 +6,7 @@ import ( "time" opv1 "github.com/openshift/api/operator/v1" + "github.com/openshift/machine-config-operator/pkg/osimagestream" "k8s.io/klog/v2" "k8s.io/utils/clock" @@ -40,8 +41,10 @@ import ( mcfgclientset "github.com/openshift/client-go/machineconfiguration/clientset/versioned" "github.com/openshift/client-go/machineconfiguration/clientset/versioned/scheme" mcfginformersv1 "github.com/openshift/client-go/machineconfiguration/informers/externalversions/machineconfiguration/v1" + mcfginformersv1alpha1 "github.com/openshift/client-go/machineconfiguration/informers/externalversions/machineconfiguration/v1alpha1" mcfglistersv1 "github.com/openshift/client-go/machineconfiguration/listers/machineconfiguration/v1" + mcfglistersv1alpha1 "github.com/openshift/client-go/machineconfiguration/listers/machineconfiguration/v1alpha1" mcopclientset "github.com/openshift/client-go/operator/clientset/versioned" mcopinformersv1 "github.com/openshift/client-go/operator/informers/externalversions/operator/v1" @@ -110,6 +113,8 @@ type Operator struct { nodeClusterLister configlistersv1.NodeLister moscLister mcfglistersv1.MachineOSConfigLister apiserverLister configlistersv1.APIServerLister + clusterVersionLister configlistersv1.ClusterVersionLister + osImageStreamLister mcfglistersv1alpha1.OSImageStreamLister crdListerSynced cache.InformerSynced deployListerSynced cache.InformerSynced @@ -142,6 +147,7 @@ type Operator struct { nodeClusterListerSynced cache.InformerSynced moscListerSynced cache.InformerSynced apiserverListerSynced cache.InformerSynced + osImageStreamListerSynced cache.InformerSynced // queue only ever has one item, but it has nice error handling backoff/retry semantics queue workqueue.TypedRateLimitingInterface[string] @@ -195,6 +201,8 @@ func New( nodeClusterInformer configinformersv1.NodeInformer, apiserverInformer configinformersv1.APIServerInformer, moscInformer mcfginformersv1.MachineOSConfigInformer, + clusterVersionInformer configinformersv1.ClusterVersionInformer, + osImageStreamInformer mcfginformersv1alpha1.OSImageStreamInformer, ctrlctx *ctrlcommon.ControllerContext, ) *Operator { eventBroadcaster := record.NewBroadcaster() @@ -332,6 +340,11 @@ func New( optr.apiserverListerSynced = apiserverInformer.Informer().HasSynced optr.moscLister = moscInformer.Lister() optr.moscListerSynced = moscInformer.Informer().HasSynced + optr.clusterVersionLister = clusterVersionInformer.Lister() + if osImageStreamInformer != nil && osimagestream.IsFeatureEnabled(optr.fgHandler) { + optr.osImageStreamLister = osImageStreamInformer.Lister() + optr.osImageStreamListerSynced = osImageStreamInformer.Informer().HasSynced + } optr.vStore.Set("operator", version.ReleaseVersion) optr.vStore.Set("operator-image", version.OperatorImage) @@ -386,7 +399,9 @@ func (optr *Operator) Run(workers int, stopCh <-chan struct{}) { optr.nodeClusterListerSynced, optr.moscListerSynced, } - + if optr.osImageStreamListerSynced != nil && osimagestream.IsFeatureEnabled(optr.fgHandler) { + cacheSynced = append(cacheSynced, optr.osImageStreamListerSynced) + } if !cache.WaitForCacheSync(stopCh, cacheSynced...) { klog.Error("failed to sync caches") @@ -510,8 +525,11 @@ func (optr *Operator) sync(key string) error { // syncFuncs is the list of sync functions that are executed in order. // any error marks sync as failure. syncFuncs := []syncFunc{ - // "RenderConfig" must always run first as it sets the renderConfig in the operator - // for the sync funcs below + // OSImageStream must run FIRST to provide OS image information as RenderConfig will read + // images references from OSImageStream + {"OSImageStream", optr.syncOSImageStream}, + // "RenderConfig" should be the first one to run (except OSImageStream) as it sets the renderConfig in + // the operator for the sync funcs below {"RenderConfig", optr.syncRenderConfig}, {"MachineConfiguration", optr.syncMachineConfiguration}, {"MachineConfigNode", optr.syncMachineConfigNodes}, diff --git a/pkg/operator/sync.go b/pkg/operator/sync.go index 5cb26773d5..5a8d50b2f8 100644 --- a/pkg/operator/sync.go +++ b/pkg/operator/sync.go @@ -18,6 +18,7 @@ import ( "time" configclientscheme "github.com/openshift/client-go/config/clientset/versioned/scheme" + "github.com/openshift/machine-config-operator/pkg/osimagestream" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -37,6 +38,7 @@ import ( "github.com/openshift/api/annotations" configv1 "github.com/openshift/api/config/v1" mcfgv1 "github.com/openshift/api/machineconfiguration/v1" + "github.com/openshift/api/machineconfiguration/v1alpha1" opv1 "github.com/openshift/api/operator/v1" features "github.com/openshift/api/features" @@ -256,6 +258,75 @@ func isCloudConfRequired(infra *configv1.Infrastructure) bool { return platformsRequiringCloudConf.Has(string(infra.Status.PlatformStatus.Type)) } +// getImageRegistryBundles retrieves and returns image registry certificate bundles. +// It fetches both user-provided additional trusted CAs and managed registry CAs. +func (optr *Operator) getImageRegistryBundles() ([]mcfgv1.ImageRegistryBundle, []mcfgv1.ImageRegistryBundle, error) { + cfg, err := optr.imgLister.Get("cluster") + if err != nil { + return nil, nil, err + } + + imgRegistryUsrData := []mcfgv1.ImageRegistryBundle{} + if cfg.Spec.AdditionalTrustedCA.Name != "" { + cm, err := optr.ocCmLister.ConfigMaps(ctrlcommon.OpenshiftConfigNamespace).Get(cfg.Spec.AdditionalTrustedCA.Name) + if err != nil { + klog.Warningf("could not find configmap specified in image.config.openshift.io/cluster with the name %s", cfg.Spec.AdditionalTrustedCA.Name) + } else { + newKeys := sets.StringKeySet(cm.Data).List() + newBinaryKeys := sets.StringKeySet(cm.BinaryData).List() + for _, key := range newKeys { + raw, err := base64.StdEncoding.DecodeString(cm.Data[key]) + if err != nil { + imgRegistryUsrData = append(imgRegistryUsrData, mcfgv1.ImageRegistryBundle{ + File: key, + Data: []byte(cm.Data[key]), + }) + } else { + imgRegistryUsrData = append(imgRegistryUsrData, mcfgv1.ImageRegistryBundle{ + File: key, + Data: raw, + }) + } + } + for _, key := range newBinaryKeys { + imgRegistryUsrData = append(imgRegistryUsrData, mcfgv1.ImageRegistryBundle{ + File: key, + Data: cm.BinaryData[key], + }) + } + } + } + + imgRegistryData := []mcfgv1.ImageRegistryBundle{} + cm, err := optr.clusterCmLister.ConfigMaps("openshift-config-managed").Get("image-registry-ca") + if err == nil { + newKeys := sets.StringKeySet(cm.Data).List() + newBinaryKeys := sets.StringKeySet(cm.BinaryData).List() + for _, key := range newKeys { + raw, err := base64.StdEncoding.DecodeString(cm.Data[key]) + if err != nil { + imgRegistryData = append(imgRegistryData, mcfgv1.ImageRegistryBundle{ + File: key, + Data: []byte(cm.Data[key]), + }) + } else { + imgRegistryData = append(imgRegistryData, mcfgv1.ImageRegistryBundle{ + File: key, + Data: raw, + }) + } + } + for _, key := range newBinaryKeys { + imgRegistryData = append(imgRegistryData, mcfgv1.ImageRegistryBundle{ + File: key, + Data: cm.BinaryData[key], + }) + } + } + + return imgRegistryData, imgRegistryUsrData, nil +} + // Sync cloud config on supported platform from cloud.conf available in openshift-config-managed/kube-cloud-config ConfigMap. func (optr *Operator) syncCloudConfig(spec *mcfgv1.ControllerConfigSpec, infra *configv1.Infrastructure) error { cm, err := optr.clusterCmLister.ConfigMaps("openshift-config-managed").Get("kube-cloud-config") @@ -332,67 +403,10 @@ func (optr *Operator) syncRenderConfig(_ *renderConfig, _ *configv1.ClusterOpera // handle image registry certificates. // parse these, add them to ctrlcfgspec and then handle these in the daemon write to disk function - cfg, err := optr.imgLister.Get("cluster") + imgRegistryData, imgRegistryUsrData, err := optr.getImageRegistryBundles() if err != nil { return err } - imgRegistryUsrData := []mcfgv1.ImageRegistryBundle{} - if cfg.Spec.AdditionalTrustedCA.Name != "" { - cm, err := optr.ocCmLister.ConfigMaps(ctrlcommon.OpenshiftConfigNamespace).Get(cfg.Spec.AdditionalTrustedCA.Name) - if err != nil { - klog.Warningf("could not find configmap specified in image.config.openshift.io/cluster with the name %s", cfg.Spec.AdditionalTrustedCA.Name) - } else { - newKeys := sets.StringKeySet(cm.Data).List() - newBinaryKeys := sets.StringKeySet(cm.BinaryData).List() - for _, key := range newKeys { - raw, err := base64.StdEncoding.DecodeString(cm.Data[key]) - if err != nil { - imgRegistryUsrData = append(imgRegistryUsrData, mcfgv1.ImageRegistryBundle{ - File: key, - Data: []byte(cm.Data[key]), - }) - } else { - imgRegistryUsrData = append(imgRegistryUsrData, mcfgv1.ImageRegistryBundle{ - File: key, - Data: raw, - }) - } - } - for _, key := range newBinaryKeys { - imgRegistryUsrData = append(imgRegistryUsrData, mcfgv1.ImageRegistryBundle{ - File: key, - Data: cm.BinaryData[key], - }) - } - } - } - - imgRegistryData := []mcfgv1.ImageRegistryBundle{} - cm, err := optr.clusterCmLister.ConfigMaps("openshift-config-managed").Get("image-registry-ca") - if err == nil { - newKeys := sets.StringKeySet(cm.Data).List() - newBinaryKeys := sets.StringKeySet(cm.BinaryData).List() - for _, key := range newKeys { - raw, err := base64.StdEncoding.DecodeString(cm.Data[key]) - if err != nil { - imgRegistryData = append(imgRegistryData, mcfgv1.ImageRegistryBundle{ - File: key, - Data: []byte(cm.Data[key]), - }) - } else { - imgRegistryData = append(imgRegistryData, mcfgv1.ImageRegistryBundle{ - File: key, - Data: raw, - }) - } - } - for _, key := range newBinaryKeys { - imgRegistryData = append(imgRegistryData, mcfgv1.ImageRegistryBundle{ - File: key, - Data: cm.BinaryData[key], - }) - } - } mergedData := append([]mcfgv1.ImageRegistryBundle{}, append(imgRegistryData, imgRegistryUsrData...)...) caData := make(map[string]string, len(mergedData)) @@ -400,7 +414,7 @@ func (optr *Operator) syncRenderConfig(_ *renderConfig, _ *configv1.ClusterOpera caData[CA.File] = string(CA.Data) } - cm, err = optr.clusterCmLister.ConfigMaps("openshift-config-managed").Get("merged-trusted-image-registry-ca") + cm, err := optr.clusterCmLister.ConfigMaps("openshift-config-managed").Get("merged-trusted-image-registry-ca") if err != nil && !apierrors.IsNotFound(err) { return err } @@ -556,15 +570,6 @@ func (optr *Operator) syncRenderConfig(_ *renderConfig, _ *configv1.ClusterOpera internalRegistryPullSecret = nil } - // sync up os image url - // TODO: this should probably be part of the imgs - oscontainer, osextensionscontainer, err := optr.getOsImageURLs(optr.namespace) - if err != nil { - return err - } - imgs.BaseOSContainerImage = oscontainer - imgs.BaseOSExtensionsContainerImage = osextensionscontainer - // sync up the ControllerConfigSpec infra, network, proxy, dns, apiServer, err := optr.getGlobalConfig() if err != nil { @@ -608,6 +613,14 @@ func (optr *Operator) syncRenderConfig(_ *renderConfig, _ *configv1.ClusterOpera return err } + oscontainer, osextensionscontainer, err := optr.getOsImageURLs(optr.namespace) + if err != nil { + return fmt.Errorf("could not get OS images: %w", err) + + } + imgs.BaseOSContainerImage = oscontainer + imgs.BaseOSExtensionsContainerImage = osextensionscontainer + spec.KubeAPIServerServingCAData = kubeAPIServerServingCABytes spec.RootCAData = machineConfigServerCABundle spec.ImageRegistryBundleData = imgRegistryData @@ -1280,6 +1293,138 @@ func (optr *Operator) reconcileMachineOSBuilder(mob *appsv1.Deployment) error { return nil } +func (optr *Operator) syncOSImageStream(_ *renderConfig, _ *configv1.ClusterOperator) error { + klog.V(4).Info("OSImageStream sync started") + defer func() { + klog.V(4).Info("OSImageStream sync complete") + }() + + // Check if the feature is enabled + if !osimagestream.IsFeatureEnabled(optr.fgHandler) { + klog.V(4).Info("OSImageStream feature is not enabled, skipping sync") + return nil + } + + // Get the existing OSImageStream if it exists + existingOSImageStream, err := optr.getExistingOSImageStream() + if err != nil { + return err + } + + // Check if an update is needed + if !osImageStreamRequiresUpdate(existingOSImageStream) { + klog.V(4).Info("OSImageStream is already up-to-date, skipping sync") + return nil + } + + klog.Info("Starting building of the OSImageStream instance") + + // Get the release payload image from ClusterVersion + image, err := osimagestream.GetReleasePayloadImage(optr.clusterVersionLister) + if err != nil { + return fmt.Errorf("error getting the Release Image digest from the ClusterVersion for OSImageStream sync: %w", err) + } + + // Get the cluster pull secret from well-known location + clusterPullSecret, err := optr.kubeClient.CoreV1().Secrets("openshift-config").Get(context.TODO(), "pull-secret", metav1.GetOptions{}) + if err != nil { + return fmt.Errorf("could not get the cluster PullSecret for OSImageStream sync: %w", err) + } + + // Build a minimal ControllerConfig with image registry certs + // We can't use renderConfig (it runs after us) so we build the cert data directly + minimalCC, err := optr.buildMinimalControllerConfigForOSImageStream() + if err != nil { + return fmt.Errorf("could not build minimal ControllerConfig for OSImageStream: %w", err) + } + + // Build the OSImageStream using the default factory + // Use a longer timeout to account for DNS/network delays during cluster bootstrap + buildCtx, buildCancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer buildCancel() + + imageStreamFactory := osimagestream.NewDefaultStreamSourceFactory(optr.mcoCmLister, &osimagestream.DefaultImagesInspectorFactory{}) + osImageStream, err := osimagestream.BuildOsImageStreamRuntime(buildCtx, clusterPullSecret, minimalCC, image, imageStreamFactory) + if err != nil { + return fmt.Errorf("error building the OSImageStream: %w", err) + } + + // Create or update the OSImageStream resource + var updateOSImageStream *v1alpha1.OSImageStream + if existingOSImageStream == nil { + klog.V(4).Info("Creating OSImageStream singleton instance") + updateOSImageStream, err = optr.client.MachineconfigurationV1alpha1().OSImageStreams().Create(context.TODO(), osImageStream, metav1.CreateOptions{}) + if err != nil { + return fmt.Errorf("error creating the OSImageStream: %w", err) + } + klog.Infof("Created OSImageStream with %d available streams, default stream: %s", + len(osImageStream.Status.AvailableStreams), osImageStream.Status.DefaultStream) + } else { + oldVersion := existingOSImageStream.Annotations[ctrlcommon.ReleaseImageVersionAnnotationKey] + klog.V(4).Infof("Updating OSImageStream (previous version: %s, new version: %s)", oldVersion, version.Hash) + // Update metadata/spec first (mainly for annotations) + existingOSImageStream.ObjectMeta.Annotations = osImageStream.ObjectMeta.Annotations + updateOSImageStream, err = optr.client.MachineconfigurationV1alpha1().OSImageStreams().Update(context.TODO(), existingOSImageStream, metav1.UpdateOptions{}) + if err != nil { + return fmt.Errorf("error updating the OSImageStream: %w", err) + } + } + + // Update the status subresource (both for newly created and updated resources) + updateOSImageStream.Status = osImageStream.Status + if _, err = optr.client. + MachineconfigurationV1alpha1(). + OSImageStreams(). + UpdateStatus(context.TODO(), updateOSImageStream, metav1.UpdateOptions{}); err != nil { + return fmt.Errorf("error updating the OSImageStream status: %w", err) + } + + klog.Infof("OSImageStream synced successfully. Available streams: %s. Default stream: %s", + osimagestream.GetStreamSetsNames(updateOSImageStream.Status.AvailableStreams), + updateOSImageStream.Status.DefaultStream) + + return nil +} + +// buildMinimalControllerConfigForOSImageStream builds a minimal ControllerConfig with just the image registry certs +// needed for OSImageStream to inspect images. This is necessary because OSImageStream must run before RenderConfig. +func (optr *Operator) buildMinimalControllerConfigForOSImageStream() (*mcfgv1.ControllerConfig, error) { + imgRegistryData, imgRegistryUsrData, err := optr.getImageRegistryBundles() + if err != nil { + return nil, fmt.Errorf("could not get image registry bundles: %w", err) + } + + return &mcfgv1.ControllerConfig{ + Spec: mcfgv1.ControllerConfigSpec{ + ImageRegistryBundleData: imgRegistryData, + ImageRegistryBundleUserData: imgRegistryUsrData, + }, + }, nil +} + +// getExistingOSImageStream retrieves the existing OSImageStream from the lister. +// Returns nil if the OSImageStream does not exist. +func (optr *Operator) getExistingOSImageStream() (*v1alpha1.OSImageStream, error) { + osImageStream, err := optr.osImageStreamLister.Get(ctrlcommon.ClusterInstanceNameOSImageStream) + if err != nil { + if !apierrors.IsNotFound(err) { + return nil, fmt.Errorf("failed to retrieve existing OSImageStream: %v", err) + } + return nil, nil + } + return osImageStream, nil +} + +// osImageStreamRequiresUpdate checks if the OSImageStream needs to be created or updated. +// Returns true if osImageStream is nil or if its version annotation doesn't match the current version. +func osImageStreamRequiresUpdate(osImageStream *v1alpha1.OSImageStream) bool { + if osImageStream == nil { + return true + } + releaseVersion, ok := osImageStream.Annotations[ctrlcommon.ReleaseImageVersionAnnotationKey] + return !ok || releaseVersion != version.Hash +} + // Validate that the nodes part of layered pools are coreos based func (optr *Operator) validateLayeredPoolNodes(layeredMCPs []*mcfgv1.MachineConfigPool) error { nodes, err := optr.GetAllManagedNodes(layeredMCPs) @@ -1730,7 +1875,7 @@ func (optr *Operator) syncRequiredMachineConfigPools(config *renderConfig, co *c if hasRequiredPoolLabel { opURL, _, err := optr.getOsImageURLs(optr.namespace) if err != nil { - klog.Errorf("Error getting configmap osImageURL: %q", err) + klog.Errorf("Error getting OS images: %q", err) return false, nil } releaseVersion, _ := optr.vStore.Get("operator") @@ -1893,8 +2038,20 @@ func (optr *Operator) waitForControllerConfigToBeCompleted(resource *mcfgv1.Cont return nil } -// getOsImageURLs returns (new type, new extensions, old type) for operating system update images. +// getOsImageURLs retrieves the base OS and OS extensions container image URLs. +// It first checks OSImageStream (if enabled), then falls back to the ConfigMap. func (optr *Operator) getOsImageURLs(namespace string) (string, string, error) { + // If OSImageStream is enabled fetch the URLs from there + if optr.osImageStreamLister != nil && osimagestream.IsFeatureEnabled(optr.fgHandler) { + osImageStream, err := optr.osImageStreamLister.Get(ctrlcommon.ClusterInstanceNameOSImageStream) + if err != nil { + return "", "", fmt.Errorf("could not get OSImageStream: %w", err) + } + + defaultStream := osimagestream.TryGetOSImageStreamSetByName(osImageStream, "") + return string(defaultStream.OSImage), string(defaultStream.OSExtensionsImage), nil + } + cm, err := optr.mcoCmLister.ConfigMaps(namespace).Get(ctrlcommon.MachineConfigOSImageURLConfigMapName) if err != nil { return "", "", err diff --git a/pkg/controller/osimagestream/clusterversion.go b/pkg/osimagestream/clusterversion.go similarity index 100% rename from pkg/controller/osimagestream/clusterversion.go rename to pkg/osimagestream/clusterversion.go diff --git a/pkg/controller/osimagestream/clusterversion_test.go b/pkg/osimagestream/clusterversion_test.go similarity index 100% rename from pkg/controller/osimagestream/clusterversion_test.go rename to pkg/osimagestream/clusterversion_test.go diff --git a/pkg/controller/osimagestream/helpers.go b/pkg/osimagestream/helpers.go similarity index 84% rename from pkg/controller/osimagestream/helpers.go rename to pkg/osimagestream/helpers.go index 0cd7b552ac..f2cdfeb0bf 100644 --- a/pkg/controller/osimagestream/helpers.go +++ b/pkg/osimagestream/helpers.go @@ -3,10 +3,12 @@ package osimagestream import ( "fmt" + "github.com/openshift/api/features" v1 "github.com/openshift/api/machineconfiguration/v1" "github.com/openshift/api/machineconfiguration/v1alpha1" "github.com/openshift/machine-config-operator/pkg/controller/common" "github.com/openshift/machine-config-operator/pkg/helpers" + "github.com/openshift/machine-config-operator/pkg/version" k8serrors "k8s.io/apimachinery/pkg/api/errors" ) @@ -58,3 +60,9 @@ func TryGetOSImageStreamFromPoolListByPoolName(osImageStream *v1alpha1.OSImageSt return TryGetOSImageStreamSetByName(osImageStream, targetPool.Spec.OSImageStream.Name) } + +// IsFeatureEnabled checks if the OSImageStream feature is enabled. +// Returns true only if the FeatureGateOSStreams is enabled and the cluster is not running SCOS or FCOS. +func IsFeatureEnabled(fgHandler common.FeatureGatesHandler) bool { + return fgHandler.Enabled(features.FeatureGateOSStreams) && !version.IsSCOS() && !version.IsFCOS() +} diff --git a/pkg/controller/osimagestream/helpers_test.go b/pkg/osimagestream/helpers_test.go similarity index 100% rename from pkg/controller/osimagestream/helpers_test.go rename to pkg/osimagestream/helpers_test.go diff --git a/pkg/controller/osimagestream/image_data.go b/pkg/osimagestream/image_data.go similarity index 100% rename from pkg/controller/osimagestream/image_data.go rename to pkg/osimagestream/image_data.go diff --git a/pkg/controller/osimagestream/image_data_test.go b/pkg/osimagestream/image_data_test.go similarity index 100% rename from pkg/controller/osimagestream/image_data_test.go rename to pkg/osimagestream/image_data_test.go diff --git a/pkg/controller/osimagestream/imagestream_provider.go b/pkg/osimagestream/imagestream_provider.go similarity index 100% rename from pkg/controller/osimagestream/imagestream_provider.go rename to pkg/osimagestream/imagestream_provider.go diff --git a/pkg/controller/osimagestream/imagestream_provider_test.go b/pkg/osimagestream/imagestream_provider_test.go similarity index 100% rename from pkg/controller/osimagestream/imagestream_provider_test.go rename to pkg/osimagestream/imagestream_provider_test.go diff --git a/pkg/controller/osimagestream/imagestream_source.go b/pkg/osimagestream/imagestream_source.go similarity index 100% rename from pkg/controller/osimagestream/imagestream_source.go rename to pkg/osimagestream/imagestream_source.go diff --git a/pkg/controller/osimagestream/imagestream_source_test.go b/pkg/osimagestream/imagestream_source_test.go similarity index 100% rename from pkg/controller/osimagestream/imagestream_source_test.go rename to pkg/osimagestream/imagestream_source_test.go diff --git a/pkg/controller/osimagestream/inspector.go b/pkg/osimagestream/inspector.go similarity index 65% rename from pkg/controller/osimagestream/inspector.go rename to pkg/osimagestream/inspector.go index 8838f7487d..27ea843b21 100644 --- a/pkg/controller/osimagestream/inspector.go +++ b/pkg/osimagestream/inspector.go @@ -3,6 +3,8 @@ package osimagestream import ( "archive/tar" "context" + "errors" + "net" "strings" "github.com/containers/common/pkg/retry" @@ -10,6 +12,44 @@ import ( "github.com/openshift/machine-config-operator/pkg/imageutils" ) +// isNetworkErrorRetryable checks if an error is a network-related error that should be retried. +// This handles DNS and timeout errors that may occur during cluster bootstrap. +func isNetworkErrorRetryable(err error) bool { + if err == nil { + return false + } + + // Check for net.DNSError (DNS lookup failures) + var dnsErr *net.DNSError + if errors.As(err, &dnsErr) { + return true + } + + // Check for timeout errors (net.Error with Timeout() == true) + var netErr net.Error + if errors.As(err, &netErr) && netErr.Timeout() { + return true + } + return false +} + +// newImageInspectionRetryOptions creates retry options suitable for image inspection operations. +// It includes special handling for DNS and network errors that may occur during cluster bootstrap. +func newImageInspectionRetryOptions() *retry.Options { + return &retry.Options{ + MaxRetry: 50, + IsErrorRetryable: func(err error) bool { + // Use the default retry logic first + if retry.IsErrorRetryable(err) { + return true + } + // Additionally check for network errors that may be wrapped in ways + // that don't match the default retry logic's type assertions + return isNetworkErrorRetryable(err) + }, + } +} + // ImagesInspector provides methods for inspecting container images and extracting their contents. type ImagesInspector interface { // Inspect retrieves metadata for one or more container images. @@ -29,9 +69,7 @@ func NewImagesInspector(sysCtx *types.SystemContext) *ImagesInspectorImpl { return &ImagesInspectorImpl{ sysCtx: sysCtx, bulkInspector: imageutils.NewBulkInspector(&imageutils.BulkInspectorOptions{ - RetryOpts: &retry.RetryOptions{ - MaxRetry: 2, - }, + RetryOpts: newImageInspectionRetryOptions(), Count: 5, FailOnErr: false, }), @@ -48,7 +86,7 @@ func (i *ImagesInspectorImpl) FetchImageFile(ctx context.Context, image, path st targetHeaderPath := strings.TrimLeft(path, "./") return imageutils.ReadImageFileContent(ctx, i.sysCtx, image, func(header *tar.Header) bool { return targetHeaderPath == strings.TrimLeft(header.Name, "./") - }) + }, newImageInspectionRetryOptions()) } // ImagesInspectorFactory creates ImagesInspector instances for different system contexts. diff --git a/pkg/controller/osimagestream/mocks_test.go b/pkg/osimagestream/mocks_test.go similarity index 100% rename from pkg/controller/osimagestream/mocks_test.go rename to pkg/osimagestream/mocks_test.go diff --git a/pkg/controller/osimagestream/osimagestream.go b/pkg/osimagestream/osimagestream.go similarity index 98% rename from pkg/controller/osimagestream/osimagestream.go rename to pkg/osimagestream/osimagestream.go index a5052cb990..a98ecb4ec3 100644 --- a/pkg/controller/osimagestream/osimagestream.go +++ b/pkg/osimagestream/osimagestream.go @@ -132,8 +132,7 @@ func BuildOSImageStreamFromSources(ctx context.Context, sources []StreamSource) ObjectMeta: metav1.ObjectMeta{ Name: ctrlcommon.ClusterInstanceNameOSImageStream, Annotations: map[string]string{ - ctrlcommon.ReleaseImageVersionAnnotationKey: version.Hash, - ctrlcommon.GeneratedByControllerVersionAnnotationKey: version.Hash, + ctrlcommon.ReleaseImageVersionAnnotationKey: version.Hash, }, }, Spec: &v1alpha1.OSImageStreamSpec{}, diff --git a/pkg/controller/osimagestream/osimagestream_test.go b/pkg/osimagestream/osimagestream_test.go similarity index 99% rename from pkg/controller/osimagestream/osimagestream_test.go rename to pkg/osimagestream/osimagestream_test.go index 139acafe40..f5c8cc3f17 100644 --- a/pkg/controller/osimagestream/osimagestream_test.go +++ b/pkg/osimagestream/osimagestream_test.go @@ -179,7 +179,6 @@ func TestBuildOSImageStreamFromSources(t *testing.T) { assert.NotNil(t, result) assert.Equal(t, "cluster", result.Name) assert.Equal(t, version.Hash, result.Annotations[ctrlcommon.ReleaseImageVersionAnnotationKey]) - assert.Equal(t, version.Hash, result.Annotations[ctrlcommon.GeneratedByControllerVersionAnnotationKey]) if tt.expectedDefault != "" { assert.Equal(t, tt.expectedDefault, result.Status.DefaultStream) } diff --git a/pkg/controller/osimagestream/urls_source.go b/pkg/osimagestream/urls_source.go similarity index 100% rename from pkg/controller/osimagestream/urls_source.go rename to pkg/osimagestream/urls_source.go diff --git a/pkg/controller/osimagestream/urls_source_test.go b/pkg/osimagestream/urls_source_test.go similarity index 100% rename from pkg/controller/osimagestream/urls_source_test.go rename to pkg/osimagestream/urls_source_test.go diff --git a/test/e2e-2of2/imageutils_test.go b/test/e2e-2of2/imageutils_test.go index 45b9fd4b72..de1b6fa44c 100644 --- a/test/e2e-2of2/imageutils_test.go +++ b/test/e2e-2of2/imageutils_test.go @@ -106,7 +106,7 @@ func TestReadImageFileContent(t *testing.T) { timedCtx, timedCtxCancelFn := context.WithTimeout(context.Background(), time.Minute) defer timedCtxCancelFn() - content, err := imageutils.ReadImageFileContent(timedCtx, sysContext.SysContext, clusterVersion.Status.Desired.Image, releaseManifestsMatcher) + content, err := imageutils.ReadImageFileContent(timedCtx, sysContext.SysContext, clusterVersion.Status.Desired.Image, releaseManifestsMatcher, nil) require.NoError(t, err) // Note: The test file is a file used in the MCO to fetch OSImageStreams, and it's diff --git a/test/e2e-2of2/osimagestream_test.go b/test/e2e-2of2/osimagestream_test.go index bdcd4558d2..0b46538251 100644 --- a/test/e2e-2of2/osimagestream_test.go +++ b/test/e2e-2of2/osimagestream_test.go @@ -6,7 +6,7 @@ import ( "github.com/openshift/machine-config-operator/internal/clients" ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common" - "github.com/openshift/machine-config-operator/pkg/controller/osimagestream" + "github.com/openshift/machine-config-operator/pkg/osimagestream" "github.com/openshift/machine-config-operator/test/framework" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" diff --git a/test/e2e-bootstrap/bootstrap_test.go b/test/e2e-bootstrap/bootstrap_test.go index 4d1e7b584a..5b505fe781 100644 --- a/test/e2e-bootstrap/bootstrap_test.go +++ b/test/e2e-bootstrap/bootstrap_test.go @@ -559,10 +559,13 @@ func createControllers(ctx *ctrlcommon.ControllerContext) []ctrlcommon.Controlle templatesDir, ctx.InformerFactory.Machineconfiguration().V1().ControllerConfigs(), ctx.InformerFactory.Machineconfiguration().V1().MachineConfigs(), + ctx.InformerFactory.Machineconfiguration().V1().MachineConfigPools(), + ctx.InformerFactory.Machineconfiguration().V1alpha1().OSImageStreams(), ctx.OpenShiftConfigKubeNamespacedInformerFactory.Core().V1().Secrets(), ctx.ConfigInformerFactory.Config().V1().APIServers(), ctx.ClientBuilder.KubeClientOrDie("template-controller"), ctx.ClientBuilder.MachineConfigClientOrDie("template-controller"), + ctx.FeatureGatesHandler, ), // Add all "sub-renderers here" kubeletconfig.New( @@ -570,6 +573,7 @@ func createControllers(ctx *ctrlcommon.ControllerContext) []ctrlcommon.Controlle ctx.InformerFactory.Machineconfiguration().V1().MachineConfigPools(), ctx.InformerFactory.Machineconfiguration().V1().ControllerConfigs(), ctx.InformerFactory.Machineconfiguration().V1().KubeletConfigs(), + ctx.InformerFactory.Machineconfiguration().V1alpha1().OSImageStreams(), ctx.ConfigInformerFactory.Config().V1().FeatureGates(), ctx.ConfigInformerFactory.Config().V1().Nodes(), ctx.ConfigInformerFactory.Config().V1().APIServers(), @@ -583,6 +587,7 @@ func createControllers(ctx *ctrlcommon.ControllerContext) []ctrlcommon.Controlle ctx.InformerFactory.Machineconfiguration().V1().MachineConfigPools(), ctx.InformerFactory.Machineconfiguration().V1().ControllerConfigs(), ctx.InformerFactory.Machineconfiguration().V1().ContainerRuntimeConfigs(), + ctx.InformerFactory.Machineconfiguration().V1alpha1().OSImageStreams(), ctx.ConfigInformerFactory.Config().V1().Images(), ctx.ConfigInformerFactory.Config().V1().ImageDigestMirrorSets(), ctx.ConfigInformerFactory.Config().V1().ImageTagMirrorSets(), @@ -603,6 +608,7 @@ func createControllers(ctx *ctrlcommon.ControllerContext) []ctrlcommon.Controlle 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, From 1ed69a85c6bd2ce0e7955e9f692443598525ecef Mon Sep 17 00:00:00 2001 From: Isabella Janssen Date: Thu, 4 Dec 2025 10:27:53 -0500 Subject: [PATCH 3/4] update os steam in status --- pkg/controller/common/constants.go | 2 + pkg/controller/node/status.go | 7 + pkg/controller/node/status_test.go | 154 ++++++++++++++++++ pkg/controller/render/hash.go | 3 +- pkg/controller/template/render.go | 2 + .../template/template_controller.go | 59 ++++++- pkg/operator/sync.go | 48 ++++-- 7 files changed, 252 insertions(+), 23 deletions(-) diff --git a/pkg/controller/common/constants.go b/pkg/controller/common/constants.go index d3e4c7578c..945f65b3ae 100644 --- a/pkg/controller/common/constants.go +++ b/pkg/controller/common/constants.go @@ -30,6 +30,8 @@ const ( // OSImageURLOverriddenKey is used to tag a rendered machineconfig when OSImageURL has been overridden from default using machineconfig OSImageURLOverriddenKey = "machineconfiguration.openshift.io/os-image-url-overridden" + RenderedMachineConfigPrefix = "rendered-" + // ControllerConfigName is the name of the ControllerConfig object that controllers use ControllerConfigName = "machine-config-controller" diff --git a/pkg/controller/node/status.go b/pkg/controller/node/status.go index fe748a4ebd..4ff0866c8f 100644 --- a/pkg/controller/node/status.go +++ b/pkg/controller/node/status.go @@ -250,6 +250,13 @@ func (ctrl *Controller) calculateStatus(mcns []*mcfgv1.MachineConfigNode, cconfi 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 ctrl.fgHandler.Enabled(features.FeatureGateOSStreams) { + 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) diff --git a/pkg/controller/node/status_test.go b/pkg/controller/node/status_test.go index 6ae7e06620..aa118b5692 100644 --- a/pkg/controller/node/status_test.go +++ b/pkg/controller/node/status_test.go @@ -437,6 +437,7 @@ func TestCalculateStatus(t *testing.T) { nodes []*corev1.Node currentConfig string paused bool + osStream mcfgv1.OSImageStreamReference verify func(mcfgv1.MachineConfigPoolStatus, *testing.T) }{{ name: "0 nodes updated, 0 nodes updating, 0 nodes degraded", @@ -884,6 +885,157 @@ func TestCalculateStatus(t *testing.T) { t.Fatalf("mismatch conddegraded.Status: got %s want: %s", got, want) } }, + }, { + name: "all nodes updated, OSStream defined in MCP Spec", + nodes: []*corev1.Node{ + helpers.NewNodeWithReady("node-0", machineConfigV0, machineConfigV0, corev1.ConditionTrue), + helpers.NewNodeWithReady("node-1", machineConfigV0, machineConfigV0, corev1.ConditionTrue), + helpers.NewNodeWithReady("node-2", machineConfigV0, machineConfigV0, corev1.ConditionTrue), + }, + currentConfig: machineConfigV0, + osStream: mcfgv1.OSImageStreamReference{ + Name: "rhel-10", + }, + verify: func(status mcfgv1.MachineConfigPoolStatus, t *testing.T) { + if got, want := status.MachineCount, int32(3); got != want { + t.Fatalf("mismatch MachineCount: got %d want: %d", got, want) + } + + if got, want := status.UpdatedMachineCount, int32(3); got != want { + t.Fatalf("mismatch UpdatedMachineCount: got %d want: %d", got, want) + } + + if got, want := status.ReadyMachineCount, int32(3); got != want { + t.Fatalf("mismatch ReadyMachineCount: got %d want: %d", got, want) + } + + if got, want := status.UnavailableMachineCount, int32(0); got != want { + t.Fatalf("mismatch UnavailableMachineCount: got %d want: %d", got, want) + } + + condupdated := apihelpers.GetMachineConfigPoolCondition(status, mcfgv1.MachineConfigPoolUpdated) + if condupdated == nil { + t.Fatal("updated condition not found") + } + + condupdating := apihelpers.GetMachineConfigPoolCondition(status, mcfgv1.MachineConfigPoolUpdating) + if condupdating == nil { + t.Fatal("updating condition not found") + } + + if got, want := condupdated.Status, corev1.ConditionTrue; got != want { + t.Fatalf("mismatch condupdated.Status: got %s want: %s", got, want) + } + + if got, want := condupdating.Status, corev1.ConditionFalse; got != want { + t.Fatalf("mismatch condupdating.Status: got %s want: %s", got, want) + } + + statusOSStreamName := status.OSImageStream.Name + if statusOSStreamName != "rhel-10" { + t.Fatal("OSImageStreamReference in MCP status not updated correctly") + } + }, + }, { + name: "some nodes still updating, OSStream defined in MCP Spec", + nodes: []*corev1.Node{ + helpers.NewNodeWithReady("node-0", machineConfigV0, machineConfigV1, corev1.ConditionTrue), + helpers.NewNodeWithReady("node-1", machineConfigV0, machineConfigV0, corev1.ConditionTrue), + helpers.NewNodeWithReady("node-2", machineConfigV0, machineConfigV0, corev1.ConditionTrue), + }, + currentConfig: machineConfigV1, + osStream: mcfgv1.OSImageStreamReference{ + Name: "rhel-10", + }, + verify: func(status mcfgv1.MachineConfigPoolStatus, t *testing.T) { + if got, want := status.MachineCount, int32(3); got != want { + t.Fatalf("mismatch MachineCount: got %d want: %d", got, want) + } + + if got, want := status.UpdatedMachineCount, int32(0); got != want { + t.Fatalf("mismatch UpdatedMachineCount: got %d want: %d", got, want) + } + + if got, want := status.ReadyMachineCount, int32(0); got != want { + t.Fatalf("mismatch ReadyMachineCount: got %d want: %d", got, want) + } + + if got, want := status.UnavailableMachineCount, int32(1); got != want { + t.Fatalf("mismatch UnavailableMachineCount: got %d want: %d", got, want) + } + + condupdated := apihelpers.GetMachineConfigPoolCondition(status, mcfgv1.MachineConfigPoolUpdated) + if condupdated == nil { + t.Fatal("updated condition not found") + } + + condupdating := apihelpers.GetMachineConfigPoolCondition(status, mcfgv1.MachineConfigPoolUpdating) + if condupdating == nil { + t.Fatal("updating condition not found") + } + + if got, want := condupdated.Status, corev1.ConditionFalse; got != want { + t.Fatalf("mismatch condupdated.Status: got %s want: %s", got, want) + } + + if got, want := condupdating.Status, corev1.ConditionTrue; got != want { + t.Fatalf("mismatch condupdating.Status: got %s want: %s", got, want) + } + + statusOSStreamName := status.OSImageStream.Name + if statusOSStreamName == "rhel-10" { + t.Fatal("OSImageStreamReference updated in MCP status, but should not be") + } + }, + }, { + name: "all nodes updated, OSStream removed from MCP Spec", + nodes: []*corev1.Node{ + helpers.NewNodeWithReady("node-0", machineConfigV0, machineConfigV0, corev1.ConditionTrue), + helpers.NewNodeWithReady("node-1", machineConfigV0, machineConfigV0, corev1.ConditionTrue), + helpers.NewNodeWithReady("node-2", machineConfigV0, machineConfigV0, corev1.ConditionTrue), + }, + currentConfig: machineConfigV0, + osStream: mcfgv1.OSImageStreamReference{}, + verify: func(status mcfgv1.MachineConfigPoolStatus, t *testing.T) { + if got, want := status.MachineCount, int32(3); got != want { + t.Fatalf("mismatch MachineCount: got %d want: %d", got, want) + } + + if got, want := status.UpdatedMachineCount, int32(3); got != want { + t.Fatalf("mismatch UpdatedMachineCount: got %d want: %d", got, want) + } + + if got, want := status.ReadyMachineCount, int32(3); got != want { + t.Fatalf("mismatch ReadyMachineCount: got %d want: %d", got, want) + } + + if got, want := status.UnavailableMachineCount, int32(0); got != want { + t.Fatalf("mismatch UnavailableMachineCount: got %d want: %d", got, want) + } + + condupdated := apihelpers.GetMachineConfigPoolCondition(status, mcfgv1.MachineConfigPoolUpdated) + if condupdated == nil { + t.Fatal("updated condition not found") + } + + condupdating := apihelpers.GetMachineConfigPoolCondition(status, mcfgv1.MachineConfigPoolUpdating) + if condupdating == nil { + t.Fatal("updating condition not found") + } + + if got, want := condupdated.Status, corev1.ConditionTrue; got != want { + t.Fatalf("mismatch condupdated.Status: got %s want: %s", got, want) + } + + if got, want := condupdating.Status, corev1.ConditionFalse; got != want { + t.Fatalf("mismatch condupdating.Status: got %s want: %s", got, want) + } + + statusOSStream := status.OSImageStream + if statusOSStream.Name != "" { + t.Fatal("OSImageStreamReference in MCP status not cleared correctly") + } + }, }} for idx, test := range tests { idx := idx @@ -894,12 +1046,14 @@ func TestCalculateStatus(t *testing.T) { Spec: mcfgv1.MachineConfigPoolSpec{ Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: test.currentConfig}}, Paused: test.paused, + OSImageStream: test.osStream, }, } f := newFixtureWithFeatureGates(t, []apicfgv1.FeatureGateName{ features.FeatureGateMachineConfigNodes, features.FeatureGatePinnedImages, + features.FeatureGateOSStreams, }, []apicfgv1.FeatureGateName{}, ) diff --git a/pkg/controller/render/hash.go b/pkg/controller/render/hash.go index 7f79d9224a..f936780b64 100644 --- a/pkg/controller/render/hash.go +++ b/pkg/controller/render/hash.go @@ -7,6 +7,7 @@ import ( "github.com/ghodss/yaml" mcfgv1 "github.com/openshift/api/machineconfiguration/v1" + ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common" ) var ( @@ -37,7 +38,7 @@ func getMachineConfigHashedName(pool *mcfgv1.MachineConfigPool, config *mcfgv1.M if err != nil { return "", err } - return fmt.Sprintf("rendered-%s-%x", pool.GetName(), h), nil + return fmt.Sprintf("%s%s-%x", ctrlcommon.RenderedMachineConfigPrefix, pool.GetName(), h), nil } func hashData(data []byte) ([]byte, error) { diff --git a/pkg/controller/template/render.go b/pkg/controller/template/render.go index b8763e6418..c4f2ff39a4 100644 --- a/pkg/controller/template/render.go +++ b/pkg/controller/template/render.go @@ -370,6 +370,8 @@ func generateMachineConfigForName(config *RenderConfig, role, name, templateDir, // MCs with the OSImageURL set are considered during the merge process. // Now the image URL is explicitly cleared to avoid confusion. Its content is never consumed. mcfg.Spec.OSImageURL = "" + // The same applies to the extensions image + mcfg.Spec.BaseOSExtensionsContainerImage = "" return mcfg, nil } diff --git a/pkg/controller/template/template_controller.go b/pkg/controller/template/template_controller.go index 4fda59c9ec..0f0e771308 100644 --- a/pkg/controller/template/template_controller.go +++ b/pkg/controller/template/template_controller.go @@ -6,33 +6,34 @@ import ( "crypto/x509" "encoding/json" "encoding/pem" + "errors" "fmt" "os" "path/filepath" "reflect" "sort" + "strings" + "sync" "time" + configv1 "github.com/openshift/api/config/v1" mcfgv1 "github.com/openshift/api/machineconfiguration/v1" + configinformersv1 "github.com/openshift/client-go/config/informers/externalversions/config/v1" + configlistersv1 "github.com/openshift/client-go/config/listers/config/v1" mcfgclientset "github.com/openshift/client-go/machineconfiguration/clientset/versioned" "github.com/openshift/client-go/machineconfiguration/clientset/versioned/scheme" mcfginformersv1 "github.com/openshift/client-go/machineconfiguration/informers/externalversions/machineconfiguration/v1" mcfglistersv1 "github.com/openshift/client-go/machineconfiguration/listers/machineconfiguration/v1" mcoResourceApply "github.com/openshift/machine-config-operator/lib/resourceapply" ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common" - "k8s.io/klog/v2" - - configv1 "github.com/openshift/api/config/v1" - configinformersv1 "github.com/openshift/client-go/config/informers/externalversions/config/v1" - configlistersv1 "github.com/openshift/client-go/config/listers/config/v1" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/wait" coreinformersv1 "k8s.io/client-go/informers/core/v1" clientset "k8s.io/client-go/kubernetes" corev1clientset "k8s.io/client-go/kubernetes/typed/core/v1" + "k8s.io/klog/v2" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -74,6 +75,8 @@ type Controller struct { secretsInformerSynced cache.InformerSynced queue workqueue.TypedRateLimitingInterface[string] + + urlClearOnce sync.Once } // New returns a new template controller. @@ -588,7 +591,7 @@ func (ctrl *Controller) syncControllerConfig(key string) error { return err } controllerconfig, err := ctrl.ccLister.Get(name) - if errors.IsNotFound(err) { + if apierrors.IsNotFound(err) { klog.V(2).Infof("ControllerConfig %v has been deleted", key) return nil } @@ -633,6 +636,15 @@ func (ctrl *Controller) syncControllerConfig(key string) error { return ctrl.syncFailingStatus(cfg, err) } + // TODO: To be removed when 4.22 is EOL + // Since 4.22 templated/generated (non-rendered) MCs no longer set the + // OS and Extensions URLs and if given, the render controller ignores their + // values. To improve UX we remove the URLs from existing MCs once. + if clearErr := ctrl.clearImageURLs(); err == nil && apiServer != nil { + // Best effort, do not fail + klog.Warningf("Clearing MCs URLs has failed: %v", clearErr) + } + mcs, err := getMachineConfigsForControllerConfig(ctrl.templatesDir, cfg, clusterPullSecretRaw, apiServer) if err != nil { return ctrl.syncFailingStatus(cfg, err) @@ -652,6 +664,39 @@ func (ctrl *Controller) syncControllerConfig(key string) error { return ctrl.syncCompletedStatus(cfg) } +// clearImageURLs clears OSImageURL and BaseOSExtensionsContainerImage from template-generated +// MachineConfigs. This is a one-time migration operation to remove these fields from generated +// MachineConfigs, as they should only be set on rendered or user-provided MachineConfigs. +// The function uses sync.Once to ensure it runs only once per controller lifecycle. +func (ctrl *Controller) clearImageURLs() error { + var err error + ctrl.urlClearOnce.Do(func() { + var mcList *mcfgv1.MachineConfigList + mcList, err = ctrl.client.MachineconfigurationV1().MachineConfigs().List(context.TODO(), metav1.ListOptions{}) + if err != nil { + return + } + for _, mc := range mcList.Items { + if mc.Annotations[ctrlcommon.GeneratedByControllerVersionAnnotationKey] == "" || strings.HasPrefix(mc.Name, ctrlcommon.RenderedMachineConfigPrefix) { + // It's a rendered MC or a user provided one. + // This update code only works with templated/generated MCs + continue + } + + if mc.Spec.OSImageURL != "" || mc.Spec.BaseOSExtensionsContainerImage != "" { + mc.Spec.OSImageURL = "" + mc.Spec.BaseOSExtensionsContainerImage = "" + + if _, updateErr := ctrl.client.MachineconfigurationV1().MachineConfigs().Update(context.TODO(), &mc, metav1.UpdateOptions{}); updateErr != nil { + err = errors.Join(err, updateErr) + } + klog.Infof("Removed old imageURLs from MachineConfig %s", mc.Name) + } + } + }) + return err +} + func getMachineConfigsForControllerConfig(templatesDir string, config *mcfgv1.ControllerConfig, clusterPullSecretRaw []byte, apiServer *configv1.APIServer) ([]*mcfgv1.MachineConfig, error) { buf := &bytes.Buffer{} if err := json.Compact(buf, clusterPullSecretRaw); err != nil { diff --git a/pkg/operator/sync.go b/pkg/operator/sync.go index 5a8d50b2f8..aeb8c2dcd9 100644 --- a/pkg/operator/sync.go +++ b/pkg/operator/sync.go @@ -1299,24 +1299,22 @@ func (optr *Operator) syncOSImageStream(_ *renderConfig, _ *configv1.ClusterOper klog.V(4).Info("OSImageStream sync complete") }() - // Check if the feature is enabled - if !osimagestream.IsFeatureEnabled(optr.fgHandler) { - klog.V(4).Info("OSImageStream feature is not enabled, skipping sync") - return nil - } - - // Get the existing OSImageStream if it exists - existingOSImageStream, err := optr.getExistingOSImageStream() - if err != nil { + // This sync runs once per version. Before performing the streams fetching + // process, that takes time as it requires inspecting images, ensure this function + // needs to build the stream. + existingOSImageStream, updateRequired, err := optr.isOSImageStreamBuildRequired() + if !updateRequired || err != nil { return err } - // Check if an update is needed - if !osImageStreamRequiresUpdate(existingOSImageStream) { - klog.V(4).Info("OSImageStream is already up-to-date, skipping sync") - return nil - } + // If the code reaches this point the OSImageStream CR is not + // present (new cluster) or it's out-dated (cluster update). + // Build the new OSImageStream and push it. + return optr.buildOSImageStream(existingOSImageStream) + +} +func (optr *Operator) buildOSImageStream(existingOSImageStream *v1alpha1.OSImageStream) error { klog.Info("Starting building of the OSImageStream instance") // Get the release payload image from ClusterVersion @@ -1382,10 +1380,30 @@ func (optr *Operator) syncOSImageStream(_ *renderConfig, _ *configv1.ClusterOper klog.Infof("OSImageStream synced successfully. Available streams: %s. Default stream: %s", osimagestream.GetStreamSetsNames(updateOSImageStream.Status.AvailableStreams), updateOSImageStream.Status.DefaultStream) - return nil } +func (optr *Operator) isOSImageStreamBuildRequired() (*v1alpha1.OSImageStream, bool, error) { + // Check if the feature is enabled + if !osimagestream.IsFeatureEnabled(optr.fgHandler) { + klog.V(4).Info("OSImageStream feature is not enabled, skipping sync") + return nil, false, nil + } + + // Get the existing OSImageStream if it exists + existingOSImageStream, err := optr.getExistingOSImageStream() + if err != nil { + return nil, true, err + } + + // Check if an update is needed + if !osImageStreamRequiresUpdate(existingOSImageStream) { + klog.V(4).Info("OSImageStream is already up-to-date, skipping sync") + return nil, false, nil + } + return existingOSImageStream, true, nil +} + // buildMinimalControllerConfigForOSImageStream builds a minimal ControllerConfig with just the image registry certs // needed for OSImageStream to inspect images. This is necessary because OSImageStream must run before RenderConfig. func (optr *Operator) buildMinimalControllerConfigForOSImageStream() (*mcfgv1.ControllerConfig, error) { From ce5b235000fef5cbb10ada82d406865d37f85fd9 Mon Sep 17 00:00:00 2001 From: Zack Zlotnik Date: Mon, 15 Dec 2025 10:20:14 -0500 Subject: [PATCH 4/4] skip OSImageStream reconciliation in OKD --- pkg/operator/osimagestream_ocp.go | 169 ++++++++++++++++++++++++++++++ pkg/operator/osimagestream_okd.go | 13 +++ pkg/operator/sync.go | 151 -------------------------- 3 files changed, 182 insertions(+), 151 deletions(-) create mode 100644 pkg/operator/osimagestream_ocp.go create mode 100644 pkg/operator/osimagestream_okd.go diff --git a/pkg/operator/osimagestream_ocp.go b/pkg/operator/osimagestream_ocp.go new file mode 100644 index 0000000000..e86852d80a --- /dev/null +++ b/pkg/operator/osimagestream_ocp.go @@ -0,0 +1,169 @@ +//go:build !fcos && !scos + +package operator + +import ( + "context" + "fmt" + "time" + + configv1 "github.com/openshift/api/config/v1" + mcfgv1 "github.com/openshift/api/machineconfiguration/v1" + "github.com/openshift/api/machineconfiguration/v1alpha1" + ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common" + "github.com/openshift/machine-config-operator/pkg/osimagestream" + "github.com/openshift/machine-config-operator/pkg/version" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/klog/v2" +) + +func (optr *Operator) syncOSImageStream(_ *renderConfig, _ *configv1.ClusterOperator) error { + klog.V(4).Info("OSImageStream sync started") + defer func() { + klog.V(4).Info("OSImageStream sync complete") + }() + + // This sync runs once per version. Before performing the streams fetching + // process, that takes time as it requires inspecting images, ensure this function + // needs to build the stream. + existingOSImageStream, updateRequired, err := optr.isOSImageStreamBuildRequired() + if !updateRequired || err != nil { + return err + } + + // If the code reaches this point the OSImageStream CR is not + // present (new cluster) or it's out-dated (cluster update). + // Build the new OSImageStream and push it. + return optr.buildOSImageStream(existingOSImageStream) + +} + +func (optr *Operator) buildOSImageStream(existingOSImageStream *v1alpha1.OSImageStream) error { + klog.Info("Starting building of the OSImageStream instance") + + // Get the release payload image from ClusterVersion + image, err := osimagestream.GetReleasePayloadImage(optr.clusterVersionLister) + if err != nil { + return fmt.Errorf("error getting the Release Image digest from the ClusterVersion for OSImageStream sync: %w", err) + } + + // Get the cluster pull secret from well-known location + clusterPullSecret, err := optr.kubeClient.CoreV1().Secrets("openshift-config").Get(context.TODO(), "pull-secret", metav1.GetOptions{}) + if err != nil { + return fmt.Errorf("could not get the cluster PullSecret for OSImageStream sync: %w", err) + } + + // Build a minimal ControllerConfig with image registry certs + // We can't use renderConfig (it runs after us) so we build the cert data directly + minimalCC, err := optr.buildMinimalControllerConfigForOSImageStream() + if err != nil { + return fmt.Errorf("could not build minimal ControllerConfig for OSImageStream: %w", err) + } + + // Build the OSImageStream using the default factory + // Use a longer timeout to account for DNS/network delays during cluster bootstrap + buildCtx, buildCancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer buildCancel() + + imageStreamFactory := osimagestream.NewDefaultStreamSourceFactory(optr.mcoCmLister, &osimagestream.DefaultImagesInspectorFactory{}) + osImageStream, err := osimagestream.BuildOsImageStreamRuntime(buildCtx, clusterPullSecret, minimalCC, image, imageStreamFactory) + if err != nil { + return fmt.Errorf("error building the OSImageStream: %w", err) + } + + // Create or update the OSImageStream resource + var updateOSImageStream *v1alpha1.OSImageStream + if existingOSImageStream == nil { + klog.V(4).Info("Creating OSImageStream singleton instance") + updateOSImageStream, err = optr.client.MachineconfigurationV1alpha1().OSImageStreams().Create(context.TODO(), osImageStream, metav1.CreateOptions{}) + if err != nil { + return fmt.Errorf("error creating the OSImageStream: %w", err) + } + klog.Infof("Created OSImageStream with %d available streams, default stream: %s", + len(osImageStream.Status.AvailableStreams), osImageStream.Status.DefaultStream) + } else { + oldVersion := existingOSImageStream.Annotations[ctrlcommon.ReleaseImageVersionAnnotationKey] + klog.V(4).Infof("Updating OSImageStream (previous version: %s, new version: %s)", oldVersion, version.Hash) + // Update metadata/spec first (mainly for annotations) + existingOSImageStream.ObjectMeta.Annotations = osImageStream.ObjectMeta.Annotations + updateOSImageStream, err = optr.client.MachineconfigurationV1alpha1().OSImageStreams().Update(context.TODO(), existingOSImageStream, metav1.UpdateOptions{}) + if err != nil { + return fmt.Errorf("error updating the OSImageStream: %w", err) + } + } + + // Update the status subresource (both for newly created and updated resources) + updateOSImageStream.Status = osImageStream.Status + if _, err = optr.client. + MachineconfigurationV1alpha1(). + OSImageStreams(). + UpdateStatus(context.TODO(), updateOSImageStream, metav1.UpdateOptions{}); err != nil { + return fmt.Errorf("error updating the OSImageStream status: %w", err) + } + + klog.Infof("OSImageStream synced successfully. Available streams: %s. Default stream: %s", + osimagestream.GetStreamSetsNames(updateOSImageStream.Status.AvailableStreams), + updateOSImageStream.Status.DefaultStream) + return nil +} + +func (optr *Operator) isOSImageStreamBuildRequired() (*v1alpha1.OSImageStream, bool, error) { + // Check if the feature is enabled + if !osimagestream.IsFeatureEnabled(optr.fgHandler) { + klog.V(4).Info("OSImageStream feature is not enabled, skipping sync") + return nil, false, nil + } + + // Get the existing OSImageStream if it exists + existingOSImageStream, err := optr.getExistingOSImageStream() + if err != nil { + return nil, true, err + } + + // Check if an update is needed + if !osImageStreamRequiresUpdate(existingOSImageStream) { + klog.V(4).Info("OSImageStream is already up-to-date, skipping sync") + return nil, false, nil + } + return existingOSImageStream, true, nil +} + +// buildMinimalControllerConfigForOSImageStream builds a minimal ControllerConfig with just the image registry certs +// needed for OSImageStream to inspect images. This is necessary because OSImageStream must run before RenderConfig. +func (optr *Operator) buildMinimalControllerConfigForOSImageStream() (*mcfgv1.ControllerConfig, error) { + imgRegistryData, imgRegistryUsrData, err := optr.getImageRegistryBundles() + if err != nil { + return nil, fmt.Errorf("could not get image registry bundles: %w", err) + } + + return &mcfgv1.ControllerConfig{ + Spec: mcfgv1.ControllerConfigSpec{ + ImageRegistryBundleData: imgRegistryData, + ImageRegistryBundleUserData: imgRegistryUsrData, + }, + }, nil +} + +// getExistingOSImageStream retrieves the existing OSImageStream from the lister. +// Returns nil if the OSImageStream does not exist. +func (optr *Operator) getExistingOSImageStream() (*v1alpha1.OSImageStream, error) { + osImageStream, err := optr.osImageStreamLister.Get(ctrlcommon.ClusterInstanceNameOSImageStream) + if err != nil { + if !apierrors.IsNotFound(err) { + return nil, fmt.Errorf("failed to retrieve existing OSImageStream: %v", err) + } + return nil, nil + } + return osImageStream, nil +} + +// osImageStreamRequiresUpdate checks if the OSImageStream needs to be created or updated. +// Returns true if osImageStream is nil or if its version annotation doesn't match the current version. +func osImageStreamRequiresUpdate(osImageStream *v1alpha1.OSImageStream) bool { + if osImageStream == nil { + return true + } + releaseVersion, ok := osImageStream.Annotations[ctrlcommon.ReleaseImageVersionAnnotationKey] + return !ok || releaseVersion != version.Hash +} diff --git a/pkg/operator/osimagestream_okd.go b/pkg/operator/osimagestream_okd.go new file mode 100644 index 0000000000..af41f6dda6 --- /dev/null +++ b/pkg/operator/osimagestream_okd.go @@ -0,0 +1,13 @@ +//go:build fcos || scos + +package operator + +import ( + configv1 "github.com/openshift/api/config/v1" + "k8s.io/klog/v2" +) + +func (optr *Operator) syncOSImageStream(_ *renderConfig, _ *configv1.ClusterOperator) error { + klog.V(4).Info("OSImageStream sync skipped") + return nil +} diff --git a/pkg/operator/sync.go b/pkg/operator/sync.go index aeb8c2dcd9..fef20d5de6 100644 --- a/pkg/operator/sync.go +++ b/pkg/operator/sync.go @@ -38,7 +38,6 @@ import ( "github.com/openshift/api/annotations" configv1 "github.com/openshift/api/config/v1" mcfgv1 "github.com/openshift/api/machineconfiguration/v1" - "github.com/openshift/api/machineconfiguration/v1alpha1" opv1 "github.com/openshift/api/operator/v1" features "github.com/openshift/api/features" @@ -1293,156 +1292,6 @@ func (optr *Operator) reconcileMachineOSBuilder(mob *appsv1.Deployment) error { return nil } -func (optr *Operator) syncOSImageStream(_ *renderConfig, _ *configv1.ClusterOperator) error { - klog.V(4).Info("OSImageStream sync started") - defer func() { - klog.V(4).Info("OSImageStream sync complete") - }() - - // This sync runs once per version. Before performing the streams fetching - // process, that takes time as it requires inspecting images, ensure this function - // needs to build the stream. - existingOSImageStream, updateRequired, err := optr.isOSImageStreamBuildRequired() - if !updateRequired || err != nil { - return err - } - - // If the code reaches this point the OSImageStream CR is not - // present (new cluster) or it's out-dated (cluster update). - // Build the new OSImageStream and push it. - return optr.buildOSImageStream(existingOSImageStream) - -} - -func (optr *Operator) buildOSImageStream(existingOSImageStream *v1alpha1.OSImageStream) error { - klog.Info("Starting building of the OSImageStream instance") - - // Get the release payload image from ClusterVersion - image, err := osimagestream.GetReleasePayloadImage(optr.clusterVersionLister) - if err != nil { - return fmt.Errorf("error getting the Release Image digest from the ClusterVersion for OSImageStream sync: %w", err) - } - - // Get the cluster pull secret from well-known location - clusterPullSecret, err := optr.kubeClient.CoreV1().Secrets("openshift-config").Get(context.TODO(), "pull-secret", metav1.GetOptions{}) - if err != nil { - return fmt.Errorf("could not get the cluster PullSecret for OSImageStream sync: %w", err) - } - - // Build a minimal ControllerConfig with image registry certs - // We can't use renderConfig (it runs after us) so we build the cert data directly - minimalCC, err := optr.buildMinimalControllerConfigForOSImageStream() - if err != nil { - return fmt.Errorf("could not build minimal ControllerConfig for OSImageStream: %w", err) - } - - // Build the OSImageStream using the default factory - // Use a longer timeout to account for DNS/network delays during cluster bootstrap - buildCtx, buildCancel := context.WithTimeout(context.Background(), 5*time.Minute) - defer buildCancel() - - imageStreamFactory := osimagestream.NewDefaultStreamSourceFactory(optr.mcoCmLister, &osimagestream.DefaultImagesInspectorFactory{}) - osImageStream, err := osimagestream.BuildOsImageStreamRuntime(buildCtx, clusterPullSecret, minimalCC, image, imageStreamFactory) - if err != nil { - return fmt.Errorf("error building the OSImageStream: %w", err) - } - - // Create or update the OSImageStream resource - var updateOSImageStream *v1alpha1.OSImageStream - if existingOSImageStream == nil { - klog.V(4).Info("Creating OSImageStream singleton instance") - updateOSImageStream, err = optr.client.MachineconfigurationV1alpha1().OSImageStreams().Create(context.TODO(), osImageStream, metav1.CreateOptions{}) - if err != nil { - return fmt.Errorf("error creating the OSImageStream: %w", err) - } - klog.Infof("Created OSImageStream with %d available streams, default stream: %s", - len(osImageStream.Status.AvailableStreams), osImageStream.Status.DefaultStream) - } else { - oldVersion := existingOSImageStream.Annotations[ctrlcommon.ReleaseImageVersionAnnotationKey] - klog.V(4).Infof("Updating OSImageStream (previous version: %s, new version: %s)", oldVersion, version.Hash) - // Update metadata/spec first (mainly for annotations) - existingOSImageStream.ObjectMeta.Annotations = osImageStream.ObjectMeta.Annotations - updateOSImageStream, err = optr.client.MachineconfigurationV1alpha1().OSImageStreams().Update(context.TODO(), existingOSImageStream, metav1.UpdateOptions{}) - if err != nil { - return fmt.Errorf("error updating the OSImageStream: %w", err) - } - } - - // Update the status subresource (both for newly created and updated resources) - updateOSImageStream.Status = osImageStream.Status - if _, err = optr.client. - MachineconfigurationV1alpha1(). - OSImageStreams(). - UpdateStatus(context.TODO(), updateOSImageStream, metav1.UpdateOptions{}); err != nil { - return fmt.Errorf("error updating the OSImageStream status: %w", err) - } - - klog.Infof("OSImageStream synced successfully. Available streams: %s. Default stream: %s", - osimagestream.GetStreamSetsNames(updateOSImageStream.Status.AvailableStreams), - updateOSImageStream.Status.DefaultStream) - return nil -} - -func (optr *Operator) isOSImageStreamBuildRequired() (*v1alpha1.OSImageStream, bool, error) { - // Check if the feature is enabled - if !osimagestream.IsFeatureEnabled(optr.fgHandler) { - klog.V(4).Info("OSImageStream feature is not enabled, skipping sync") - return nil, false, nil - } - - // Get the existing OSImageStream if it exists - existingOSImageStream, err := optr.getExistingOSImageStream() - if err != nil { - return nil, true, err - } - - // Check if an update is needed - if !osImageStreamRequiresUpdate(existingOSImageStream) { - klog.V(4).Info("OSImageStream is already up-to-date, skipping sync") - return nil, false, nil - } - return existingOSImageStream, true, nil -} - -// buildMinimalControllerConfigForOSImageStream builds a minimal ControllerConfig with just the image registry certs -// needed for OSImageStream to inspect images. This is necessary because OSImageStream must run before RenderConfig. -func (optr *Operator) buildMinimalControllerConfigForOSImageStream() (*mcfgv1.ControllerConfig, error) { - imgRegistryData, imgRegistryUsrData, err := optr.getImageRegistryBundles() - if err != nil { - return nil, fmt.Errorf("could not get image registry bundles: %w", err) - } - - return &mcfgv1.ControllerConfig{ - Spec: mcfgv1.ControllerConfigSpec{ - ImageRegistryBundleData: imgRegistryData, - ImageRegistryBundleUserData: imgRegistryUsrData, - }, - }, nil -} - -// getExistingOSImageStream retrieves the existing OSImageStream from the lister. -// Returns nil if the OSImageStream does not exist. -func (optr *Operator) getExistingOSImageStream() (*v1alpha1.OSImageStream, error) { - osImageStream, err := optr.osImageStreamLister.Get(ctrlcommon.ClusterInstanceNameOSImageStream) - if err != nil { - if !apierrors.IsNotFound(err) { - return nil, fmt.Errorf("failed to retrieve existing OSImageStream: %v", err) - } - return nil, nil - } - return osImageStream, nil -} - -// osImageStreamRequiresUpdate checks if the OSImageStream needs to be created or updated. -// Returns true if osImageStream is nil or if its version annotation doesn't match the current version. -func osImageStreamRequiresUpdate(osImageStream *v1alpha1.OSImageStream) bool { - if osImageStream == nil { - return true - } - releaseVersion, ok := osImageStream.Annotations[ctrlcommon.ReleaseImageVersionAnnotationKey] - return !ok || releaseVersion != version.Hash -} - // Validate that the nodes part of layered pools are coreos based func (optr *Operator) validateLayeredPoolNodes(layeredMCPs []*mcfgv1.MachineConfigPool) error { nodes, err := optr.GetAllManagedNodes(layeredMCPs)