OTA-1531: Rework error handling in FeatureGate processing

- The `FeatureGate` GET is now made over an informer client which is
  backed by a cache, so polling to smooth over non-isNotFound errors no
  longer makes sense. The GET runs after a cache sync succeeded earlier
  so no errors are expected (and if an error happens, it is unlikely to
  be resolved by a retry)
- Waiting for cache sync was not bounded; impose a 30s timeout on startup
  (this is similar to the deadline imposed by the poll before we started
  to use the informer client)
- Return pointers from `processInitialFeatureGate` to make return errors
  easier

Author: petr-muller

pkg/start/start.go

@@ -19,7 +19,6 @@ import (
 	"k8s.io/apimachinery/pkg/fields"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/apimachinery/pkg/util/sets"
-	"k8s.io/apimachinery/pkg/util/wait"
 	coreinformers "k8s.io/client-go/informers"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/scheme"
@@ -195,7 +194,7 @@ func (o *Options) Run(ctx context.Context) error {
 	}
 
 	// initialize the controllers and attempt to load the payload information
-	controllerCtx, err := o.NewControllerContext(cb, startingFeatureSet, startingCvoGates, clusterVersionConfigInformerFactory, configInformerFactory)
+	controllerCtx, err := o.NewControllerContext(cb, *startingFeatureSet, *startingCvoGates, clusterVersionConfigInformerFactory, configInformerFactory)
 	if err != nil {
 		return err
 	}
@@ -243,57 +242,49 @@ 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())
+
+	ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
+	defer cancel()
+
+	for key, synced := range configInformerFactory.WaitForCacheSync(ctx.Done()) {
+		if !synced {
+			return nil, nil, fmt.Errorf("failed to sync %s informer cache: %v", key.String(), ctx.Err())
+		}
+	}
 
 	cvoOpenShiftVersion := o.getOpenShiftVersion()
 	cvoGates := featuregates.DefaultCvoGates(cvoOpenShiftVersion)
 
 	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 := featureGates.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, will assume 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
-			cvoGates = featuregates.CvoGatesFromFeatureGate(clusterFeatureGate, cvoOpenShiftVersion)
-			klog.Infof("FeatureGate found in cluster, using its feature set %q at startup", startingFeatureSet)
-			return true, nil
-		}
-	}); err != nil {
-		if lastError != nil {
-			return "", cvoGates, lastError
-		}
-		return "", cvoGates, err
+	gate, err := featureGates.Get("cluster")
+	switch {
+	case err != nil && apierrors.IsNotFound(err):
+		// 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, will assume default feature set %q at startup", startingFeatureSet)
+	case err != nil:
+		// This should not happen because featureGates is backed by the informer cache which successfully synced earlier
+		klog.Errorf("Failed to get FeatureGate from cluster: %v", err)
+		return nil, nil, fmt.Errorf("failed to get FeatureGate from informer cache: %w", err)
+	default:
+		clusterFeatureGate = gate
+		startingFeatureSet = gate.Spec.FeatureSet
+		cvoGates = featuregates.CvoGatesFromFeatureGate(clusterFeatureGate, cvoOpenShiftVersion)
+		klog.Infof("FeatureGate found in cluster, using its feature set %q at startup", startingFeatureSet)
 	}
 
 	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,

pkg/start/start_integration_test.go

@@ -194,7 +194,7 @@ func TestIntegrationCVO_initializeAndUpgrade(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	controllers, err := options.NewControllerContext(cb, featureset, gates, clusterVersionConfigInformerFactory, configInformerFactory)
+	controllers, err := options.NewControllerContext(cb, *featureset, *gates, clusterVersionConfigInformerFactory, configInformerFactory)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -335,7 +335,7 @@ func TestIntegrationCVO_gracefulStepDown(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	controllers, err := options.NewControllerContext(cb, featureset, gates, clusterVersionConfigInformerFactory, configInformerFactory)
+	controllers, err := options.NewControllerContext(cb, *featureset, *gates, clusterVersionConfigInformerFactory, configInformerFactory)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -538,7 +538,7 @@ metadata:
 	if err != nil {
 		t.Fatal(err)
 	}
-	controllers, err := options.NewControllerContext(cb, featureset, gates, clusterVersionConfigInformerFactory, configInformerFactory)
+	controllers, err := options.NewControllerContext(cb, *featureset, *gates, clusterVersionConfigInformerFactory, configInformerFactory)
 	if err != nil {
 		t.Fatal(err)
 	}