Merge pull request #1032 from petr-muller/ota-1159-result-of-work-rri-condition

OTA-1159: [2/3] Maintain `ReconciliationIssues` condition

Author: openshift-merge-bot[bot]

install/0000_00_cluster-version-operator_03_deployment.yaml

@@ -56,6 +56,9 @@ spec:
             name: kube-api-access
             readOnly: true
         env:
+          # Unfortunately the placeholder is not replaced, reported as OCPBUGS-30080
+          - name: OPERATOR_IMAGE_VERSION
+            value: "0.0.1-snapshot"
           - name: KUBERNETES_SERVICE_PORT # allows CVO to communicate with apiserver directly on same host.  Is substituted with port from infrastructures.status.apiServerInternalURL if available.
             value: "6443"
           - name: KUBERNETES_SERVICE_HOST # allows CVO to communicate with apiserver directly on same host.  Is substituted with hostname from infrastructures.status.apiServerInternalURL if available.

pkg/cvo/cvo.go

@@ -45,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"
@@ -164,9 +165,7 @@ 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
+	enabledFeatureGates featuregates.CvoGateChecker
 
 	clusterProfile string
 	uid            types.UID
@@ -187,7 +186,6 @@ func New(
 	client clientset.Interface,
 	kubeClient kubernetes.Interface,
 	exclude string,
-	requiredFeatureSet string,
 	clusterProfile string,
 	promqlTarget clusterconditions.PromQLTarget,
 	injectClusterIdIntoPromQL bool,
@@ -219,10 +217,14 @@ func New(
 		upgradeableQueue:      workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "upgradeable"),
 
 		exclude:                   exclude,
-		requiredFeatureSet:        requiredFeatureSet,
 		clusterProfile:            clusterProfile,
 		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,
 	}
 
 	if _, err := cvInformer.Informer().AddEventHandler(optr.clusterVersionEventHandler()); err != nil {
@@ -254,10 +256,9 @@ func New(
 	return optr, nil
 }
 
-// InitializeFromPayload waits until a ClusterVersion object exists. It then retrieves the payload contents and verifies the
-// initial state, then configures the controller that loads and applies content to the cluster. It returns an error if the
-// payload appears to be in error rather than continuing.
-func (optr *Operator) InitializeFromPayload(ctx context.Context, restConfig *rest.Config, burstRestConfig *rest.Config) error {
+// 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) {
 
 	// wait until cluster version object exists
 	if err := wait.PollUntilContextCancel(ctx, 3*time.Second, true, func(ctx context.Context) (bool, error) {
@@ -274,24 +275,19 @@ func (optr *Operator) InitializeFromPayload(ctx context.Context, restConfig *res
 		}
 		return true, nil
 	}); err != nil {
-		return fmt.Errorf("Error when attempting to get cluster version object: %w", err)
+		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, optr.requiredFeatureSet,
+	update, err := payload.LoadUpdate(optr.defaultPayloadDir(), optr.release.Image, optr.exclude, string(startingRequiredFeatureSet),
 		optr.clusterProfile, capability.GetKnownCapabilities())
 
 	if err != nil {
-		return fmt.Errorf("the local release contents are invalid - no current version can be determined from disk: %v", err)
+		return nil, fmt.Errorf("the local release contents are invalid - no current version can be determined from disk: %v", err)
 	}
-
-	optr.release = update.Release
-	optr.releaseCreated = update.ImageRef.CreationTimestamp.Time
-	optr.SetArchitecture(update.Architecture)
-
 	httpClientConstructor := sigstore.NewCachedHTTPClientConstructor(optr.HTTPClient, nil)
 	configClient, err := coreclientsetv1.NewForConfig(restConfig)
 	if err != nil {
-		return fmt.Errorf("unable to create a configuration client: %v", err)
+		return nil, fmt.Errorf("unable to create a configuration client: %v", err)
 	}
 
 	customSignatureStore := &customsignaturestore.Store{
@@ -303,7 +299,7 @@ func (optr *Operator) InitializeFromPayload(ctx context.Context, restConfig *res
 	// attempt to load a verifier as defined in the payload
 	verifier, signatureStore, err := loadConfigMapVerifierDataFromUpdate(update, httpClientConstructor.HTTPClient, configClient, customSignatureStore)
 	if err != nil {
-		return err
+		return nil, err
 	}
 	if verifier != nil {
 		klog.Infof("Verifying release authenticity: %v", verifier)
@@ -313,6 +309,16 @@ func (optr *Operator) InitializeFromPayload(ctx context.Context, restConfig *res
 	}
 	optr.verifier = verifier
 	optr.signatureStore = signatureStore
+	return update, nil
+}
+
+// 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
+	optr.release = update.Release
+	optr.releaseCreated = update.ImageRef.CreationTimestamp.Time
+	optr.SetArchitecture(update.Architecture)
 
 	// after the verifier has been loaded, initialize the sync worker with a payload retriever
 	// which will consume the verifier
@@ -328,12 +334,10 @@ func (optr *Operator) InitializeFromPayload(ctx context.Context, restConfig *res
 			Cap:      time.Second * 15,
 		},
 		optr.exclude,
-		optr.requiredFeatureSet,
+		requiredFeatureSet,
 		optr.eventRecorder,
 		optr.clusterProfile,
 	)
-
-	return nil
 }
 
 // ownerReferenceModifier sets the owner reference to the current CV resource if no other reference exists. It also resets

pkg/cvo/cvo_scenarios_test.go

@@ -26,10 +26,11 @@ import (
 
 	configv1 "github.com/openshift/api/config/v1"
 	"github.com/openshift/client-go/config/clientset/versioned/fake"
+	"github.com/openshift/library-go/pkg/manifest"
 
+	"github.com/openshift/cluster-version-operator/pkg/featuregates"
 	"github.com/openshift/cluster-version-operator/pkg/payload"
 	"github.com/openshift/cluster-version-operator/pkg/payload/precondition"
-	"github.com/openshift/library-go/pkg/manifest"
 )
 
 var architecture string
@@ -108,14 +109,15 @@ func setupCVOTest(payloadDir string) (*Operator, map[string]apiruntime.Object, *
 	}
 
 	o := &Operator{
-		namespace:      "test",
-		name:           "version",
-		queue:          workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "cvo-loop-test"),
-		client:         client,
-		cvLister:       &clientCVLister{client: client},
-		exclude:        "exclude-test",
-		eventRecorder:  record.NewFakeRecorder(100),
-		clusterProfile: payload.DefaultClusterProfile,
+		namespace:           "test",
+		name:                "version",
+		queue:               workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "cvo-loop-test"),
+		client:              client,
+		enabledFeatureGates: featuregates.DefaultCvoGates("version"),
+		cvLister:            &clientCVLister{client: client},
+		exclude:             "exclude-test",
+		eventRecorder:       record.NewFakeRecorder(100),
+		clusterProfile:      payload.DefaultClusterProfile,
 	}
 
 	dynamicScheme := apiruntime.NewScheme()

pkg/cvo/cvo_test.go

@@ -39,11 +39,12 @@ import (
 	configv1 "github.com/openshift/api/config/v1"
 	clientset "github.com/openshift/client-go/config/clientset/versioned"
 	"github.com/openshift/client-go/config/clientset/versioned/fake"
-
-	"github.com/openshift/cluster-version-operator/pkg/payload"
 	"github.com/openshift/library-go/pkg/manifest"
 	"github.com/openshift/library-go/pkg/verify/store/serial"
 	"github.com/openshift/library-go/pkg/verify/store/sigstore"
+
+	"github.com/openshift/cluster-version-operator/pkg/featuregates"
+	"github.com/openshift/cluster-version-operator/pkg/payload"
 )
 
 var (
@@ -2273,6 +2274,7 @@ func TestOperator_sync(t *testing.T) {
 				optr.configSync = &fakeSyncRecorder{Returns: expectStatus}
 			}
 			optr.eventRecorder = record.NewFakeRecorder(100)
+			optr.enabledFeatureGates = featuregates.DefaultCvoGates("version")
 
 			ctx := context.Background()
 			err := optr.sync(ctx, optr.queueKey())

pkg/cvo/reconciliation_issues.go

@@ -0,0 +1,13 @@
+package cvo
+
+import v1 "github.com/openshift/api/config/v1"
+
+const (
+	reconciliationIssuesConditionType v1.ClusterStatusConditionType = "ReconciliationIssues"
+
+	noReconciliationIssuesReason  string = "NoIssues"
+	noReconciliationIssuesMessage string = "No issues found during reconciliation"
+
+	reconciliationIssuesFoundReason  string = "IssuesFound"
+	reconciliationIssuesFoundMessage string = "Issues found during reconciliation"
+)

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, validationErrs)
+	updateClusterVersionStatus(&config.Status, status, optr.release, optr.getAvailableUpdates, optr.enabledFeatureGates, validationErrs)
 
 	if klog.V(6).Enabled() {
 		klog.Infof("Apply config: %s", diff.ObjectReflectDiff(original, config))
@@ -210,7 +211,8 @@ 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, validationErrs field.ErrorList) {
+	release configv1.Release, getAvailableUpdates func() *availableUpdates, enabledGates featuregates.CvoGateChecker,
+	validationErrs field.ErrorList) {
 
 	cvStatus.ObservedGeneration = status.Generation
 	if len(status.VersionHash) > 0 {
@@ -379,6 +381,24 @@ func updateClusterVersionStatus(cvStatus *configv1.ClusterVersionStatus, status
 		}
 	}
 
+	oldRiCondition := resourcemerge.FindOperatorStatusCondition(cvStatus.Conditions, reconciliationIssuesConditionType)
+	if enabledGates.ReconciliationIssuesCondition() || (oldRiCondition != nil && enabledGates.UnknownVersion()) {
+		riCondition := configv1.ClusterOperatorStatusCondition{
+			Type:    reconciliationIssuesConditionType,
+			Status:  configv1.ConditionFalse,
+			Reason:  noReconciliationIssuesReason,
+			Message: noReconciliationIssuesMessage,
+		}
+		if status.Failure != nil {
+			riCondition.Status = configv1.ConditionTrue
+			riCondition.Reason = reconciliationIssuesFoundReason
+			riCondition.Message = fmt.Sprintf("%s: %s", reconciliationIssuesFoundMessage, status.Failure.Error())
+		}
+		resourcemerge.SetOperatorStatusCondition(&cvStatus.Conditions, riCondition)
+	} else if oldRiCondition != nil {
+		resourcemerge.RemoveOperatorStatusCondition(&cvStatus.Conditions, reconciliationIssuesConditionType)
+	}
+
 	// default retrieved updates if it is not set
 	if resourcemerge.FindOperatorStatusCondition(cvStatus.Conditions, configv1.RetrievedUpdates) == nil {
 		resourcemerge.SetOperatorStatusCondition(&cvStatus.Conditions, configv1.ClusterOperatorStatusCondition{