From eeed4fad96c94c7e36b7f5f9c696fe6b69db1d03 Mon Sep 17 00:00:00 2001 From: Petr Muller Date: Wed, 7 May 2025 20:11:46 +0200 Subject: [PATCH 1/4] refactor: unify names in informer-related code The code to set up informers is messy so this change attempts to make it more consistent: - Alias all informer-related imports to `somethinginformers` - Everything made by `NewSharedInformerFactory` methods is now called `somethingInformerFactory` - Name factories in `Context` consistently: `SomethingFactory` - Document the purpose of individual factories in `Context` - Reorder code into clusters that deal with individual factory types --- pkg/start/start.go | 78 ++++++++++++++++++++++++++-------------------- 1 file changed, 44 insertions(+), 34 deletions(-) diff --git a/pkg/start/start.go b/pkg/start/start.go index 9b2af53c7..6e37403be 100644 --- a/pkg/start/start.go +++ b/pkg/start/start.go @@ -13,7 +13,6 @@ import ( "time" "github.com/google/uuid" - configlistersv1 "github.com/openshift/client-go/config/listers/config/v1" v1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -21,7 +20,7 @@ import ( utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/wait" - "k8s.io/client-go/informers" + coreinformers "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/scheme" coreclientsetv1 "k8s.io/client-go/kubernetes/typed/core/v1" @@ -35,9 +34,10 @@ import ( configv1 "github.com/openshift/api/config/v1" clientset "github.com/openshift/client-go/config/clientset/versioned" - "github.com/openshift/client-go/config/informers/externalversions" + configinformers "github.com/openshift/client-go/config/informers/externalversions" + configlistersv1 "github.com/openshift/client-go/config/listers/config/v1" operatorclientset "github.com/openshift/client-go/operator/clientset/versioned" - operatorexternalversions "github.com/openshift/client-go/operator/informers/externalversions" + operatorinformers "github.com/openshift/client-go/operator/informers/externalversions" "github.com/openshift/library-go/pkg/config/clusterstatus" libgoleaderelection "github.com/openshift/library-go/pkg/config/leaderelection" @@ -255,13 +255,13 @@ func (o *Options) run(ctx context.Context, controllerCtx *Context, lock resource informersDone := postMainContext.Done() // FIXME: would be nice if there was a way to collect these. - controllerCtx.CVInformerFactory.Start(informersDone) + controllerCtx.ClusterVersionInformerFactory.Start(informersDone) controllerCtx.OpenshiftConfigInformerFactory.Start(informersDone) controllerCtx.OpenshiftConfigManagedInformerFactory.Start(informersDone) - controllerCtx.InformerFactory.Start(informersDone) + controllerCtx.ConfigInformerFactory.Start(informersDone) controllerCtx.OperatorInformerFactory.Start(informersDone) - allSynced := controllerCtx.CVInformerFactory.WaitForCacheSync(informersDone) + allSynced := controllerCtx.ClusterVersionInformerFactory.WaitForCacheSync(informersDone) for _, synced := range allSynced { if !synced { klog.Fatalf("Caches never synchronized: %v", postMainContext.Err()) @@ -489,11 +489,21 @@ type Context struct { AutoUpdate *autoupdate.Controller StopOnFeatureGateChange *featuregates.ChangeStopper - CVInformerFactory externalversions.SharedInformerFactory - OpenshiftConfigInformerFactory informers.SharedInformerFactory - OpenshiftConfigManagedInformerFactory informers.SharedInformerFactory - InformerFactory externalversions.SharedInformerFactory - OperatorInformerFactory operatorexternalversions.SharedInformerFactory + // ClusterVersionInformerFactory should be used to get informers / listers for code that works with ClusterVersion resource + // singleton in the cluster. + ClusterVersionInformerFactory configinformers.SharedInformerFactory + // ConfigInformerFactory should be used to get informers / listers for code that works with resources from the + // config.openshift.io group, _except_ the ClusterVersion resource singleton. + ConfigInformerFactory configinformers.SharedInformerFactory + // OpenshiftConfigManagedInformerFactory should be used to get informers / listers for code that works with core k8s + // resources in the openshift-config namespace. + OpenshiftConfigInformerFactory coreinformers.SharedInformerFactory + // OpenshiftConfigManagedInformerFactory should be used to get informers / listers for code that works with core k8s + // resources in the openshift-config-managed namespace. + OpenshiftConfigManagedInformerFactory coreinformers.SharedInformerFactory + // OperatorInformerFactory should be used to get informers / listers for code that works with resources from the + // operator.openshift.io group + OperatorInformerFactory operatorinformers.SharedInformerFactory fgLister configlistersv1.FeatureGateLister } @@ -503,20 +513,20 @@ type Context struct { func (o *Options) NewControllerContext(cb *ClientBuilder) (*Context, error) { client := cb.ClientOrDie("shared-informer") kubeClient := cb.KubeClientOrDie(internal.ConfigNamespace, useProtobuf) - operatorClient := cb.OperatorClientOrDie("operator-client") + openshiftConfigInformerFactory := coreinformers.NewSharedInformerFactoryWithOptions(kubeClient, resyncPeriod(o.ResyncInterval), coreinformers.WithNamespace(internal.ConfigNamespace)) + openshiftConfigManagedInformerFactory := coreinformers.NewSharedInformerFactoryWithOptions(kubeClient, resyncPeriod(o.ResyncInterval), coreinformers.WithNamespace(internal.ConfigManagedNamespace)) - cvInformer := externalversions.NewFilteredSharedInformerFactory(client, resyncPeriod(o.ResyncInterval), "", func(opts *metav1.ListOptions) { + clusterVersionConfigInformerFactory := configinformers.NewFilteredSharedInformerFactory(client, resyncPeriod(o.ResyncInterval), "", func(opts *metav1.ListOptions) { opts.FieldSelector = fmt.Sprintf("metadata.name=%s", o.Name) }) - openshiftConfigInformer := informers.NewSharedInformerFactoryWithOptions(kubeClient, resyncPeriod(o.ResyncInterval), informers.WithNamespace(internal.ConfigNamespace)) - openshiftConfigManagedInformer := informers.NewSharedInformerFactoryWithOptions(kubeClient, resyncPeriod(o.ResyncInterval), informers.WithNamespace(internal.ConfigManagedNamespace)) - sharedInformers := externalversions.NewSharedInformerFactory(client, resyncPeriod(o.ResyncInterval)) - operatorInformerFactory := operatorexternalversions.NewSharedInformerFactoryWithOptions(operatorClient, o.ResyncInterval, - operatorexternalversions.WithTweakListOptions(func(opts *metav1.ListOptions) { - opts.FieldSelector = fields.OneTermEqualSelector("metadata.name", configuration.ClusterVersionOperatorConfigurationName).String() - })) + configInformerFactory := configinformers.NewSharedInformerFactory(client, resyncPeriod(o.ResyncInterval)) - coInformer := sharedInformers.Config().V1().ClusterOperators() + operatorClient := cb.OperatorClientOrDie("operator-client") + filterByName := func(opts *metav1.ListOptions) { + opts.FieldSelector = fields.OneTermEqualSelector("metadata.name", configuration.ClusterVersionOperatorConfigurationName).String() + } + operatorInformerFactory := operatorinformers.NewSharedInformerFactoryWithOptions(operatorClient, o.ResyncInterval, operatorinformers.WithTweakListOptions(filterByName)) + coInformer := configInformerFactory.Config().V1().ClusterOperators() cvoKubeClient := cb.KubeClientOrDie(o.Namespace, useProtobuf) o.PromQLTarget.KubeClient = cvoKubeClient @@ -527,11 +537,11 @@ func (o *Options) NewControllerContext(cb *ClientBuilder) (*Context, error) { o.ReleaseImage, o.PayloadOverride, resyncPeriod(o.ResyncInterval), - cvInformer.Config().V1().ClusterVersions(), + clusterVersionConfigInformerFactory.Config().V1().ClusterVersions(), coInformer, - openshiftConfigInformer.Core().V1().ConfigMaps(), - openshiftConfigManagedInformer.Core().V1().ConfigMaps(), - sharedInformers.Config().V1().Proxies(), + openshiftConfigInformerFactory.Core().V1().ConfigMaps(), + openshiftConfigManagedInformerFactory.Core().V1().ConfigMaps(), + configInformerFactory.Config().V1().Proxies(), operatorInformerFactory, cb.ClientOrDie(o.Namespace), cvoKubeClient, @@ -548,28 +558,28 @@ func (o *Options) NewControllerContext(cb *ClientBuilder) (*Context, error) { return nil, err } - featureChangeStopper, err := featuregates.NewChangeStopper(sharedInformers.Config().V1().FeatureGates()) + featureChangeStopper, err := featuregates.NewChangeStopper(configInformerFactory.Config().V1().FeatureGates()) if err != nil { return nil, err } ctx := &Context{ - CVInformerFactory: cvInformer, - OpenshiftConfigInformerFactory: openshiftConfigInformer, - OpenshiftConfigManagedInformerFactory: openshiftConfigManagedInformer, - InformerFactory: sharedInformers, + ClusterVersionInformerFactory: clusterVersionConfigInformerFactory, + ConfigInformerFactory: configInformerFactory, + OpenshiftConfigInformerFactory: openshiftConfigInformerFactory, + OpenshiftConfigManagedInformerFactory: openshiftConfigManagedInformerFactory, OperatorInformerFactory: operatorInformerFactory, CVO: cvo, StopOnFeatureGateChange: featureChangeStopper, - fgLister: sharedInformers.Config().V1().FeatureGates().Lister(), + fgLister: configInformerFactory.Config().V1().FeatureGates().Lister(), } if o.EnableAutoUpdate { ctx.AutoUpdate, err = autoupdate.New( o.Namespace, o.Name, - cvInformer.Config().V1().ClusterVersions(), - sharedInformers.Config().V1().ClusterOperators(), + clusterVersionConfigInformerFactory.Config().V1().ClusterVersions(), + configInformerFactory.Config().V1().ClusterOperators(), cb.ClientOrDie(o.Namespace), cb.KubeClientOrDie(o.Namespace), ) From 6d46b8f3022a312ba65b0eda496e914b8edf6e67 Mon Sep 17 00:00:00 2001 From: Petr Muller Date: Wed, 7 May 2025 20:18:16 +0200 Subject: [PATCH 2/4] refactor: extract config informer factory prep OTA-1531 will need the config.openshift.io group lister early in the CVO execution and outside of the `Context` structure, so this commit moves the necessary preparation code outside of `NewControllerContext` and passes these factories into it as parameters. Constructing factories at separate places is a little awkward but for now I try keeping the change small instead of trying to invent some centralized informer factory management. I think it is reasonable to treat config.openshift.io informers separately if they are actually used separately. --- pkg/start/start.go | 23 +++++++++++++++-------- pkg/start/start_integration_test.go | 12 +++++++++--- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/pkg/start/start.go b/pkg/start/start.go index 6e37403be..6b2d575a9 100644 --- a/pkg/start/start.go +++ b/pkg/start/start.go @@ -214,8 +214,10 @@ func (o *Options) Run(ctx context.Context) error { klog.Infof("Determined OCP version for this CVO: %q", cvoOcpVersion) } + clusterVersionConfigInformerFactory, configInformerFactory := o.prepareConfigInformerFactories(cb) + // initialize the controllers and attempt to load the payload information - controllerCtx, err := o.NewControllerContext(cb) + controllerCtx, err := o.NewControllerContext(cb, clusterVersionConfigInformerFactory, configInformerFactory) if err != nil { return err } @@ -224,6 +226,16 @@ func (o *Options) Run(ctx context.Context) error { return nil } +func (o *Options) prepareConfigInformerFactories(cb *ClientBuilder) (configinformers.SharedInformerFactory, configinformers.SharedInformerFactory) { + client := cb.ClientOrDie("shared-informer") + clusterVersionConfigInformerFactory := configinformers.NewFilteredSharedInformerFactory(client, resyncPeriod(o.ResyncInterval), "", func(opts *metav1.ListOptions) { + opts.FieldSelector = fmt.Sprintf("metadata.name=%s", o.Name) + }) + configInformerFactory := configinformers.NewSharedInformerFactory(client, resyncPeriod(o.ResyncInterval)) + + return clusterVersionConfigInformerFactory, configInformerFactory +} + // run launches a number of goroutines to handle manifest application, // metrics serving, etc. It continues operating until ctx.Done(), // and then attempts a clean shutdown limited by an internal context @@ -510,22 +522,17 @@ type Context struct { // NewControllerContext initializes the default Context for the current Options. It does // not start any background processes. -func (o *Options) NewControllerContext(cb *ClientBuilder) (*Context, error) { - client := cb.ClientOrDie("shared-informer") +func (o *Options) NewControllerContext(cb *ClientBuilder, clusterVersionConfigInformerFactory, configInformerFactory configinformers.SharedInformerFactory) (*Context, error) { kubeClient := cb.KubeClientOrDie(internal.ConfigNamespace, useProtobuf) openshiftConfigInformerFactory := coreinformers.NewSharedInformerFactoryWithOptions(kubeClient, resyncPeriod(o.ResyncInterval), coreinformers.WithNamespace(internal.ConfigNamespace)) openshiftConfigManagedInformerFactory := coreinformers.NewSharedInformerFactoryWithOptions(kubeClient, resyncPeriod(o.ResyncInterval), coreinformers.WithNamespace(internal.ConfigManagedNamespace)) - clusterVersionConfigInformerFactory := configinformers.NewFilteredSharedInformerFactory(client, resyncPeriod(o.ResyncInterval), "", func(opts *metav1.ListOptions) { - opts.FieldSelector = fmt.Sprintf("metadata.name=%s", o.Name) - }) - configInformerFactory := configinformers.NewSharedInformerFactory(client, resyncPeriod(o.ResyncInterval)) - operatorClient := cb.OperatorClientOrDie("operator-client") filterByName := func(opts *metav1.ListOptions) { opts.FieldSelector = fields.OneTermEqualSelector("metadata.name", configuration.ClusterVersionOperatorConfigurationName).String() } operatorInformerFactory := operatorinformers.NewSharedInformerFactoryWithOptions(operatorClient, o.ResyncInterval, operatorinformers.WithTweakListOptions(filterByName)) + coInformer := configInformerFactory.Config().V1().ClusterOperators() cvoKubeClient := cb.KubeClientOrDie(o.Namespace, useProtobuf) diff --git a/pkg/start/start_integration_test.go b/pkg/start/start_integration_test.go index d3f063d24..325fca66d 100644 --- a/pkg/start/start_integration_test.go +++ b/pkg/start/start_integration_test.go @@ -188,7 +188,9 @@ func TestIntegrationCVO_initializeAndUpgrade(t *testing.T) { if err := options.ValidateAndComplete(); err != nil { t.Fatalf("incorrectly initialized options: %v", err) } - controllers, err := options.NewControllerContext(cb) + + cvif, cif := options.prepareConfigInformerFactories(cb) + controllers, err := options.NewControllerContext(cb, cvif, cif) if err != nil { t.Fatal(err) } @@ -323,7 +325,9 @@ func TestIntegrationCVO_gracefulStepDown(t *testing.T) { if err := options.ValidateAndComplete(); err != nil { t.Fatalf("incorrectly initialized options: %v", err) } - controllers, err := options.NewControllerContext(cb) + + cvif, cif := options.prepareConfigInformerFactories(cb) + controllers, err := options.NewControllerContext(cb, cvif, cif) if err != nil { t.Fatal(err) } @@ -520,7 +524,9 @@ metadata: if err := options.ValidateAndComplete(); err != nil { t.Fatalf("incorrectly initialized options: %v", err) } - controllers, err := options.NewControllerContext(cb) + + cvif, cif := options.prepareConfigInformerFactories(cb) + controllers, err := options.NewControllerContext(cb, cvif, cif) if err != nil { t.Fatal(err) } From 70d7ebf4e90e2cb218d51fe2fb36149a30eda285 Mon Sep 17 00:00:00 2001 From: Petr Muller Date: Wed, 7 May 2025 20:31:09 +0200 Subject: [PATCH 3/4] refactor: modernize CV informer factory creation `NewFilteredSharedInformerFactory` is deprecated and can be replaced by `NewSharedInformerFactoryWithOptions`, where the filter is expressed as `TweakListOption`. The direct `FieldSelector` equality can be replaced with the `OneTermEqualSelector` helper. --- pkg/start/start.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/start/start.go b/pkg/start/start.go index 6b2d575a9..9df5f8142 100644 --- a/pkg/start/start.go +++ b/pkg/start/start.go @@ -228,9 +228,10 @@ func (o *Options) Run(ctx context.Context) error { func (o *Options) prepareConfigInformerFactories(cb *ClientBuilder) (configinformers.SharedInformerFactory, configinformers.SharedInformerFactory) { client := cb.ClientOrDie("shared-informer") - clusterVersionConfigInformerFactory := configinformers.NewFilteredSharedInformerFactory(client, resyncPeriod(o.ResyncInterval), "", func(opts *metav1.ListOptions) { - opts.FieldSelector = fmt.Sprintf("metadata.name=%s", o.Name) - }) + filterByName := func(opts *metav1.ListOptions) { + opts.FieldSelector = fields.OneTermEqualSelector("metadata.name", o.Name).String() + } + clusterVersionConfigInformerFactory := configinformers.NewSharedInformerFactoryWithOptions(client, resyncPeriod(o.ResyncInterval), configinformers.WithTweakListOptions(filterByName)) configInformerFactory := configinformers.NewSharedInformerFactory(client, resyncPeriod(o.ResyncInterval)) return clusterVersionConfigInformerFactory, configInformerFactory From 61ca4451fdfb5f0598614d9bcf5ba0fc2ed5e356 Mon Sep 17 00:00:00 2001 From: Petr Muller Date: Tue, 13 May 2025 14:46:01 +0200 Subject: [PATCH 4/4] fix: do not log a `nil` error Address an earlier review comment: https://github.com/openshift/cluster-version-operator/pull/1188#discussion_r2084906818 --- pkg/start/start.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/start/start.go b/pkg/start/start.go index 9df5f8142..e719ccb6e 100644 --- a/pkg/start/start.go +++ b/pkg/start/start.go @@ -208,7 +208,7 @@ func (o *Options) Run(ctx context.Context) error { case err != nil: klog.Warningf("Failed to read release metadata to determine OCP version for this CVO (will use placeholder version %q): %v", cvoOcpVersion, err) case releaseMetadata.Version == "": - klog.Warningf("Version missing from release metadata, cannot determine OCP version for this CVO (will use placeholder version %q): %v", cvoOcpVersion, err) + klog.Warningf("Version missing from release metadata, cannot determine OCP version for this CVO (will use placeholder version %q)", cvoOcpVersion) default: cvoOcpVersion = releaseMetadata.Version klog.Infof("Determined OCP version for this CVO: %q", cvoOcpVersion)