refactor: move gates-related code into a featuregates package

pkg/cvo/cvo.go

@@ -32,7 +32,6 @@ import (
 	configinformersv1 "github.com/openshift/client-go/config/informers/externalversions/config/v1"
 	configlistersv1 "github.com/openshift/client-go/config/listers/config/v1"
 	"github.com/openshift/library-go/pkg/manifest"
-	"github.com/openshift/library-go/pkg/operator/status"
 	"github.com/openshift/library-go/pkg/verify"
 	"github.com/openshift/library-go/pkg/verify/store"
 	"github.com/openshift/library-go/pkg/verify/store/configmap"
@@ -46,6 +45,7 @@ import (
 	"github.com/openshift/cluster-version-operator/pkg/customsignaturestore"
 	cvointernal "github.com/openshift/cluster-version-operator/pkg/cvo/internal"
 	"github.com/openshift/cluster-version-operator/pkg/cvo/internal/dynamicclient"
+	"github.com/openshift/cluster-version-operator/pkg/featuregates"
 	"github.com/openshift/cluster-version-operator/pkg/internal"
 	"github.com/openshift/cluster-version-operator/pkg/payload"
 	"github.com/openshift/cluster-version-operator/pkg/payload/precondition"
@@ -57,59 +57,6 @@ const (
 	maxRetries = 15
 )
 
-// FeatureGates 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).
-type FeatureGates struct {
-	// UnknownVersion flag is set to true if CVO did not find a matching version in the FeatureGate
-	// status resource, meaning the current set of enabled and disabled feature gates is unknown for
-	// this version. This should be a temporary state (config-operator should eventually add the
-	// enabled/disabled flags for this version), so CVO should try to behave in a way that reflects
-	// a "good default": default-on flags are enabled, default-off flags are disabled. Where reasonable,
-	// It can also attempt to tolerate the existing state: if it finds evidence that a feature was
-	// enabled, it can continue to behave as if it was enabled and vice versa. This temporary state
-	// should be eventually resolved when the FeatureGate status resource is updated, which forces CVO
-	// to restart when the flags change.
-	UnknownVersion bool
-
-	// ResourceReconciliationIssuesCondition controls whether CVO maintains a Condition with
-	// ResourceReconciliationIssues type, containing a JSON that describes all "issues" that prevented
-	// or delayed CVO from reconciling individual resources in the cluster. This is a pseudo-API
-	// that the experimental work for "oc adm upgrade status" uses to report upgrade status, and
-	// should never be relied upon by any production code. We may want to eventually turn this into
-	// some kind of "real" API.
-	ResourceReconciliationIssuesCondition bool
-}
-
-func DefaultGatesWhenUnknown() FeatureGates {
-	return FeatureGates{
-		UnknownVersion: true,
-
-		ResourceReconciliationIssuesCondition: false,
-	}
-}
-
-func GetCvoGatesFrom(gate *configv1.FeatureGate) FeatureGates {
-	enabledGates := DefaultGatesWhenUnknown()
-	operatorVersion := status.VersionForOperatorFromEnv()
-	klog.Infof("Looking up feature gates for version %s", operatorVersion)
-	for _, g := range gate.Status.FeatureGates {
-
-		if g.Version != operatorVersion {
-			continue
-		}
-		// We found the matching version, so we do not need to run in the unknown version mode
-		enabledGates.UnknownVersion = false
-		for _, enabled := range g.Enabled {
-			if enabled.Name == configv1.FeatureGateUpgradeStatus {
-				enabledGates.ResourceReconciliationIssuesCondition = true
-			}
-		}
-	}
-
-	return enabledGates
-}
-
 // Operator defines cluster version operator. The CVO attempts to reconcile the appropriate image
 // onto the cluster, writing status to the ClusterVersion object as it goes. A background loop
 // periodically checks for new updates from a server described by spec.upstream and spec.channel.
@@ -218,14 +165,16 @@ type Operator struct {
 	// via annotation
 	exclude string
 
-	// requiredFeatureSet is set the value of featuregates.config.openshift.io|.spec.featureSet.  It's a very slow
-	// moving resource, so it is not re-detected live.
-	requiredFeatureSet string
+	clusterFeatures featuregates.ClusterFeatures
 
-	// enabledFeatureGates 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).
-	enabledFeatureGates FeatureGates
+	// // requiredFeatureSet is set the value of featuregates.config.openshift.io|.spec.featureSet.  It's a very slow
+	// // moving resource, so it is not re-detected live.
+	// requiredFeatureSet string
+	//
+	// // enabledFeatureGates 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).
+	// enabledFeatureGates CVOGates
 
 	clusterProfile string
 	uid            types.UID
@@ -246,8 +195,7 @@ func New(
 	client clientset.Interface,
 	kubeClient kubernetes.Interface,
 	exclude string,
-	requiredFeatureSet string,
-	enabledFeatureGates FeatureGates,
+	clusterFeatures featuregates.ClusterFeatures,
 	clusterProfile string,
 	promqlTarget clusterconditions.PromQLTarget,
 	injectClusterIdIntoPromQL bool,
@@ -279,8 +227,7 @@ func New(
 		upgradeableQueue:      workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "upgradeable"),
 
 		exclude:                   exclude,
-		requiredFeatureSet:        requiredFeatureSet,
-		enabledFeatureGates:       enabledFeatureGates,
+		clusterFeatures:           clusterFeatures,
 		clusterProfile:            clusterProfile,
 		conditionRegistry:         standard.NewConditionRegistry(promqlTarget),
 		injectClusterIdIntoPromQL: injectClusterIdIntoPromQL,
@@ -338,7 +285,7 @@ func (optr *Operator) InitializeFromPayload(ctx context.Context, restConfig *res
 		return fmt.Errorf("Error when attempting to get cluster version object: %w", err)
 	}
 
-	update, err := payload.LoadUpdate(optr.defaultPayloadDir(), optr.release.Image, optr.exclude, optr.requiredFeatureSet,
+	update, err := payload.LoadUpdate(optr.defaultPayloadDir(), optr.release.Image, optr.exclude, optr.clusterFeatures.StartingRequiredFeatureSet,
 		optr.clusterProfile, capability.GetKnownCapabilities())
 
 	if err != nil {
@@ -389,7 +336,7 @@ func (optr *Operator) InitializeFromPayload(ctx context.Context, restConfig *res
 			Cap:      time.Second * 15,
 		},
 		optr.exclude,
-		optr.requiredFeatureSet,
+		optr.clusterFeatures.StartingRequiredFeatureSet,
 		optr.eventRecorder,
 		optr.clusterProfile,
 	)

pkg/cvo/status.go

@@ -23,6 +23,7 @@ import (
 	configclientv1 "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
 
 	"github.com/openshift/cluster-version-operator/lib/resourcemerge"
+	"github.com/openshift/cluster-version-operator/pkg/featuregates"
 	"github.com/openshift/cluster-version-operator/pkg/payload"
 )
 
@@ -198,7 +199,7 @@ func (optr *Operator) syncStatus(ctx context.Context, original, config *configv1
 		original = config.DeepCopy()
 	}
 
-	updateClusterVersionStatus(&config.Status, status, optr.release, optr.getAvailableUpdates, optr.enabledFeatureGates, validationErrs)
+	updateClusterVersionStatus(&config.Status, status, optr.release, optr.getAvailableUpdates, optr.clusterFeatures.StartingCvoFeatureGates, validationErrs)
 
 	if klog.V(6).Enabled() {
 		klog.Infof("Apply config: %s", diff.ObjectReflectDiff(original, config))
@@ -210,7 +211,7 @@ func (optr *Operator) syncStatus(ctx context.Context, original, config *configv1
 
 // updateClusterVersionStatus updates the passed cvStatus with the latest status information
 func updateClusterVersionStatus(cvStatus *configv1.ClusterVersionStatus, status *SyncWorkerStatus,
-	release configv1.Release, getAvailableUpdates func() *availableUpdates, enabledGates FeatureGates,
+	release configv1.Release, getAvailableUpdates func() *availableUpdates, enabledGates featuregates.CVOGates,
 	validationErrs field.ErrorList) {
 
 	cvStatus.ObservedGeneration = status.Generation

pkg/cvo/status_test.go

@@ -8,14 +8,16 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
-	"github.com/openshift/cluster-version-operator/lib/resourcemerge"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/diff"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/client-go/tools/record"
 
 	configv1 "github.com/openshift/api/config/v1"
 	"github.com/openshift/client-go/config/clientset/versioned/fake"
+
+	"github.com/openshift/cluster-version-operator/lib/resourcemerge"
+	"github.com/openshift/cluster-version-operator/pkg/featuregates"
 )
 
 func Test_mergeEqualVersions(t *testing.T) {
@@ -245,7 +247,7 @@ func TestUpdateClusterVersionStatus_UnknownVersionAndRRI(t *testing.T) {
 	for _, tc := range testCases {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
-			gates := FeatureGates{
+			gates := featuregates.CVOGates{
 				UnknownVersion:                        tc.unknownVersion,
 				ResourceReconciliationIssuesCondition: false,
 			}
@@ -315,7 +317,7 @@ func TestUpdateClusterVersionStatus_ResourceReconciliationIssues(t *testing.T) {
 	for _, tc := range testCases {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
-			gates := FeatureGates{ResourceReconciliationIssuesCondition: tc.enabled}
+			gates := featuregates.CVOGates{ResourceReconciliationIssuesCondition: tc.enabled}
 			release := configv1.Release{}
 			getAvailableUpdates := func() *availableUpdates { return nil }
 			var noErrors field.ErrorList

pkg/featurechangestopper/featurechangestopper.go → pkg/featuregates/featurechangestopper.go

@@ -1,4 +1,4 @@
-package featurechangestopper
+package featuregates
 
 import (
 	"context"
@@ -15,14 +15,11 @@ import (
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/util/workqueue"
 	"k8s.io/klog/v2"
-
-	"github.com/openshift/cluster-version-operator/pkg/cvo"
 )
 
-// FeatureChangeStopper calls stop when the value of the featureset changes
-type FeatureChangeStopper struct {
-	startingRequiredFeatureSet string
-	startingCvoFeatureGates    cvo.FeatureGates
+// ChangeStopper calls stop when the value of the featureset changes
+type ChangeStopper struct {
+	clusterFeatures ClusterFeatures
 
 	featureGateLister configlistersv1.FeatureGateLister
 	cacheSynced       []cache.InformerSynced
@@ -31,18 +28,16 @@ type FeatureChangeStopper struct {
 	shutdownFn context.CancelFunc
 }
 
-// New returns a new FeatureChangeStopper.
+// New returns a new ChangeStopper.
 func New(
-	startingRequiredFeatureSet string,
-	startingCvoGates cvo.FeatureGates,
+	clusterFeatures ClusterFeatures,
 	featureGateInformer configinformersv1.FeatureGateInformer,
-) (*FeatureChangeStopper, error) {
-	c := &FeatureChangeStopper{
-		startingRequiredFeatureSet: startingRequiredFeatureSet,
-		startingCvoFeatureGates:    startingCvoGates,
-		featureGateLister:          featureGateInformer.Lister(),
-		cacheSynced:                []cache.InformerSynced{featureGateInformer.Informer().HasSynced},
-		queue:                      workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "feature-gate-stopper"),
+) (*ChangeStopper, error) {
+	c := &ChangeStopper{
+		clusterFeatures:   clusterFeatures,
+		featureGateLister: featureGateInformer.Lister(),
+		cacheSynced:       []cache.InformerSynced{featureGateInformer.Informer().HasSynced},
+		queue:             workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "feature-gate-stopper"),
 	}
 
 	c.queue.Add("cluster") // seed an initial sync, in case startingRequiredFeatureSet is wrong
@@ -66,18 +61,18 @@ func New(
 // 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).
-func (c *FeatureChangeStopper) syncHandler(_ context.Context) (done bool, err error) {
+func (c *ChangeStopper) syncHandler(_ context.Context) (done bool, err error) {
 	var current configv1.FeatureSet
-	var currentCvoGates cvo.FeatureGates
+	var currentCvoGates CVOGates
 	if featureGates, err := c.featureGateLister.Get("cluster"); err == nil {
 		current = featureGates.Spec.FeatureSet
-		currentCvoGates = cvo.GetCvoGatesFrom(featureGates)
+		currentCvoGates = getCvoGatesFrom(featureGates, c.clusterFeatures.VersionForGates)
 	} else if !apierrors.IsNotFound(err) {
 		return false, err
 	}
 
-	featureSetChanged := string(current) != c.startingRequiredFeatureSet
-	cvoFeaturesChanged := currentCvoGates != c.startingCvoFeatureGates
+	featureSetChanged := string(current) != c.clusterFeatures.StartingRequiredFeatureSet
+	cvoFeaturesChanged := currentCvoGates != c.clusterFeatures.StartingCvoFeatureGates
 	if featureSetChanged || cvoFeaturesChanged {
 		var action string
 		if c.shutdownFn == nil {
@@ -86,10 +81,10 @@ func (c *FeatureChangeStopper) syncHandler(_ context.Context) (done bool, err er
 			action = "requesting shutdown"
 		}
 		if featureSetChanged {
-			klog.Infof("FeatureSet was %q, but the current feature set is %q; %s.", c.startingRequiredFeatureSet, current, action)
+			klog.Infof("FeatureSet was %q, but the current feature set is %q; %s.", c.clusterFeatures.StartingRequiredFeatureSet, current, action)
 		}
 		if cvoFeaturesChanged {
-			klog.Infof("CVO feature flags were %v, but changed to %v; %s.", c.startingCvoFeatureGates, currentCvoGates, action)
+			klog.Infof("CVO feature flags were %v, but changed to %v; %s.", c.clusterFeatures.StartingCvoFeatureGates, currentCvoGates, action)
 		}
 
 		if c.shutdownFn != nil {
@@ -101,7 +96,7 @@ func (c *FeatureChangeStopper) syncHandler(_ context.Context) (done bool, err er
 }
 
 // Run launches the controller and blocks until it is canceled or work completes.
-func (c *FeatureChangeStopper) Run(ctx context.Context, shutdownFn context.CancelFunc) error {
+func (c *ChangeStopper) Run(ctx context.Context, shutdownFn context.CancelFunc) error {
 	// don't let panics crash the process
 	defer utilruntime.HandleCrash()
 	// make sure the work queue is shutdown which will trigger workers to end
@@ -114,7 +109,7 @@ func (c *FeatureChangeStopper) Run(ctx context.Context, shutdownFn context.Cance
 	}()
 	c.shutdownFn = shutdownFn
 
-	klog.Infof("Starting stop-on-featureset-change controller with %q.", c.startingRequiredFeatureSet)
+	klog.Infof("Starting stop-on-featureset-change controller with %q.", c.clusterFeatures.StartingRequiredFeatureSet)
 
 	// wait for your secondary caches to fill before starting your work
 	if !cache.WaitForCacheSync(ctx.Done(), c.cacheSynced...) {
@@ -129,7 +124,7 @@ func (c *FeatureChangeStopper) Run(ctx context.Context, shutdownFn context.Cance
 // runWorker handles a single worker poll round, processing as many
 // work items as possible, and returning done when there will be no
 // more work.
-func (c *FeatureChangeStopper) runWorker(ctx context.Context) (done bool, err error) {
+func (c *ChangeStopper) runWorker(ctx context.Context) (done bool, err error) {
 	// hot loop until we're told to stop.  processNextWorkItem will
 	// automatically wait until there's work available, so we don't worry
 	// about secondary waits
@@ -142,7 +137,7 @@ func (c *FeatureChangeStopper) runWorker(ctx context.Context) (done bool, err er
 
 // processNextWorkItem deals with one key off the queue.  It returns
 // done when there will be no more work.
-func (c *FeatureChangeStopper) processNextWorkItem(ctx context.Context) (done bool, err error) {
+func (c *ChangeStopper) processNextWorkItem(ctx context.Context) (done bool, err error) {
 	// pull the next work item from queue.  It should be a key we use to lookup
 	// something in a cache
 	key, quit := c.queue.Get()

pkg/featurechangestopper/featurechangestopper_test.go → pkg/featuregates/featurechangestopper_test.go

@@ -1,4 +1,4 @@
-package featurechangestopper
+package featuregates
 
 import (
 	"context"
@@ -9,16 +9,14 @@ import (
 	fakeconfigv1client "github.com/openshift/client-go/config/clientset/versioned/fake"
 	configv1informer "github.com/openshift/client-go/config/informers/externalversions"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-
-	"github.com/openshift/cluster-version-operator/pkg/cvo"
-	"github.com/openshift/cluster-version-operator/pkg/version"
 )
 
 func TestTechPreviewChangeStopper(t *testing.T) {
+	versionForGates := "1.2.3"
 	tests := []struct {
 		name                       string
 		startingRequiredFeatureSet string
-		startingCvoFeatureGates    cvo.FeatureGates
+		startingCvoFeatureGates    CVOGates
 
 		featureSet        string
 		featureGateStatus *configv1.FeatureGateStatus
@@ -58,14 +56,14 @@ func TestTechPreviewChangeStopper(t *testing.T) {
 		{
 			name:                       "cvo flags changed",
 			startingRequiredFeatureSet: "TechPreviewNoUpgrade",
-			startingCvoFeatureGates: cvo.FeatureGates{
+			startingCvoFeatureGates: CVOGates{
 				UnknownVersion: true,
 			},
 			featureSet: "TechPreviewNoUpgrade",
 			featureGateStatus: &configv1.FeatureGateStatus{
 				FeatureGates: []configv1.FeatureGateDetails{
 					{
-						Version: version.Version.String(),
+						Version: versionForGates,
 						Enabled: []configv1.FeatureGateAttributes{{Name: configv1.FeatureGateUpgradeStatus}},
 					},
 				},
@@ -95,14 +93,19 @@ func TestTechPreviewChangeStopper(t *testing.T) {
 				fg.Status = *tt.featureGateStatus
 			} else {
 				fg.Status = configv1.FeatureGateStatus{}
-				tt.startingCvoFeatureGates = cvo.FeatureGates{UnknownVersion: true}
+				tt.startingCvoFeatureGates = CVOGates{UnknownVersion: true}
 			}
 
 			client := fakeconfigv1client.NewSimpleClientset(fg)
 
 			informerFactory := configv1informer.NewSharedInformerFactory(client, 0)
 			featureGates := informerFactory.Config().V1().FeatureGates()
-			c, err := New(tt.startingRequiredFeatureSet, tt.startingCvoFeatureGates, featureGates)
+			cf := ClusterFeatures{
+				StartingRequiredFeatureSet: tt.startingRequiredFeatureSet,
+				StartingCvoFeatureGates:    tt.startingCvoFeatureGates,
+				VersionForGates:            versionForGates,
+			}
+			c, err := New(cf, featureGates)
 			if err != nil {
 				t.Fatal(err)
 			}