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/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/common/helpers.go b/pkg/controller/common/helpers.go index c8ed0ac473..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,16 +188,24 @@ 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] != "" { + continue + } if cfg.Spec.OSImageURL != "" { osImageURL = cfg.Spec.OSImageURL } } // 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] != "" { + continue + } if cfg.Spec.BaseOSExtensionsContainerImage != "" { baseOSExtensionsContainerImage = cfg.Spec.BaseOSExtensionsContainerImage } @@ -1035,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/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/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/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/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 35268a8d01..c4f2ff39a4 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) } @@ -361,13 +365,13 @@ 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 = "" + // 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/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/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 5cb26773d5..fef20d5de6 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" @@ -256,6 +257,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 +402,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 +413,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 +569,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 +612,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 @@ -1730,7 +1742,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 +1905,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,