cvo: use early gate checker to avoid delayed initialization

Previous changes prepared the code for this switch by adding code to detect
enabled CVO gates and featureset early in the CVO execution, right before
the controllers are initialized (well before CVO acquires its leader lease
and actually starts the controllers). These early detected gate checker
was not used anywhere yet.

This change makes CVO stop using its late detected gate checker (detected
while loading its initial payload) and start using the early one instead.
This eliminates the need for the panicking gate checker that was used to
guard against checking gates before the actual checker could be established.

The change also allows to simplify some code layers. Late initialization
on initial payload load is now only necessary for the actual CVO controller,
not the featurechangestopper (it is already initialized at that time), so
the `Context` layer does not need to coordinate anymore. The starting
features set can be initialized in CVO right away, avoiding the need
to wire it through call chains; it can simply be an `Operator` member.

Author: petr-muller

pkg/cvo/cvo.go

@@ -174,6 +174,19 @@ type Operator struct {
 	// via annotation
 	exclude string
 
+	// requiredFeatureSet is the feature set that was detected in the cluster when CVO was started and is used
+	// to select the manifests that will be applied in the cluster. The starting value cannot be changed in the executing
+	// CVO but the featurechangestopper controller will detect a feature set change in the cluster and shutdown the CVO.
+	// Enforcing featuresets is a standard GA CVO behavior that supports the feature gating functionality across the whole
+	// cluster, as opposed to the enabledFeatureGates which controls what gated behaviors of CVO itself are enabled by
+	// the cluster feature gates.
+	// See: https://github.com/openshift/enhancements/blob/master/enhancements/update/cvo-techpreview-manifests.md
+	requiredFeatureSet configv1.FeatureSet
+
+	// enabledFeatureGates is the checker for what gated CVO behaviors are enabled or disabled by specific cluster-level
+	// feature gates. It allows multiplexing the cluster-level feature gates to more granular CVO-level gates. Similarly
+	// to the requiredFeatureSet, the enabledFeatureGates cannot be changed in the executing CVO but the
+	// featurechangestopper controller will detect when cluster feature gate config changes and shutdown the CVO.
 	enabledFeatureGates featuregates.CvoGateChecker
 
 	clusterProfile string
@@ -210,6 +223,8 @@ func New(
 	injectClusterIdIntoPromQL bool,
 	updateService string,
 	alwaysEnableCapabilities []configv1.ClusterVersionCapability,
+	featureSet configv1.FeatureSet,
+	cvoGates featuregates.CvoGateChecker,
 ) (*Operator, error) {
 	eventBroadcaster := record.NewBroadcaster()
 	eventBroadcaster.StartLogging(klog.Infof)
@@ -243,10 +258,9 @@ func New(
 		conditionRegistry:         standard.NewConditionRegistry(promqlTarget),
 		injectClusterIdIntoPromQL: injectClusterIdIntoPromQL,
 
-		// Because of OCPBUGS-30080, we can only detect the enabled feature gates after Operator loads the initial payload
-		// from disk via LoadInitialPayload. We must not have any gate-checking code until that happens, so we initialize
-		// this field with a checker that panics when used.
-		enabledFeatureGates:      featuregates.PanicOnUsageBeforeInitialization,
+		requiredFeatureSet:  featureSet,
+		enabledFeatureGates: cvoGates,
+
 		alwaysEnableCapabilities: alwaysEnableCapabilities,
 	}
 
@@ -283,7 +297,7 @@ func New(
 
 // LoadInitialPayload waits until a ClusterVersion object exists. It then retrieves the payload contents, verifies the
 // initial state and returns it. If the payload is invalid, an error is returned.
-func (optr *Operator) LoadInitialPayload(ctx context.Context, startingRequiredFeatureSet configv1.FeatureSet, restConfig *rest.Config) (*payload.Update, error) {
+func (optr *Operator) LoadInitialPayload(ctx context.Context, restConfig *rest.Config) (*payload.Update, error) {
 
 	// wait until cluster version object exists
 	if err := wait.PollUntilContextCancel(ctx, 3*time.Second, true, func(ctx context.Context) (bool, error) {
@@ -303,7 +317,7 @@ func (optr *Operator) LoadInitialPayload(ctx context.Context, startingRequiredFe
 		return nil, fmt.Errorf("Error when attempting to get cluster version object: %w", err)
 	}
 
-	update, err := payload.LoadUpdate(optr.defaultPayloadDir(), optr.release.Image, optr.exclude, string(startingRequiredFeatureSet),
+	update, err := payload.LoadUpdate(optr.defaultPayloadDir(), optr.release.Image, optr.exclude, string(optr.requiredFeatureSet),
 		optr.clusterProfile, configv1.KnownClusterVersionCapabilities)
 
 	if err != nil {
@@ -339,8 +353,12 @@ func (optr *Operator) LoadInitialPayload(ctx context.Context, startingRequiredFe
 
 // InitializeFromPayload configures the controller that loads and applies content to the cluster given an initial payload
 // and feature gate data.
-func (optr *Operator) InitializeFromPayload(update *payload.Update, requiredFeatureSet configv1.FeatureSet, cvoFlags featuregates.CvoGateChecker, restConfig *rest.Config, burstRestConfig *rest.Config) {
-	optr.enabledFeatureGates = cvoFlags
+func (optr *Operator) InitializeFromPayload(ctx context.Context, restConfig *rest.Config, burstRestConfig *rest.Config) error {
+	update, err := optr.LoadInitialPayload(ctx, restConfig)
+	if err != nil {
+		return err
+	}
+
 	optr.release = update.Release
 	optr.releaseCreated = update.ImageRef.CreationTimestamp.Time
 
@@ -358,11 +376,13 @@ func (optr *Operator) InitializeFromPayload(update *payload.Update, requiredFeat
 			Cap:      time.Second * 15,
 		},
 		optr.exclude,
-		requiredFeatureSet,
+		optr.requiredFeatureSet,
 		optr.eventRecorder,
 		optr.clusterProfile,
 		optr.alwaysEnableCapabilities,
 	)
+
+	return nil
 }
 
 // ownerReferenceModifier sets the owner reference to the current CV resource if no other reference exists. It also resets

pkg/featuregates/featurechangestopper.go

@@ -30,12 +30,16 @@ type ChangeStopper struct {
 }
 
 // NewChangeStopper returns a new ChangeStopper.
-func NewChangeStopper(featureGateInformer configinformersv1.FeatureGateInformer) (*ChangeStopper, error) {
+func NewChangeStopper(featureGateInformer configinformersv1.FeatureGateInformer, requiredFeatureSet configv1.FeatureSet, cvoGates CvoGates) (*ChangeStopper, error) {
 	c := &ChangeStopper{
 		featureGateLister: featureGateInformer.Lister(),
 		cacheSynced:       []cache.InformerSynced{featureGateInformer.Informer().HasSynced},
 		queue:             workqueue.NewTypedRateLimitingQueueWithConfig[any](workqueue.DefaultTypedControllerRateLimiter[any](), workqueue.TypedRateLimitingQueueConfig[any]{Name: "feature-gate-stopper"}),
+
+		startingRequiredFeatureSet: &requiredFeatureSet,
+		startingCvoGates:           &cvoGates,
 	}
+
 	c.queue.Add("cluster") // seed an initial sync, in case startingRequiredFeatureSet is wrong
 	if _, err := featureGateInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
 		AddFunc: func(_ interface{}) {
@@ -54,11 +58,6 @@ func NewChangeStopper(featureGateInformer configinformersv1.FeatureGateInformer)
 	return c, nil
 }
 
-func (c *ChangeStopper) SetStartingFeatures(requiredFeatureSet configv1.FeatureSet, cvoGates CvoGates) {
-	c.startingRequiredFeatureSet = &requiredFeatureSet
-	c.startingCvoGates = &cvoGates
-}
-
 // syncHandler processes a single work entry, with the
 // processNextWorkItem caller handling the queue management.  It returns
 // done when there will be no more work (because the feature gate changed).

pkg/featuregates/featurechangestopper_test.go

@@ -101,11 +101,10 @@ func TestTechPreviewChangeStopper(t *testing.T) {
 			client := fakeconfigv1client.NewSimpleClientset(fg)
 
 			informerFactory := configv1informer.NewSharedInformerFactory(client, 0)
-			c, err := NewChangeStopper(informerFactory.Config().V1().FeatureGates())
+			c, err := NewChangeStopper(informerFactory.Config().V1().FeatureGates(), tt.startingRequiredFeatureSet, tt.startingCvoFeatureGates)
 			if err != nil {
 				t.Fatal(err)
 			}
-			c.SetStartingFeatures(tt.startingRequiredFeatureSet, tt.startingCvoFeatureGates)
 			informerFactory.Start(ctx.Done())
 
 			if err := c.Run(ctx, shutdownFn); err != nil {

pkg/featuregates/featuregates.go

@@ -45,36 +45,6 @@ type CvoGateChecker interface {
 	CVOConfiguration() bool
 }
 
-type panicOnUsageBeforeInitializationFunc func()
-
-func panicOnUsageBeforeInitialization() {
-	panic("CVO feature flags were used before they were initialized")
-}
-
-// PanicOnUsageBeforeInitialization is a CvoGateChecker that panics if any of its methods are called. This checker should
-// be used before CVO feature gates are actually known and some code tries to check them.
-var PanicOnUsageBeforeInitialization = panicOnUsageBeforeInitializationFunc(panicOnUsageBeforeInitialization)
-
-func (p panicOnUsageBeforeInitializationFunc) ReconciliationIssuesCondition() bool {
-	p()
-	return false
-}
-
-func (p panicOnUsageBeforeInitializationFunc) StatusReleaseArchitecture() bool {
-	p()
-	return false
-}
-
-func (p panicOnUsageBeforeInitializationFunc) UnknownVersion() bool {
-	p()
-	return false
-}
-
-func (p panicOnUsageBeforeInitializationFunc) CVOConfiguration() bool {
-	p()
-	return false
-}
-
 // CvoGates contains flags that control CVO functionality gated by product feature gates. The
 // names do not correspond to product feature gates, the booleans here are "smaller" (product-level
 // gate will enable multiple CVO behaviors).

pkg/start/start.go

@@ -35,7 +35,6 @@ import (
 	configv1 "github.com/openshift/api/config/v1"
 	clientset "github.com/openshift/client-go/config/clientset/versioned"
 	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"
 	operatorinformers "github.com/openshift/client-go/operator/informers/externalversions"
 	"github.com/openshift/library-go/pkg/config/clusterstatus"
@@ -190,13 +189,13 @@ func (o *Options) Run(ctx context.Context) error {
 	}
 
 	clusterVersionConfigInformerFactory, configInformerFactory := o.prepareConfigInformerFactories(cb)
-	_, _, err = o.processInitialFeatureGate(ctx, configInformerFactory)
+	startingFeatureSet, startingCvoGates, err := o.processInitialFeatureGate(ctx, configInformerFactory)
 	if err != nil {
 		return fmt.Errorf("error processing feature gates: %w", err)
 	}
 
 	// initialize the controllers and attempt to load the payload information
-	controllerCtx, err := o.NewControllerContext(cb, clusterVersionConfigInformerFactory, configInformerFactory)
+	controllerCtx, err := o.NewControllerContext(cb, startingFeatureSet, startingCvoGates, clusterVersionConfigInformerFactory, configInformerFactory)
 	if err != nil {
 		return err
 	}
@@ -244,7 +243,7 @@ func (o *Options) getOpenShiftVersion() string {
 	return releaseMetadata.Version
 }
 
-func (o *Options) processInitialFeatureGate(ctx context.Context, configInformerFactory configinformers.SharedInformerFactory) (configv1.FeatureSet, *featuregates.CvoGates, error) {
+func (o *Options) processInitialFeatureGate(ctx context.Context, configInformerFactory configinformers.SharedInformerFactory) (configv1.FeatureSet, featuregates.CvoGates, error) {
 	featureGates := configInformerFactory.Config().V1().FeatureGates().Lister()
 	configInformerFactory.Start(ctx.Done())
 	configInformerFactory.WaitForCacheSync(ctx.Done())
@@ -284,17 +283,17 @@ func (o *Options) processInitialFeatureGate(ctx context.Context, configInformerF
 		}
 	}); err != nil {
 		if lastError != nil {
-			return "", nil, lastError
+			return "", cvoGates, lastError
 		}
-		return "", nil, err
+		return "", cvoGates, err
 	}
 
 	if cvoGates.UnknownVersion() {
 		klog.Warningf("CVO features for version %s could not be detected from FeatureGate; will use defaults plus special UnknownVersion feature gate", cvoOpenShiftVersion)
 	}
 	klog.Infof("CVO features for version %s enabled at startup: %+v", cvoOpenShiftVersion, cvoGates)
 
-	return startingFeatureSet, &cvoGates, nil
+	return startingFeatureSet, cvoGates, nil
 }
 
 // run launches a number of goroutines to handle manifest application,
@@ -362,7 +361,7 @@ func (o *Options) run(ctx context.Context, controllerCtx *Context, lock resource
 							resultChannel <- asyncResult{name: "metrics server", error: err}
 						}()
 					}
-					if err := controllerCtx.InitializeFromPayload(runContext, restConfig, burstRestConfig); err != nil {
+					if err := controllerCtx.CVO.InitializeFromPayload(runContext, restConfig, burstRestConfig); err != nil {
 						if firstError == nil {
 							firstError = err
 						}
@@ -577,13 +576,17 @@ type Context struct {
 	// 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, clusterVersionConfigInformerFactory, configInformerFactory configinformers.SharedInformerFactory) (*Context, error) {
+func (o *Options) NewControllerContext(
+	cb *ClientBuilder,
+	startingFeatureSet configv1.FeatureSet,
+	startingCvoGates featuregates.CvoGates,
+	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))
@@ -621,12 +624,14 @@ func (o *Options) NewControllerContext(cb *ClientBuilder, clusterVersionConfigIn
 		o.InjectClusterIdIntoPromQL,
 		o.UpdateService,
 		stringsToCapabilities(o.AlwaysEnableCapabilities),
+		startingFeatureSet,
+		startingCvoGates,
 	)
 	if err != nil {
 		return nil, err
 	}
 
-	featureChangeStopper, err := featuregates.NewChangeStopper(configInformerFactory.Config().V1().FeatureGates())
+	featureChangeStopper, err := featuregates.NewChangeStopper(configInformerFactory.Config().V1().FeatureGates(), startingFeatureSet, startingCvoGates)
 	if err != nil {
 		return nil, err
 	}
@@ -639,8 +644,6 @@ func (o *Options) NewControllerContext(cb *ClientBuilder, clusterVersionConfigIn
 		OperatorInformerFactory:               operatorInformerFactory,
 		CVO:                                   cvo,
 		StopOnFeatureGateChange:               featureChangeStopper,
-
-		fgLister: configInformerFactory.Config().V1().FeatureGates().Lister(),
 	}
 
 	if o.EnableAutoUpdate {
@@ -663,70 +666,6 @@ func (o *Options) NewControllerContext(cb *ClientBuilder, clusterVersionConfigIn
 	return ctx, nil
 }
 
-// InitializeFromPayload initializes the CVO and FeatureGate ChangeStoppers controllers from the payload. It extracts the
-// current CVO version from the initial payload and uses it to determine the initial the required featureset and enabled
-// feature gates. Both the payload and determined feature information are used to initialize CVO and feature gate
-// ChangeStopper controllers.
-func (c *Context) InitializeFromPayload(ctx context.Context, restConfig *rest.Config, burstRestConfig *rest.Config) error {
-	var startingFeatureSet configv1.FeatureSet
-	var clusterFeatureGate *configv1.FeatureGate
-
-	// client-go automatically retries some network blip errors on GETs for 30s by default, and we want to
-	// retry the remaining ones ourselves. If we fail longer than that, CVO won't be able to do work anyway.
-	// Return the error and crashloop.
-	//
-	// We implement the timeout with a context because the timeout in PollImmediateWithContext does not behave
-	// well when ConditionFunc takes longer time to execute, like here where the GET can be retried by client-go
-	var lastError error
-	if err := wait.PollUntilContextTimeout(context.Background(), 2*time.Second, 25*time.Second, true, func(ctx context.Context) (bool, error) {
-		gate, fgErr := c.fgLister.Get("cluster")
-		switch {
-		case apierrors.IsNotFound(fgErr):
-			// if we have no featuregates, then the cluster is using the default featureset, which is "".
-			// This excludes everything that could possibly depend on a different feature set.
-			startingFeatureSet = ""
-			klog.Infof("FeatureGate not found in cluster, using default feature set %q at startup", startingFeatureSet)
-			return true, nil
-		case fgErr != nil:
-			lastError = fgErr
-			klog.Warningf("Failed to get FeatureGate from cluster: %v", fgErr)
-			return false, nil
-		default:
-			clusterFeatureGate = gate
-			startingFeatureSet = gate.Spec.FeatureSet
-			klog.Infof("FeatureGate found in cluster, using its feature set %q at startup", startingFeatureSet)
-			return true, nil
-		}
-	}); err != nil {
-		if lastError != nil {
-			return lastError
-		}
-		return err
-	}
-
-	payload, err := c.CVO.LoadInitialPayload(ctx, startingFeatureSet, restConfig)
-	if err != nil {
-		return err
-	}
-
-	var cvoGates featuregates.CvoGates
-	if clusterFeatureGate != nil {
-		cvoGates = featuregates.CvoGatesFromFeatureGate(clusterFeatureGate, payload.Release.Version)
-	} else {
-		cvoGates = featuregates.DefaultCvoGates(payload.Release.Version)
-	}
-
-	if cvoGates.UnknownVersion() {
-		klog.Infof("CVO features for version %s could not be detected from FeatureGate; will use defaults plus special UnknownVersion feature gate", payload.Release.Version)
-	}
-	klog.Infof("CVO features for version %s enabled at startup: %+v", payload.Release.Version, cvoGates)
-
-	c.StopOnFeatureGateChange.SetStartingFeatures(startingFeatureSet, cvoGates)
-	c.CVO.InitializeFromPayload(payload, startingFeatureSet, cvoGates, restConfig, burstRestConfig)
-
-	return nil
-}
-
 func stringsToCapabilities(names []string) []configv1.ClusterVersionCapability {
 	caps := make([]configv1.ClusterVersionCapability, len(names))
 	for i, c := range names {

pkg/start/start_integration_test.go

@@ -190,7 +190,11 @@ func TestIntegrationCVO_initializeAndUpgrade(t *testing.T) {
 	}
 
 	cvif, cif := options.prepareConfigInformerFactories(cb)
-	controllers, err := options.NewControllerContext(cb, cvif, cif)
+	featureset, gates, err := options.processInitialFeatureGate(context.Background(), cvif)
+	if err != nil {
+		t.Fatal(err)
+	}
+	controllers, err := options.NewControllerContext(cb, featureset, gates, cvif, cif)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -327,7 +331,11 @@ func TestIntegrationCVO_gracefulStepDown(t *testing.T) {
 	}
 
 	cvif, cif := options.prepareConfigInformerFactories(cb)
-	controllers, err := options.NewControllerContext(cb, cvif, cif)
+	featureset, gates, err := options.processInitialFeatureGate(context.Background(), cvif)
+	if err != nil {
+		t.Fatal(err)
+	}
+	controllers, err := options.NewControllerContext(cb, featureset, gates, cvif, cif)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -526,7 +534,11 @@ metadata:
 	}
 
 	cvif, cif := options.prepareConfigInformerFactories(cb)
-	controllers, err := options.NewControllerContext(cb, cvif, cif)
+	featureset, gates, err := options.processInitialFeatureGate(context.Background(), cvif)
+	if err != nil {
+		t.Fatal(err)
+	}
+	controllers, err := options.NewControllerContext(cb, featureset, gates, cvif, cif)
 	if err != nil {
 		t.Fatal(err)
 	}