Merge pull request #1216 from petr-muller/ota-1578-disable-rri

OTA-1578: Disable `ResourceReconciliationIssues` condition

Author: openshift-merge-bot[bot]

pkg/cvo/reconciliation_issues.go

@@ -12,11 +12,6 @@ import (
 
 const (
 	reconciliationIssuesConditionType configv1.ClusterStatusConditionType = "ReconciliationIssues"
-
-	noReconciliationIssuesReason  string = "NoIssues"
-	noReconciliationIssuesMessage string = "No issues found during reconciliation"
-
-	reconciliationIssuesFoundReason string = "IssuesFound"
 )
 
 // errorWalkCallback processes an error.  It returns an error to fail the walk.

pkg/cvo/status.go

@@ -411,25 +411,9 @@ 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 {
-			message, err := reconciliationIssueFromError(status.Failure)
-			if err != nil {
-				message = err.Error()
-			}
-			riCondition.Status = configv1.ConditionTrue
-			riCondition.Reason = reconciliationIssuesFoundReason
-			riCondition.Message = message
-		}
-		resourcemerge.SetOperatorStatusCondition(&cvStatus.Conditions, riCondition)
-	} else if oldRiCondition != nil {
+	// Pre-GA 4.20 TechPreview clusters may have this condition and we need to remove it if it exists when updated to this version
+	// TODO: Remove this code in 4.21
+	if oldRiCondition := resourcemerge.FindOperatorStatusCondition(cvStatus.Conditions, reconciliationIssuesConditionType); oldRiCondition != nil {
 		resourcemerge.RemoveOperatorStatusCondition(&cvStatus.Conditions, reconciliationIssuesConditionType)
 	}
 

pkg/cvo/status_test.go

@@ -200,20 +200,15 @@ func TestOperator_syncFailingStatus(t *testing.T) {
 }
 
 type fakeRiFlags struct {
-	unknownVersion                bool
-	reconciliationIssuesCondition bool
-	statusReleaseArchitecture     bool
-	cvoConfiguration              bool
+	unknownVersion            bool
+	statusReleaseArchitecture bool
+	cvoConfiguration          bool
 }
 
 func (f fakeRiFlags) UnknownVersion() bool {
 	return f.unknownVersion
 }
 
-func (f fakeRiFlags) ReconciliationIssuesCondition() bool {
-	return f.reconciliationIssuesCondition
-}
-
 func (f fakeRiFlags) StatusReleaseArchitecture() bool {
 	return f.statusReleaseArchitecture
 }
@@ -222,59 +217,46 @@ func (f fakeRiFlags) CVOConfiguration() bool {
 	return f.cvoConfiguration
 }
 
-func TestUpdateClusterVersionStatus_UnknownVersionAndReconciliationIssues(t *testing.T) {
-	ignoreLastTransitionTime := cmpopts.IgnoreFields(configv1.ClusterOperatorStatusCondition{}, "LastTransitionTime")
-
+// TODO: Can be removed in 4.21
+func TestUpdateClusterVersionStatus_AlwaysRemove_ReconciliationIssues(t *testing.T) {
 	testCases := []struct {
 		name string
 
 		unknownVersion bool
 		oldCondition   *configv1.ClusterOperatorStatusCondition
 		failure        error
-
-		expectedRiCondition *configv1.ClusterOperatorStatusCondition
 	}{
 		{
-			name:                "ReconciliationIssues disabled, version known, no failure => condition not present",
-			unknownVersion:      false,
-			expectedRiCondition: nil,
+			name:           "version known, no failure => condition not present",
+			unknownVersion: false,
 		},
 		{
-			name:                "ReconciliationIssues disabled, version known, failure => condition not present",
-			unknownVersion:      false,
-			failure:             fmt.Errorf("Something happened"),
-			expectedRiCondition: nil,
+			name:           "version known, failure => condition not present",
+			unknownVersion: false,
+			failure:        fmt.Errorf("something happened"),
 		},
 		{
-			name: "ReconciliationIssues disabled, version unknown, failure, existing condition => condition present",
+			name: "version unknown, failure, existing condition => condition present",
 			oldCondition: &configv1.ClusterOperatorStatusCondition{
 				Type:    reconciliationIssuesConditionType,
 				Status:  configv1.ConditionFalse,
-				Reason:  noReconciliationIssuesReason,
+				Reason:  "noReconciliationIssuesReason",
 				Message: "Happy condition is happy",
 			},
 			unknownVersion: true,
-			failure:        fmt.Errorf("Something happened"),
-			expectedRiCondition: &configv1.ClusterOperatorStatusCondition{
-				Type:    reconciliationIssuesConditionType,
-				Status:  configv1.ConditionTrue,
-				Reason:  reconciliationIssuesFoundReason,
-				Message: `{"message":"Something happened"}`,
-			},
+			failure:        fmt.Errorf("something happened"),
 		},
 		{
-			name:                "ReconciliationIssues disabled, version unknown, failure, no existing condition => condition not present",
-			unknownVersion:      true,
-			failure:             fmt.Errorf("Something happened"),
-			expectedRiCondition: nil,
+			name:           "version unknown, failure, no existing condition => condition not present",
+			unknownVersion: true,
+			failure:        fmt.Errorf("something happened"),
 		},
 	}
 	for _, tc := range testCases {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
 			gates := fakeRiFlags{
-				unknownVersion:                tc.unknownVersion,
-				reconciliationIssuesCondition: false,
+				unknownVersion: tc.unknownVersion,
 			}
 			release := configv1.Release{}
 			getAvailableUpdates := func() *availableUpdates { return nil }
@@ -285,80 +267,15 @@ func TestUpdateClusterVersionStatus_UnknownVersionAndReconciliationIssues(t *tes
 			}
 			updateClusterVersionStatus(&cvStatus, &SyncWorkerStatus{Failure: tc.failure}, release, getAvailableUpdates, gates, noErrors)
 			condition := resourcemerge.FindOperatorStatusCondition(cvStatus.Conditions, reconciliationIssuesConditionType)
-			if diff := cmp.Diff(tc.expectedRiCondition, condition, ignoreLastTransitionTime); diff != "" {
-				t.Errorf("unexpected condition\n:%s", diff)
+			if condition != nil {
+				t.Errorf("expected condition %s to not be present, but it was: %v", reconciliationIssuesConditionType, condition)
 			}
 		})
 
 	}
 
 }
 
-func TestUpdateClusterVersionStatus_ReconciliationIssues(t *testing.T) {
-	ignoreLastTransitionTime := cmpopts.IgnoreFields(configv1.ClusterOperatorStatusCondition{}, "LastTransitionTime")
-
-	testCases := []struct {
-		name             string
-		syncWorkerStatus SyncWorkerStatus
-
-		enabled bool
-
-		expectedCondition *configv1.ClusterOperatorStatusCondition
-	}{
-		{
-			name:             "ReconciliationIssues present and happy when gate is enabled and no failures happened",
-			syncWorkerStatus: SyncWorkerStatus{},
-			enabled:          true,
-			expectedCondition: &configv1.ClusterOperatorStatusCondition{
-				Type:    reconciliationIssuesConditionType,
-				Status:  configv1.ConditionFalse,
-				Reason:  noReconciliationIssuesReason,
-				Message: noReconciliationIssuesMessage,
-			},
-		},
-		{
-			name: "ReconciliationIssues present and unhappy when gate is enabled and failures happened",
-			syncWorkerStatus: SyncWorkerStatus{
-				Failure: fmt.Errorf("Something happened"),
-			},
-			enabled: true,
-			expectedCondition: &configv1.ClusterOperatorStatusCondition{
-				Type:    reconciliationIssuesConditionType,
-				Status:  configv1.ConditionTrue,
-				Reason:  reconciliationIssuesFoundReason,
-				Message: `{"message":"Something happened"}`,
-			},
-		},
-		{
-			name: "ReconciliationIssues not present when gate is enabled and failures happened",
-			syncWorkerStatus: SyncWorkerStatus{
-				Failure: fmt.Errorf("Something happened"),
-			},
-			enabled:           false,
-			expectedCondition: nil,
-		},
-	}
-
-	for _, tc := range testCases {
-		tc := tc
-		t.Run(tc.name, func(t *testing.T) {
-			gates := fakeRiFlags{
-				unknownVersion:                false,
-				reconciliationIssuesCondition: tc.enabled,
-			}
-			release := configv1.Release{}
-			getAvailableUpdates := func() *availableUpdates { return nil }
-			var noErrors field.ErrorList
-			cvStatus := configv1.ClusterVersionStatus{}
-			updateClusterVersionStatus(&cvStatus, &tc.syncWorkerStatus, release, getAvailableUpdates, gates, noErrors)
-			condition := resourcemerge.FindOperatorStatusCondition(cvStatus.Conditions, reconciliationIssuesConditionType)
-			if diff := cmp.Diff(tc.expectedCondition, condition, ignoreLastTransitionTime); diff != "" {
-				t.Errorf("unexpected condition\n:%s", diff)
-			}
-		})
-	}
-}
-
 func TestUpdateClusterVersionStatus_FilteringMultipleErrorsForFailingCondition(t *testing.T) {
 	ignoreLastTransitionTime := cmpopts.IgnoreFields(configv1.ClusterOperatorStatusCondition{}, "LastTransitionTime")
 	type args struct {

pkg/featuregates/featuregates.go

@@ -28,14 +28,6 @@ type CvoGateChecker interface {
 	// to restart when the flags change.
 	UnknownVersion() bool
 
-	// ReconciliationIssuesCondition controls whether CVO maintains a Condition with
-	// ReconciliationIssues 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.
-	ReconciliationIssuesCondition() bool
-
 	// StatusReleaseArchitecture controls whether CVO populates
 	// Release.Architecture in status properties like status.desired and status.history[].
 	StatusReleaseArchitecture() bool
@@ -54,14 +46,9 @@ type CvoGates struct {
 	desiredVersion string
 
 	// individual flags mirror the CvoGateChecker interface
-	unknownVersion                bool
-	reconciliationIssuesCondition bool
-	statusReleaseArchitecture     bool
-	cvoConfiguration              bool
-}
-
-func (c CvoGates) ReconciliationIssuesCondition() bool {
-	return c.reconciliationIssuesCondition
+	unknownVersion            bool
+	statusReleaseArchitecture bool
+	cvoConfiguration          bool
 }
 
 func (c CvoGates) StatusReleaseArchitecture() bool {
@@ -79,11 +66,10 @@ func (c CvoGates) CVOConfiguration() bool {
 // DefaultCvoGates apply when actual features for given version are unknown
 func DefaultCvoGates(version string) CvoGates {
 	return CvoGates{
-		desiredVersion:                version,
-		unknownVersion:                true,
-		reconciliationIssuesCondition: false,
-		statusReleaseArchitecture:     false,
-		cvoConfiguration:              false,
+		desiredVersion:            version,
+		unknownVersion:            true,
+		statusReleaseArchitecture: false,
+		cvoConfiguration:          false,
 	}
 }
 
@@ -101,8 +87,6 @@ func CvoGatesFromFeatureGate(gate *configv1.FeatureGate, version string) CvoGate
 		enabledGates.unknownVersion = false
 		for _, enabled := range g.Enabled {
 			switch enabled.Name {
-			case features.FeatureGateUpgradeStatus:
-				enabledGates.reconciliationIssuesCondition = true
 			case features.FeatureGateImageStreamImportMode:
 				enabledGates.statusReleaseArchitecture = true
 			case features.FeatureGateCVOConfiguration:
@@ -111,8 +95,6 @@ func CvoGatesFromFeatureGate(gate *configv1.FeatureGate, version string) CvoGate
 		}
 		for _, disabled := range g.Disabled {
 			switch disabled.Name {
-			case features.FeatureGateUpgradeStatus:
-				enabledGates.reconciliationIssuesCondition = false
 			case features.FeatureGateImageStreamImportMode:
 				enabledGates.statusReleaseArchitecture = false
 			case features.FeatureGateCVOConfiguration: