Skip to content

Commit b0af690

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 <pegoncal@redhat.com> Signed-off-by: Per G. da Silva <pegoncal@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Per G. da Silva <pegoncal@redhat.com>
1 parent 1ef820f commit b0af690

5 files changed

Lines changed: 182 additions & 57 deletions

File tree

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

Lines changed: 37 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,32 @@ 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 {
281285
return ctrl.Result{}, fmt.Errorf("error stopping informers: %v", err)
282286
}
287+
if err := c.removeFinalizer(ctx, cer, clusterExtensionRevisionTeardownFinalizer); err != nil {
288+
return ctrl.Result{}, fmt.Errorf("error removing teardown finalizer: %v", err)
289+
}
290+
return ctrl.Result{}, nil
291+
}
283292

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

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

Lines changed: 74 additions & 36 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,15 +670,15 @@ 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:Archived:Unknown and Progressing:False:Archived conditions when a revision is archived",
673674
revisionResult: mockRevisionResult{},
674675
existingObjs: func() []client.Object {
675676
ext := newTestClusterExtension()
676677
rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName, ext, testScheme)
677678
rev1.Finalizers = []string{
678679
"olm.operatorframework.io/teardown",
679680
}
680-
rev1.DeletionTimestamp = &metav1.Time{Time: time.Now()}
681+
rev1.Spec.LifecycleState = ocv1.ClusterExtensionRevisionLifecycleStateArchived
681682
return []client.Object{rev1, ext}
682683
},
683684
revisionEngineTeardownFn: func(t *testing.T) func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error) {
@@ -688,55 +689,64 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) {
688689
}
689690
},
690691
validate: func(t *testing.T, c client.Client) {
691-
t.Log("cluster revision is deleted")
692692
rev := &ocv1.ClusterExtensionRevision{}
693693
err := c.Get(t.Context(), client.ObjectKey{
694694
Name: clusterExtensionRevisionName,
695695
}, rev)
696-
require.Error(t, err)
697-
require.True(t, apierrors.IsNotFound(err))
696+
require.NoError(t, err)
697+
cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeAvailable)
698+
require.NotNil(t, cond)
699+
require.Equal(t, metav1.ConditionUnknown, cond.Status)
700+
require.Equal(t, ocv1.ClusterExtensionRevisionReasonArchived, cond.Reason)
701+
require.Equal(t, "revision is archived", cond.Message)
702+
require.Equal(t, int64(1), cond.ObservedGeneration)
703+
704+
cond = meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeProgressing)
705+
require.NotNil(t, cond)
706+
require.Equal(t, metav1.ConditionFalse, cond.Status)
707+
require.Equal(t, ocv1.ClusterExtensionRevisionReasonArchived, cond.Reason)
708+
require.Equal(t, "revision is archived", cond.Message)
709+
require.Equal(t, int64(1), cond.ObservedGeneration)
698710
},
699711
},
700712
{
701-
name: "set Available:Unknown:Reconciling and surface tracking cache cleanup errors when deleted",
713+
name: "set Progressing:True:Retrying and requeue when archived revision archival is incomplete",
702714
revisionResult: mockRevisionResult{},
703715
existingObjs: func() []client.Object {
704716
ext := newTestClusterExtension()
705717
rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName, ext, testScheme)
706718
rev1.Finalizers = []string{
707719
"olm.operatorframework.io/teardown",
708720
}
709-
rev1.DeletionTimestamp = &metav1.Time{Time: time.Now()}
721+
rev1.Spec.LifecycleState = ocv1.ClusterExtensionRevisionLifecycleStateArchived
710722
return []client.Object{rev1, ext}
711723
},
712724
revisionEngineTeardownFn: func(t *testing.T) func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error) {
713725
return func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error) {
714726
return &mockRevisionTeardownResult{
715-
isComplete: true,
727+
isComplete: false,
716728
}, nil
717729
}
718730
},
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",
731+
expectedResult: ctrl.Result{RequeueAfter: 5 * time.Second},
723732
validate: func(t *testing.T, c client.Client) {
724-
t.Log("cluster revision is not deleted and still contains finalizer")
725733
rev := &ocv1.ClusterExtensionRevision{}
726734
err := c.Get(t.Context(), client.ObjectKey{
727735
Name: clusterExtensionRevisionName,
728736
}, rev)
729737
require.NoError(t, err)
730-
cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeAvailable)
738+
cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeProgressing)
731739
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)
740+
require.Equal(t, metav1.ConditionTrue, cond.Status)
741+
require.Equal(t, ocv1.ClusterExtensionRevisionReasonRetrying, cond.Reason)
742+
require.Equal(t, "removing revision resources that are not owned by another revision", cond.Message)
743+
744+
// Finalizer should still be present
745+
require.Contains(t, rev.Finalizers, "olm.operatorframework.io/teardown")
736746
},
737747
},
738748
{
739-
name: "set Available:Archived:Unknown and Progressing:False:Archived conditions when a revision is archived",
749+
name: "return error and set retrying conditions when archived revision teardown fails",
740750
revisionResult: mockRevisionResult{},
741751
existingObjs: func() []client.Object {
742752
ext := newTestClusterExtension()
@@ -749,30 +759,57 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) {
749759
},
750760
revisionEngineTeardownFn: func(t *testing.T) func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error) {
751761
return func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error) {
752-
return &mockRevisionTeardownResult{
753-
isComplete: true,
754-
}, nil
762+
return nil, fmt.Errorf("teardown failed: connection refused")
755763
}
756764
},
765+
expectedErr: "error archiving revision",
757766
validate: func(t *testing.T, c client.Client) {
758767
rev := &ocv1.ClusterExtensionRevision{}
759768
err := c.Get(t.Context(), client.ObjectKey{
760769
Name: clusterExtensionRevisionName,
761770
}, rev)
762771
require.NoError(t, err)
763-
cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeAvailable)
772+
cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeProgressing)
764773
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)
774+
require.Equal(t, metav1.ConditionTrue, cond.Status)
775+
require.Equal(t, ocv1.ClusterExtensionRevisionReasonRetrying, cond.Reason)
776+
require.Contains(t, cond.Message, "teardown failed: connection refused")
769777

770-
cond = meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeProgressing)
778+
// Finalizer should still be present
779+
require.Contains(t, rev.Finalizers, "olm.operatorframework.io/teardown")
780+
},
781+
},
782+
{
783+
name: "return error and set retrying conditions when factory fails to create engine during archived teardown",
784+
revisionResult: mockRevisionResult{},
785+
existingObjs: func() []client.Object {
786+
ext := newTestClusterExtension()
787+
rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName, ext, testScheme)
788+
rev1.Finalizers = []string{
789+
"olm.operatorframework.io/teardown",
790+
}
791+
rev1.Spec.LifecycleState = ocv1.ClusterExtensionRevisionLifecycleStateArchived
792+
return []client.Object{rev1, ext}
793+
},
794+
revisionEngineTeardownFn: func(t *testing.T) func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error) {
795+
return nil
796+
},
797+
revisionEngineFactoryErr: fmt.Errorf("token getter failed"),
798+
expectedErr: "failed to create revision engine",
799+
validate: func(t *testing.T, c client.Client) {
800+
rev := &ocv1.ClusterExtensionRevision{}
801+
err := c.Get(t.Context(), client.ObjectKey{
802+
Name: clusterExtensionRevisionName,
803+
}, rev)
804+
require.NoError(t, err)
805+
cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeProgressing)
771806
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)
807+
require.Equal(t, metav1.ConditionTrue, cond.Status)
808+
require.Equal(t, ocv1.ClusterExtensionRevisionReasonRetrying, cond.Reason)
809+
require.Contains(t, cond.Message, "token getter failed")
810+
811+
// Finalizer should still be present
812+
require.Contains(t, rev.Finalizers, "olm.operatorframework.io/teardown")
776813
},
777814
},
778815
{
@@ -833,9 +870,10 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) {
833870
},
834871
teardown: tc.revisionEngineTeardownFn(t),
835872
}
873+
factory := &mockRevisionEngineFactory{engine: mockEngine, createErr: tc.revisionEngineFactoryErr}
836874
result, err := (&controllers.ClusterExtensionRevisionReconciler{
837875
Client: testClient,
838-
RevisionEngineFactory: &mockRevisionEngineFactory{engine: mockEngine},
876+
RevisionEngineFactory: factory,
839877
TrackingCache: &mockTrackingCache{
840878
client: testClient,
841879
freeFn: tc.trackingCacheFreeFn,
@@ -847,7 +885,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) {
847885
})
848886

849887
// reconcile cluster extension revision
850-
require.Equal(t, ctrl.Result{}, result)
888+
require.Equal(t, tc.expectedResult, result)
851889
if tc.expectedErr != "" {
852890
require.Contains(t, err.Error(), tc.expectedErr)
853891
} 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)