From 424e392b2e63ff18478a603e24e197b737017fda Mon Sep 17 00:00:00 2001 From: Vedant Durgam Date: Wed, 8 Jul 2026 23:06:57 +0530 Subject: [PATCH 1/2] fix: set Progressing condition to False on successful reconciliation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Progressing condition on ClusterExtension resources was always set to status: True with reason: Succeeded after a successful reconciliation. This is semantically incorrect — Progressing=True means "still working," which contradicts the Succeeded reason and the "Desired state reached" message. This fix changes the default Progressing status to ConditionFalse (done progressing) and only sets ConditionTrue when there is an active non-terminal error causing retries. Terminal errors correctly remain ConditionFalse with reason Blocked. Before: Installed=True Progressing=True reason=Succeeded "Desired state reached" After: Installed=True Progressing=False reason=Succeeded "Desired state reached" Signed-off-by: Venkatesh Durgam Co-authored-by: Cursor --- .../controllers/clusterextension_controller_test.go | 10 +++++----- .../controllers/common_controller.go | 3 ++- .../controllers/common_controller_test.go | 4 ++-- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/internal/operator-controller/controllers/clusterextension_controller_test.go b/internal/operator-controller/controllers/clusterextension_controller_test.go index 675305e858..05985163b6 100644 --- a/internal/operator-controller/controllers/clusterextension_controller_test.go +++ b/internal/operator-controller/controllers/clusterextension_controller_test.go @@ -1352,7 +1352,7 @@ func TestClusterExtensionInstallationSucceeds(t *testing.T) { t.Log("By checking the expected progressing conditions") progressingCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeProgressing) require.NotNil(t, progressingCond) - require.Equal(t, metav1.ConditionTrue, progressingCond.Status) + require.Equal(t, metav1.ConditionFalse, progressingCond.Status) require.Equal(t, ocv1.ReasonSucceeded, progressingCond.Reason) require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{})) @@ -2674,7 +2674,7 @@ func TestResolutionFallbackToInstalledBundle(t *testing.T) { progCond := apimeta.FindStatusCondition(ext.Status.Conditions, ocv1.TypeProgressing) require.NotNil(t, progCond) - require.Equal(t, metav1.ConditionTrue, progCond.Status) + require.Equal(t, metav1.ConditionFalse, progCond.Status) require.Equal(t, ocv1.ReasonSucceeded, progCond.Reason) // Verify all conditions are present and valid after first reconcile @@ -2695,7 +2695,7 @@ func TestResolutionFallbackToInstalledBundle(t *testing.T) { // Progressing should be Succeeded (apply completed successfully) progCond = apimeta.FindStatusCondition(ext.Status.Conditions, ocv1.TypeProgressing) require.NotNil(t, progCond) - require.Equal(t, metav1.ConditionTrue, progCond.Status) + require.Equal(t, metav1.ConditionFalse, progCond.Status) require.Equal(t, ocv1.ReasonSucceeded, progCond.Reason) // Installed should be True (maintaining current version) @@ -2828,7 +2828,7 @@ func TestResolutionFallbackToInstalledBundle(t *testing.T) { progCond := apimeta.FindStatusCondition(ext.Status.Conditions, ocv1.TypeProgressing) require.NotNil(t, progCond) - require.Equal(t, metav1.ConditionTrue, progCond.Status) + require.Equal(t, metav1.ConditionFalse, progCond.Status) require.Equal(t, ocv1.ReasonSucceeded, progCond.Reason) // Note: When falling back without catalog access initially, deprecation conditions @@ -2851,7 +2851,7 @@ func TestResolutionFallbackToInstalledBundle(t *testing.T) { progCond = apimeta.FindStatusCondition(ext.Status.Conditions, ocv1.TypeProgressing) require.NotNil(t, progCond) - require.Equal(t, metav1.ConditionTrue, progCond.Status) + require.Equal(t, metav1.ConditionFalse, progCond.Status) require.Equal(t, ocv1.ReasonSucceeded, progCond.Reason) // Verify all conditions remain valid after upgrade diff --git a/internal/operator-controller/controllers/common_controller.go b/internal/operator-controller/controllers/common_controller.go index 46197c6c36..1608d79203 100644 --- a/internal/operator-controller/controllers/common_controller.go +++ b/internal/operator-controller/controllers/common_controller.go @@ -148,13 +148,14 @@ func setInstallStatus(ext *ocv1.ClusterExtension, installStatus *ocv1.ClusterExt func setStatusProgressing(ext *ocv1.ClusterExtension, err error) { progressingCond := metav1.Condition{ Type: ocv1.TypeProgressing, - Status: metav1.ConditionTrue, + Status: metav1.ConditionFalse, Reason: ocv1.ReasonSucceeded, Message: "Desired state reached", ObservedGeneration: ext.GetGeneration(), } if err != nil { + progressingCond.Status = metav1.ConditionTrue progressingCond.Reason = ocv1.ReasonRetrying // Unwrap TerminalError to avoid "terminal error:" prefix in message progressingCond.Message = errorutil.SanitizeNetworkError(errorutil.UnwrapTerminal(err)) diff --git a/internal/operator-controller/controllers/common_controller_test.go b/internal/operator-controller/controllers/common_controller_test.go index 6edf8751ba..9d8abced6a 100644 --- a/internal/operator-controller/controllers/common_controller_test.go +++ b/internal/operator-controller/controllers/common_controller_test.go @@ -26,12 +26,12 @@ func TestSetStatusProgressing(t *testing.T) { expected metav1.Condition }{ { - name: "non-nil ClusterExtension, nil error, Progressing condition has status True with reason Success", + name: "non-nil ClusterExtension, nil error, Progressing condition has status False with reason Success", err: nil, clusterExtension: &ocv1.ClusterExtension{}, expected: metav1.Condition{ Type: ocv1.TypeProgressing, - Status: metav1.ConditionTrue, + Status: metav1.ConditionFalse, Reason: ocv1.ReasonSucceeded, Message: "Desired state reached", }, From 722ae1a1e367ba955c83a9b6931ea1fd8e3acbde Mon Sep 17 00:00:00 2001 From: Vedant Durgam Date: Thu, 9 Jul 2026 00:06:06 +0530 Subject: [PATCH 2/2] fix: update e2e tests to expect Progressing=False on successful rollout Update ClusterExtension e2e assertions to match the corrected Progressing condition semantics. ClusterObjectSet assertions are unchanged as they use a separate controller. Signed-off-by: Venkatesh Durgam Co-authored-by: Cursor --- test/e2e/features/install.feature | 2 +- test/e2e/features/recover.feature | 8 ++++---- test/e2e/steps/steps.go | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/test/e2e/features/install.feature b/test/e2e/features/install.feature index e82ad10e1d..83465507c0 100644 --- a/test/e2e/features/install.feature +++ b/test/e2e/features/install.feature @@ -676,5 +676,5 @@ Feature: Install ClusterExtension """ When ServiceAccount "olm-sa" with needed permissions is available in test namespace Then ClusterExtension is available - And ClusterExtension reports Progressing as True with Reason Succeeded + And ClusterExtension reports Progressing as False with Reason Succeeded And ClusterExtension reports Installed as True diff --git a/test/e2e/features/recover.feature b/test/e2e/features/recover.feature index 1dbc7bd753..7513a51113 100644 --- a/test/e2e/features/recover.feature +++ b/test/e2e/features/recover.feature @@ -54,7 +54,7 @@ Feature: Recover cluster extension from errors that might occur during its lifet And ClusterExtension reports Progressing as True with Reason Retrying When ServiceAccount "olm-sa" with needed permissions is available in test namespace Then ClusterExtension is available - And ClusterExtension reports Progressing as True with Reason Succeeded + And ClusterExtension reports Progressing as False with Reason Succeeded Scenario: Install ClusterExtension after conflicting resource is removed Given ServiceAccount "olm-sa" with needed permissions is available in test namespace @@ -114,7 +114,7 @@ Feature: Recover cluster extension from errors that might occur during its lifet And ClusterExtension reports Installed as False When resource "deployment/test-operator-${SCENARIO_ID}" is removed Then ClusterExtension is available - And ClusterExtension reports Progressing as True with Reason Succeeded + And ClusterExtension reports Progressing as False with Reason Succeeded And ClusterExtension reports Installed as True @PreflightPermissions @@ -149,7 +149,7 @@ Feature: Recover cluster extension from errors that might occur during its lifet """ When ServiceAccount "olm-sa" with needed permissions is available in test namespace Then ClusterExtension is available - And ClusterExtension reports Progressing as True with Reason Succeeded + And ClusterExtension reports Progressing as False with Reason Succeeded And ClusterExtension reports Installed as True # CATALOG DELETION RESILIENCE SCENARIOS @@ -250,6 +250,6 @@ Feature: Recover cluster extension from errors that might occur during its lifet "olm.operatorframework.io/metadata.name": ${CATALOG:test} """ And ClusterExtension latest generation has been reconciled - And ClusterExtension reports Progressing as True with Reason Succeeded + And ClusterExtension reports Progressing as False with Reason Succeeded Then ClusterExtension is available And ClusterExtension reports Installed as True diff --git a/test/e2e/steps/steps.go b/test/e2e/steps/steps.go index c987ef44c4..44fb6ca64a 100644 --- a/test/e2e/steps/steps.go +++ b/test/e2e/steps/steps.go @@ -584,7 +584,7 @@ func ClusterExtensionReconciledLatestGeneration(ctx context.Context) error { return nil } -// ClusterExtensionIsRolledOut waits for the ClusterExtension's Progressing condition to be True with reason Succeeded, +// ClusterExtensionIsRolledOut waits for the ClusterExtension's Progressing condition to be False with reason Succeeded, // then gathers its constituent resources into the scenario context. Polls with timeout. func ClusterExtensionIsRolledOut(ctx context.Context) error { sc := scenarioCtx(ctx) @@ -599,7 +599,7 @@ func ClusterExtensionIsRolledOut(ctx context.Context) error { return false } - return condition["status"] == "True" && condition["reason"] == "Succeeded" && condition["type"] == "Progressing" + return condition["status"] == "False" && condition["reason"] == "Succeeded" && condition["type"] == "Progressing" }, timeout, tick) if rec := RecorderFromContext(ctx); rec != nil {