Skip to content

Commit 88f1f38

Browse files
Per Goncalves da Silvaclaude
Per Goncalves da Silva
and
claude
committed
Call RevisionEngine.Teardown when CER is archived
When a ClusterExtensionRevision transitions to the Archived lifecycle state, invoke the revision engine's Teardown method to clean up managed resources that are no longer part of the active revision. This ensures resources removed between bundle versions (e.g. a ConfigMap present in v1.0.0 but absent in v1.2.0) are deleted from the cluster. Changes: - Split teardown() into delete() (CER deletion) and archive() (CER archival); only archive() calls revisionEngine.Teardown() - Move RevisionEngine creation and watch establishment before the archive check so they are available for both archive and reconcile paths - Handle incomplete teardown (requeue after 5s) and teardown errors (return error for controller retry) - Rename rev variable to cer for consistency with the type name - Update unit tests: rename to ArchivalAndDeletion, add test cases for incomplete teardown requeue, teardown error propagation, and factory creation error during archived teardown - Add dummy-configmap to v1.0.0 test bundle (absent in v1.2.0) to validate teardown on archival in e2e tests - Add e2e step ClusterExtensionRevision phase objects are not found or not owned by the revision: fetches all objects from the CER's phases and asserts each one either does not exist or does not list the CER in its ownerReferences - Change extensionObjects in scenarioContext from []client.Object to [][]client.Object to track objects per revision rollout; add GetClusterExtensionObjectsForRevision helper - Replace specific configmap-not-found assertion in the update e2e scenario with the new generic phase-objects step Signed-off-by: Per Goncalves da Silva <[email protected]> Signed-off-by: Per G. da Silva <[email protected]> Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: Per G. da Silva <[email protected]>
1 parent 1ef820f commit 88f1f38

File tree

5 files changed

+221
-56
lines changed

5 files changed

+221
-56
lines changed

‎internal/operator-controller/controllers/clusterextensionrevision_controller.go‎

Lines changed: 38 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -134,20 +134,30 @@ func checkForUnexpectedClusterExtensionRevisionFieldChange(a, b ocv1.ClusterExte
134134
func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev *ocv1.ClusterExtensionRevision) (ctrl.Result, error) {
135135
l := log.FromContext(ctx)
136136

137+
if !rev.DeletionTimestamp.IsZero() {
138+
return c.delete(ctx, rev)
139+
}
140+
137141
revision, opts, err := c.toBoxcutterRevision(ctx, rev)
138142
if err != nil {
139143
setRetryingConditions(rev, err.Error())
140144
return ctrl.Result{}, fmt.Errorf("converting to boxcutter revision: %v", err)
141145
}
142146

143-
if !rev.DeletionTimestamp.IsZero() || rev.Spec.LifecycleState == ocv1.ClusterExtensionRevisionLifecycleStateArchived {
144-
return c.teardown(ctx, rev)
147+
revisionEngine, err := c.RevisionEngineFactory.CreateRevisionEngine(ctx, rev)
148+
if err != nil {
149+
setRetryingConditions(rev, err.Error())
150+
return ctrl.Result{}, fmt.Errorf("failed to create revision engine: %v", err)
151+
}
152+
153+
if rev.Spec.LifecycleState == ocv1.ClusterExtensionRevisionLifecycleStateArchived {
154+
if err := c.TrackingCache.Free(ctx, rev); err != nil {
155+
markAsAvailableUnknown(rev, ocv1.ClusterExtensionRevisionReasonReconciling, err.Error())
156+
return ctrl.Result{}, fmt.Errorf("error stopping informers: %v", err)
157+
}
158+
return c.archive(ctx, revisionEngine, rev, revision)
145159
}
146160

147-
revVersion := rev.GetAnnotations()[labels.BundleVersionKey]
148-
//
149-
// Reconcile
150-
//
151161
if err := c.ensureFinalizer(ctx, rev, clusterExtensionRevisionTeardownFinalizer); err != nil {
152162
return ctrl.Result{}, fmt.Errorf("error ensuring teardown finalizer: %v", err)
153163
}
@@ -158,12 +168,6 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
158168
return ctrl.Result{}, werr
159169
}
160170

161-
revisionEngine, err := c.RevisionEngineFactory.CreateRevisionEngine(ctx, rev)
162-
if err != nil {
163-
setRetryingConditions(rev, err.Error())
164-
return ctrl.Result{}, fmt.Errorf("failed to create revision engine: %v", err)
165-
}
166-
167171
rres, err := revisionEngine.Reconcile(ctx, *revision, opts...)
168172
if err != nil {
169173
if rres != nil {
@@ -203,6 +207,7 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
203207
}
204208
}
205209

210+
revVersion := rev.GetAnnotations()[labels.BundleVersionKey]
206211
if !rres.InTransition() {
207212
markAsProgressing(rev, ocv1.ReasonSucceeded, fmt.Sprintf("Revision %s has rolled out.", revVersion))
208213
} else {
@@ -275,18 +280,33 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
275280
return ctrl.Result{}, nil
276281
}
277282

278-
func (c *ClusterExtensionRevisionReconciler) teardown(ctx context.Context, rev *ocv1.ClusterExtensionRevision) (ctrl.Result, error) {
279-
if err := c.TrackingCache.Free(ctx, rev); err != nil {
280-
markAsAvailableUnknown(rev, ocv1.ClusterExtensionRevisionReasonReconciling, err.Error())
283+
func (c *ClusterExtensionRevisionReconciler) delete(ctx context.Context, cer *ocv1.ClusterExtensionRevision) (ctrl.Result, error) {
284+
if err := c.TrackingCache.Free(ctx, cer); err != nil {
285+
markAsAvailableUnknown(cer, ocv1.ClusterExtensionRevisionReasonReconciling, err.Error())
281286
return ctrl.Result{}, fmt.Errorf("error stopping informers: %v", err)
282287
}
288+
if err := c.removeFinalizer(ctx, cer, clusterExtensionRevisionTeardownFinalizer); err != nil {
289+
return ctrl.Result{}, fmt.Errorf("error removing teardown finalizer: %v", err)
290+
}
291+
return ctrl.Result{}, nil
292+
}
283293

294+
func (c *ClusterExtensionRevisionReconciler) archive(ctx context.Context, revisionEngine RevisionEngine, cer *ocv1.ClusterExtensionRevision, revision *boxcutter.Revision) (ctrl.Result, error) {
295+
tdres, err := revisionEngine.Teardown(ctx, *revision)
296+
if err != nil {
297+
err = fmt.Errorf("error archiving revision: %v", err)
298+
setRetryingConditions(cer, err.Error())
299+
return ctrl.Result{}, err
300+
}
301+
if tdres != nil && !tdres.IsComplete() {
302+
setRetryingConditions(cer, "removing revision resources that are not owned by another revision")
303+
return ctrl.Result{RequeueAfter: 5 * time.Second}, nil
304+
}
284305
// Ensure conditions are set before removing the finalizer when archiving
285-
if rev.Spec.LifecycleState == ocv1.ClusterExtensionRevisionLifecycleStateArchived && markAsArchived(rev) {
306+
if markAsArchived(cer) {
286307
return ctrl.Result{}, nil
287308
}
288-
289-
if err := c.removeFinalizer(ctx, rev, clusterExtensionRevisionTeardownFinalizer); err != nil {
309+
if err := c.removeFinalizer(ctx, cer, clusterExtensionRevisionTeardownFinalizer); err != nil {
290310
return ctrl.Result{}, fmt.Errorf("error removing teardown finalizer: %v", err)
291311
}
292312
return ctrl.Result{}, nil

‎internal/operator-controller/controllers/clusterextensionrevision_controller_test.go‎

Lines changed: 105 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import (
99

1010
"github.com/stretchr/testify/require"
1111
corev1 "k8s.io/api/core/v1"
12-
apierrors "k8s.io/apimachinery/pkg/api/errors"
1312
"k8s.io/apimachinery/pkg/api/meta"
1413
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1514
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -628,7 +627,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_ValidationError_Retries(t
628627
}
629628
}
630629

631-
func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) {
630+
func Test_ClusterExtensionRevisionReconciler_Reconcile_ArchivalAndDeletion(t *testing.T) {
632631
const (
633632
clusterExtensionRevisionName = "test-ext-1"
634633
)
@@ -641,9 +640,11 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) {
641640
existingObjs func() []client.Object
642641
revisionResult machinery.RevisionResult
643642
revisionEngineTeardownFn func(*testing.T) func(context.Context, machinerytypes.Revision, ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error)
643+
revisionEngineFactoryErr error
644644
validate func(*testing.T, client.Client)
645645
trackingCacheFreeFn func(context.Context, client.Object) error
646646
expectedErr string
647+
expectedResult ctrl.Result
647648
}{
648649
{
649650
name: "teardown finalizer is removed",
@@ -669,7 +670,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) {
669670
},
670671
},
671672
{
672-
name: "revision is torn down and deleted when deleted",
673+
name: "set Available:Unknown:Reconciling when tracking cache free fails during deletion",
673674
revisionResult: mockRevisionResult{},
674675
existingObjs: func() []client.Object {
675676
ext := newTestClusterExtension()
@@ -678,6 +679,38 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) {
678679
"olm.operatorframework.io/teardown",
679680
}
680681
rev1.DeletionTimestamp = &metav1.Time{Time: time.Now()}
682+
return []client.Object{ext, rev1}
683+
},
684+
trackingCacheFreeFn: func(_ context.Context, _ client.Object) error {
685+
return fmt.Errorf("tracking cache free failed")
686+
},
687+
expectedErr: "error stopping informers",
688+
validate: func(t *testing.T, c client.Client) {
689+
rev := &ocv1.ClusterExtensionRevision{}
690+
err := c.Get(t.Context(), client.ObjectKey{
691+
Name: clusterExtensionRevisionName,
692+
}, rev)
693+
require.NoError(t, err)
694+
cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeAvailable)
695+
require.NotNil(t, cond)
696+
require.Equal(t, metav1.ConditionUnknown, cond.Status)
697+
require.Equal(t, ocv1.ClusterExtensionRevisionReasonReconciling, cond.Reason)
698+
require.Contains(t, cond.Message, "tracking cache free failed")
699+
},
700+
revisionEngineTeardownFn: func(t *testing.T) func(context.Context, machinerytypes.Revision, ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error) {
701+
return nil
702+
},
703+
},
704+
{
705+
name: "set Available:Archived:Unknown and Progressing:False:Archived conditions when a revision is archived",
706+
revisionResult: mockRevisionResult{},
707+
existingObjs: func() []client.Object {
708+
ext := newTestClusterExtension()
709+
rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName, ext, testScheme)
710+
rev1.Finalizers = []string{
711+
"olm.operatorframework.io/teardown",
712+
}
713+
rev1.Spec.LifecycleState = ocv1.ClusterExtensionRevisionLifecycleStateArchived
681714
return []client.Object{rev1, ext}
682715
},
683716
revisionEngineTeardownFn: func(t *testing.T) func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error) {
@@ -688,55 +721,64 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) {
688721
}
689722
},
690723
validate: func(t *testing.T, c client.Client) {
691-
t.Log("cluster revision is deleted")
692724
rev := &ocv1.ClusterExtensionRevision{}
693725
err := c.Get(t.Context(), client.ObjectKey{
694726
Name: clusterExtensionRevisionName,
695727
}, rev)
696-
require.Error(t, err)
697-
require.True(t, apierrors.IsNotFound(err))
728+
require.NoError(t, err)
729+
cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeAvailable)
730+
require.NotNil(t, cond)
731+
require.Equal(t, metav1.ConditionUnknown, cond.Status)
732+
require.Equal(t, ocv1.ClusterExtensionRevisionReasonArchived, cond.Reason)
733+
require.Equal(t, "revision is archived", cond.Message)
734+
require.Equal(t, int64(1), cond.ObservedGeneration)
735+
736+
cond = meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeProgressing)
737+
require.NotNil(t, cond)
738+
require.Equal(t, metav1.ConditionFalse, cond.Status)
739+
require.Equal(t, ocv1.ClusterExtensionRevisionReasonArchived, cond.Reason)
740+
require.Equal(t, "revision is archived", cond.Message)
741+
require.Equal(t, int64(1), cond.ObservedGeneration)
698742
},
699743
},
700744
{
701-
name: "set Available:Unknown:Reconciling and surface tracking cache cleanup errors when deleted",
745+
name: "set Progressing:True:Retrying and requeue when archived revision archival is incomplete",
702746
revisionResult: mockRevisionResult{},
703747
existingObjs: func() []client.Object {
704748
ext := newTestClusterExtension()
705749
rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName, ext, testScheme)
706750
rev1.Finalizers = []string{
707751
"olm.operatorframework.io/teardown",
708752
}
709-
rev1.DeletionTimestamp = &metav1.Time{Time: time.Now()}
753+
rev1.Spec.LifecycleState = ocv1.ClusterExtensionRevisionLifecycleStateArchived
710754
return []client.Object{rev1, ext}
711755
},
712756
revisionEngineTeardownFn: func(t *testing.T) func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error) {
713757
return func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error) {
714758
return &mockRevisionTeardownResult{
715-
isComplete: true,
759+
isComplete: false,
716760
}, nil
717761
}
718762
},
719-
trackingCacheFreeFn: func(ctx context.Context, object client.Object) error {
720-
return fmt.Errorf("some tracking cache cleanup error")
721-
},
722-
expectedErr: "some tracking cache cleanup error",
763+
expectedResult: ctrl.Result{RequeueAfter: 5 * time.Second},
723764
validate: func(t *testing.T, c client.Client) {
724-
t.Log("cluster revision is not deleted and still contains finalizer")
725765
rev := &ocv1.ClusterExtensionRevision{}
726766
err := c.Get(t.Context(), client.ObjectKey{
727767
Name: clusterExtensionRevisionName,
728768
}, rev)
729769
require.NoError(t, err)
730-
cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeAvailable)
770+
cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeProgressing)
731771
require.NotNil(t, cond)
732-
require.Equal(t, metav1.ConditionUnknown, cond.Status)
733-
require.Equal(t, ocv1.ClusterExtensionRevisionReasonReconciling, cond.Reason)
734-
require.Equal(t, "some tracking cache cleanup error", cond.Message)
735-
require.Equal(t, int64(1), cond.ObservedGeneration)
772+
require.Equal(t, metav1.ConditionTrue, cond.Status)
773+
require.Equal(t, ocv1.ClusterExtensionRevisionReasonRetrying, cond.Reason)
774+
require.Equal(t, "removing revision resources that are not owned by another revision", cond.Message)
775+
776+
// Finalizer should still be present
777+
require.Contains(t, rev.Finalizers, "olm.operatorframework.io/teardown")
736778
},
737779
},
738780
{
739-
name: "set Available:Archived:Unknown and Progressing:False:Archived conditions when a revision is archived",
781+
name: "return error and set retrying conditions when archived revision teardown fails",
740782
revisionResult: mockRevisionResult{},
741783
existingObjs: func() []client.Object {
742784
ext := newTestClusterExtension()
@@ -749,30 +791,57 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) {
749791
},
750792
revisionEngineTeardownFn: func(t *testing.T) func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error) {
751793
return func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error) {
752-
return &mockRevisionTeardownResult{
753-
isComplete: true,
754-
}, nil
794+
return nil, fmt.Errorf("teardown failed: connection refused")
755795
}
756796
},
797+
expectedErr: "error archiving revision",
757798
validate: func(t *testing.T, c client.Client) {
758799
rev := &ocv1.ClusterExtensionRevision{}
759800
err := c.Get(t.Context(), client.ObjectKey{
760801
Name: clusterExtensionRevisionName,
761802
}, rev)
762803
require.NoError(t, err)
763-
cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeAvailable)
804+
cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeProgressing)
764805
require.NotNil(t, cond)
765-
require.Equal(t, metav1.ConditionUnknown, cond.Status)
766-
require.Equal(t, ocv1.ClusterExtensionRevisionReasonArchived, cond.Reason)
767-
require.Equal(t, "revision is archived", cond.Message)
768-
require.Equal(t, int64(1), cond.ObservedGeneration)
806+
require.Equal(t, metav1.ConditionTrue, cond.Status)
807+
require.Equal(t, ocv1.ClusterExtensionRevisionReasonRetrying, cond.Reason)
808+
require.Contains(t, cond.Message, "teardown failed: connection refused")
769809

770-
cond = meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeProgressing)
810+
// Finalizer should still be present
811+
require.Contains(t, rev.Finalizers, "olm.operatorframework.io/teardown")
812+
},
813+
},
814+
{
815+
name: "return error and set retrying conditions when factory fails to create engine during archived teardown",
816+
revisionResult: mockRevisionResult{},
817+
existingObjs: func() []client.Object {
818+
ext := newTestClusterExtension()
819+
rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName, ext, testScheme)
820+
rev1.Finalizers = []string{
821+
"olm.operatorframework.io/teardown",
822+
}
823+
rev1.Spec.LifecycleState = ocv1.ClusterExtensionRevisionLifecycleStateArchived
824+
return []client.Object{rev1, ext}
825+
},
826+
revisionEngineTeardownFn: func(t *testing.T) func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error) {
827+
return nil
828+
},
829+
revisionEngineFactoryErr: fmt.Errorf("token getter failed"),
830+
expectedErr: "failed to create revision engine",
831+
validate: func(t *testing.T, c client.Client) {
832+
rev := &ocv1.ClusterExtensionRevision{}
833+
err := c.Get(t.Context(), client.ObjectKey{
834+
Name: clusterExtensionRevisionName,
835+
}, rev)
836+
require.NoError(t, err)
837+
cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeProgressing)
771838
require.NotNil(t, cond)
772-
require.Equal(t, metav1.ConditionFalse, cond.Status)
773-
require.Equal(t, ocv1.ClusterExtensionRevisionReasonArchived, cond.Reason)
774-
require.Equal(t, "revision is archived", cond.Message)
775-
require.Equal(t, int64(1), cond.ObservedGeneration)
839+
require.Equal(t, metav1.ConditionTrue, cond.Status)
840+
require.Equal(t, ocv1.ClusterExtensionRevisionReasonRetrying, cond.Reason)
841+
require.Contains(t, cond.Message, "token getter failed")
842+
843+
// Finalizer should still be present
844+
require.Contains(t, rev.Finalizers, "olm.operatorframework.io/teardown")
776845
},
777846
},
778847
{
@@ -833,9 +902,10 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) {
833902
},
834903
teardown: tc.revisionEngineTeardownFn(t),
835904
}
905+
factory := &mockRevisionEngineFactory{engine: mockEngine, createErr: tc.revisionEngineFactoryErr}
836906
result, err := (&controllers.ClusterExtensionRevisionReconciler{
837907
Client: testClient,
838-
RevisionEngineFactory: &mockRevisionEngineFactory{engine: mockEngine},
908+
RevisionEngineFactory: factory,
839909
TrackingCache: &mockTrackingCache{
840910
client: testClient,
841911
freeFn: tc.trackingCacheFreeFn,
@@ -847,7 +917,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) {
847917
})
848918

849919
// reconcile cluster extension revision
850-
require.Equal(t, ctrl.Result{}, result)
920+
require.Equal(t, tc.expectedResult, result)
851921
if tc.expectedErr != "" {
852922
require.Contains(t, err.Error(), tc.expectedErr)
853923
} else {

‎test/e2e/features/update.feature‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ Feature: Update ClusterExtension
181181
Then bundle "test-operator.1.3.0" is installed in version "1.3.0"
182182

183183
@BoxcutterRuntime
184-
Scenario: Each update creates a new revision
184+
Scenario: Each update creates a new revision and resources not present in the new revision are removed from the cluster
185185
Given ClusterExtension is applied
186186
"""
187187
apiVersion: olm.operatorframework.io/v1
@@ -212,6 +212,7 @@ Feature: Update ClusterExtension
212212
And ClusterExtensionRevision "${NAME}-2" reports Progressing as True with Reason Succeeded
213213
And ClusterExtensionRevision "${NAME}-2" reports Available as True with Reason ProbesSucceeded
214214
And ClusterExtensionRevision "${NAME}-1" is archived
215+
And ClusterExtensionRevision "${NAME}-1" phase objects are not found or not owned by the revision
215216

216217
@BoxcutterRuntime
217218
Scenario: Report all active revisions on ClusterExtension

0 commit comments

Comments
 (0)