Uh oh!
There was an error while loading. Please reload this page.
🐛 Add teardown on revision archival - #2502
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
01760b0 to
06afd4aCompareThere was a problem hiding this comment.
Pull request overview
Updates ClusterExtensionRevision teardown behavior to actively tear down Boxcutter-managed resources when a revision is archived, and extends test coverage/data to validate resource removal across bundle versions.
Changes:
- Invoke
RevisionEngine.Teardownduring teardown for archived revisions, with retry + requeue when teardown is incomplete. - Expand deletion/archival reconciliation tests to cover incomplete teardown and teardown failures (including expected requeue behavior).
- Add a dummy ConfigMap manifest to the v1.0.0 test bundle to validate removal of resources that disappear in later bundle versions.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
internal/operator-controller/controllers/clusterextensionrevision_controller.go | Calls Boxcutter teardown for archived revisions and requeues while teardown is incomplete. |
internal/operator-controller/controllers/clusterextensionrevision_controller_test.go | Adds test cases for archived teardown retry/error paths and asserts reconcile results. |
testdata/images/bundles/test-operator/v1.0.0/manifests/dummy.configmap.yaml | Introduces a version-specific manifest used to test removal of resources across upgrades. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #2502 +/- ##
==========================================
- Coverage 73.27% 73.18% -0.09%
==========================================
Files 103 103 Lines 8516 8533 +17 ==========================================
+ Hits 6240 6245 +5 - Misses 1801 1808 +7 - Partials 475 480 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
06afd4a to
5ca3f6eCompare5ca3f6e to
6e1e134CompareThere was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
6e1e134 to
7c6037bCompare7c6037b to
b51648bCompareThere was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pedjak
left a comment
There was a problem hiding this comment.
what if two revisions are sharing the same resource? how do ensure that we do not delete something accidentally?
| setRetryingConditions(rev, err.Error()) | ||
| return ctrl.Result{}, fmt.Errorf("converting to boxcutter revision: %v", err) | ||
| } | ||
There was a problem hiding this comment.
why do we need to move this further down?
There was a problem hiding this comment.
depends on whether we want to remove revision creation from the deletion path or not. Since we want the garbage collector to handle deletion, it could avoid transient (or maybe even persistent) errors from getting in the way. If so, it might also make sense to move it closer to where revision is first needed
| } | ||
| func (c *ClusterExtensionRevisionReconciler) archive(ctx context.Context, cer *ocv1.ClusterExtensionRevision) (machinery.RevisionTeardownResult, error) { | ||
| revision, _, err := c.toBoxcutterRevision(ctx, cer) |
There was a problem hiding this comment.
why do we convert this multiple times, the caller can pass it?
There was a problem hiding this comment.
it was a suggestion by copilot to remove revision creation from the deletion path
There was a problem hiding this comment.
I've restructured things to just create those structures once
| if err != nil { | ||
| return nil, fmt.Errorf("converting to boxcutter revision: %v", err) | ||
| } | ||
| revisionEngine, err := c.RevisionEngineFactory.CreateRevisionEngine(ctx, cer) |
There was a problem hiding this comment.
revision engine is also already available at the caller side?
There was a problem hiding this comment.
I think this is a similar argument as with creating the revision. It's not needed in the deletion path. Maybe we could just separate the deletion and archival paths further. Then we only repeat finalizer removal.
Actually, what do you think about removing the owner ref from archived revisions? They would still carry the owner labels we apply, so they are still attached. But, it gives a cleaner kubectl lineage output XD and might avoid CE reconciliations (although we don't really expect any changes to archived revisions...)
There was a problem hiding this comment.
The revision engine also has a dependency on the service account, which might be deleted together with the revision. So, it's probably safer to not have revision engine creation on the deletion path.
| And ClusterExtensionRevision "${NAME}-2" reports Available as True with Reason ProbesSucceeded | ||
| And ClusterExtensionRevision "${NAME}-1" is archived | ||
| # dummy-config map exists only in v1.0.0 - once the v1.0.0 revision is archived, it should be gone from the cluster | ||
| And resource "configmap/dummy-configmap" is eventually not found |
There was a problem hiding this comment.
should we check if other resources are there?
There was a problem hiding this comment.
should we move that check to a more general upgrade test?
There was a problem hiding this comment.
I mean, there is overlap between the resources. So, some of the v1.0.0 resources would still be present.
There was a problem hiding this comment.
I mean, there is overlap between the resources. So, some of the v1.0.0 resources would still be present.
right, should have steps that checks for that explicitly in this test?
There was a problem hiding this comment.
Claude and I updated things to check that the revision-1 resources either don't exist or are no longer in the object's ownerrefs
b51648b to
fdcc6d9Comparefdcc6d9 to
9105af6CompareThere was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev *ocv1.ClusterExtensionRevision) (ctrl.Result, error) { | ||
| func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, cer *ocv1.ClusterExtensionRevision) (ctrl.Result, error) { |
There was a problem hiding this comment.
is really needed in this PR to rename arg from rev to cer? It makes the diff bigger than it needs to be.
There was a problem hiding this comment.
I feel it reads better, especially when you have two parallel revision objects. It makes it easier to know that you're operating over the CER vs the Boxcutter revision. It might even be worth going forward and renaming revision to boxcutterRevision or bcRev, or something like that.
There was a problem hiding this comment.
If it would be helpful, I could revert it for this PR and do a follow up with just that change
There was a problem hiding this comment.
If it would be helpful, I could revert it for this PR and do a follow up with just that change
Yeah, I would keep the PR focused on the teardown, the renamings could be another focused PR.
Uh oh!
There was an error while loading. Please reload this page.
9105af6 to
b306567Compareb306567 to
d9f672aCompareea6fe54 to
57250dfCompareThere was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
internal/operator-controller/controllers/clusterextensionrevision_controller_test.go:658
- In the "teardown finalizer is removed" test case, the revision is neither deleting (
DeletionTimestampis unset) nor archived (Spec.LifecycleStatestays Active), but the validation expects the teardown finalizer to be removed. With the new reconciler flow, finalizer removal happens indelete()(deletion) orarchive()(archival), so this case will not exercise either path. Set aDeletionTimestamp(to test deletion) or mark the revision Archived (to test archival), or adjust the expectation accordingly.
name: "teardown finalizer is removed",
revisionResult: mockRevisionResult{},
existingObjs: func() []client.Object {
ext := newTestClusterExtension()
rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName, ext, testScheme)
rev1.Finalizers = []string{
"olm.operatorframework.io/teardown",
}
return []client.Object{ext, rev1}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
57250df to
2a93572Compare| } | ||
| func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev *ocv1.ClusterExtensionRevision) (ctrl.Result, error) { | ||
| func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, cer *ocv1.ClusterExtensionRevision) (ctrl.Result, error) { |
There was a problem hiding this comment.
If it would be helpful, I could revert it for this PR and do a follow up with just that change
Yeah, I would keep the PR focused on the teardown, the renamings could be another focused PR.
| setRetryingConditions(cer, fmt.Sprintf("error archiving: %v", err)) | ||
| return ctrl.Result{}, fmt.Errorf("error tearing down revision: %v", err) |
There was a problem hiding this comment.
what is the reason that pass fmt.Sprintf("error archiving: %v", err) to setRetryingConditions and then return back to the caller fmt.Errorf("error tearing down revision: %v", err). Can we have the same error?
| return ctrl.Result{}, fmt.Errorf("error tearing down revision: %v", err) | ||
| } | ||
| if tdres != nil && !tdres.IsComplete() { | ||
| setRetryingConditions(cer, "tearing down revision") |
There was a problem hiding this comment.
can we be here a bit more specific and share with user what is actually going on? tearing down revision can mean many things. Why do we requeue here?
There was a problem hiding this comment.
part of the archival process is clearing the tracking cache. If the tear down process hasn't finished completely yet, we should retry (but given that we have cleared the cache, deletions won't trigger reconciliation)
2a93572 to
b0af690CompareThere was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
internal/operator-controller/controllers/clusterextensionrevision_controller_test.go:659
- In the "teardown finalizer is removed" test case, the revision is neither marked for deletion (DeletionTimestamp) nor archived, so the reconciler will take the active reconcile path and will not remove the teardown finalizer. Either set a DeletionTimestamp on rev1 (to exercise delete()) or adjust the test name/expectations to match the active path.
name: "teardown finalizer is removed",
revisionResult: mockRevisionResult{},
existingObjs: func() []client.Object {
ext := newTestClusterExtension()
rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName, ext, testScheme)
rev1.Finalizers = []string{
"olm.operatorframework.io/teardown",
}
return []client.Object{ext, rev1}
},
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
b0af690 to
110d036CompareThere was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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>
110d036 to
88f1f38Compare
camilamacedo86
left a comment
There was a problem hiding this comment.
It seems great now 🎉
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
camilamacedo86
commented
Feb 23, 2026
/lgtm cancel To let other person just LGTM and we win the second review |
06da96a
into
operator-framework:mainUh oh!
There was an error while loading. Please reload this page.
Description
When a
ClusterExtensionRevisiontransitions to theArchivedlifecycle state, invoke the revision engine'sTeardownmethod 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.teardown()intodelete()(CER deletion) andarchive()(CER archival); onlyarchive()callsrevisionEngine.Teardown()RevisionEnginecreation and watch establishment before the archive check so they are available for both archive and reconcile pathsrevvariable tocerfor consistency with the type nameClusterExtensionRevision "<name>" 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 on the cluster or does not list the CER in its ownerReferencesconfigmap/dummy-configmapnot-found assertion in the update e2e scenario with the new generic phase-objects stepTest plan
Test_ClusterExtensionRevisionReconciler_Reconcile_ArchivalAndDeletion)🤖 Generated with Claude Code
Reviewer Checklist