From de75b8e569a538b35cbeec5bc9dd26a6c28ebb1b Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Fri, 23 Oct 2026 14:50:32 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20report=20Archiving=20status=20wh?= =?UTF-8?q?en=20ClusterObjectSet=20teardown=20retries=20fail?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When RevisionEngine.Teardown returns an error during archival, the archive path returned the error for backoff but left the Ready condition unchanged. A revision whose teardown first fails before any Archiving status was set would keep displaying its prior condition (e.g. AllObjectsReady), masking that cleanup is still in progress. Set Ready=False/Archiving in the teardown error branch before returning the error. This keeps the surfaced status honest during teardown retries and is consistent with the incomplete-but-no-error branch, which already reports Archiving. The condition is never set to Archived here, so the ClusterObjectDeployment garbage collector (which prunes only revisions whose Ready reason is Archived) still cannot select a revision mid-teardown. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Per G. da Silva --- .../controllers/clusterobjectset_controller.go | 6 ++++-- .../controllers/clusterobjectset_controller_test.go | 12 ++++++++---- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/internal/object-controller/controllers/clusterobjectset_controller.go b/internal/object-controller/controllers/clusterobjectset_controller.go index 6354f35abe..92b35db789 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller.go +++ b/internal/object-controller/controllers/clusterobjectset_controller.go @@ -417,8 +417,10 @@ func (c *ClusterObjectSetReconciler) delete(ctx context.Context, cos *ocv1.Clust func (c *ClusterObjectSetReconciler) archive(ctx context.Context, revisionEngine RevisionEngine, cos *ocv1.ClusterObjectSet, revision boxcutter.RevisionBuilder) (ctrl.Result, error) { tdres, err := revisionEngine.Teardown(ctx, revision, machinerytypes.WithObserveAfterIncomplete{}) if err != nil { - // Transient teardown error: leave the Ready condition unchanged and return - // the error for rate-limited backoff. + // Teardown is still in progress: surface Archiving so the status reflects that + // cleanup has not completed (never Archived, which the deployment GC treats as + // torn-down), then return the error for rate-limited backoff. + markAsArchiving(cos) return ctrl.Result{}, fmt.Errorf("error archiving revision: %v", err) } // Set object counts diff --git a/internal/object-controller/controllers/clusterobjectset_controller_test.go b/internal/object-controller/controllers/clusterobjectset_controller_test.go index f5e830526a..51f3c8502d 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller_test.go +++ b/internal/object-controller/controllers/clusterobjectset_controller_test.go @@ -1299,7 +1299,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_ArchivalAndDeletion(t *testing.T) }, }, { - name: "return error and leave Ready untouched when archived revision teardown fails", + name: "set Ready:False:Archiving and return error when archived revision teardown fails", revisionResult: newMockRevisionResult(mockCtrl, revisionResultConfig{}), existingObjs: func() []client.Object { ext := newTestClusterExtension() @@ -1322,9 +1322,13 @@ func Test_ClusterObjectSetReconciler_Reconcile_ArchivalAndDeletion(t *testing.T) Name: clusterObjectSetName, }, rev) require.NoError(t, err) - // A teardown error is transient: the error is returned for backoff and the - // Ready condition is left untouched (absent here). - require.Nil(t, meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeReady)) + // A teardown error means cleanup is still in progress: the error is returned + // for backoff and Ready reports Archiving (never Archived, which the deployment + // GC would treat as fully torn down). + cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeReady) + require.NotNil(t, cond) + require.Equal(t, metav1.ConditionFalse, cond.Status) + require.Equal(t, ocv1.ClusterObjectSetReasonArchiving, cond.Reason) // Finalizer should still be present require.Contains(t, rev.Finalizers, "olm.operatorframework.io/teardown")