Skip to content

OTA-1531: [4/x] cvo: extract config informer creation #1189

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 14, 2025
Merged

OTA-1531: [4/x] cvo: extract config informer creation #1189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eeed4fa
refactor: unify names in informer-related code
petr-muller May 7, 2025
6d46b8f
refactor: extract config informer factory prep
petr-muller May 7, 2025
70d7ebf
refactor: modernize CV informer factory creation
petr-muller May 7, 2025
61ca445
fix: do not log a `nil` error
petr-muller May 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 58 additions & 40 deletions pkg/start/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,14 @@ 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"
"k8s.io/apimachinery/pkg/fields"
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"
Expand All @@ -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"

Expand Down Expand Up @@ -208,14 +208,16 @@ 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)
}

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
}
Expand All @@ -224,6 +226,17 @@ 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")
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
}

// 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
Expand Down Expand Up @@ -255,13 +268,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())
Expand Down Expand Up @@ -489,34 +502,39 @@ 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
Copy link
Member

@hongkailiu hongkailiu May 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love those comments. Thanks.

// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not understand why we use another factory for CV. But this is me, need more time to read the code.
My native guess is that their configurations are different for some reason.

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
}

// 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)
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) {
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()
}))
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 := sharedInformers.Config().V1().ClusterOperators()
coInformer := configInformerFactory.Config().V1().ClusterOperators()

cvoKubeClient := cb.KubeClientOrDie(o.Namespace, useProtobuf)
o.PromQLTarget.KubeClient = cvoKubeClient
Expand All @@ -527,11 +545,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,
Expand All @@ -548,28 +566,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),
)
Expand Down
12 changes: 9 additions & 3 deletions pkg/start/start_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down