diff --git a/pkg/cvo/reconciliation_issues.go b/pkg/cvo/reconciliation_issues.go index 2a6eab313..8c43ea065 100644 --- a/pkg/cvo/reconciliation_issues.go +++ b/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. diff --git a/pkg/cvo/status.go b/pkg/cvo/status.go index 7429ce10f..c97608426 100644 --- a/pkg/cvo/status.go +++ b/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) } diff --git a/pkg/cvo/status_test.go b/pkg/cvo/status_test.go index 38738a889..8f720d2c1 100644 --- a/pkg/cvo/status_test.go +++ b/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,8 +267,8 @@ 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) } }) @@ -294,71 +276,6 @@ func TestUpdateClusterVersionStatus_UnknownVersionAndReconciliationIssues(t *tes } -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 { diff --git a/pkg/featuregates/featuregates.go b/pkg/featuregates/featuregates.go index b8c24059e..27fd627c3 100644 --- a/pkg/featuregates/featuregates.go +++ b/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: