🐛 Add teardown on revision archival#2502
🐛 Add teardown on revision archival#2502perdasilva wants to merge 1 commit intooperator-framework:mainfrom
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
perdasilva
force-pushed
the
fix-cer-archival-teardown
branch
from
01760b0 to
06afd4a
Compare
There 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.
internal/operator-controller/controllers/clusterextensionrevision_controller_test.go
Show resolved
Hide resolved
internal/operator-controller/controllers/clusterextensionrevision_controller.go
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2502 +/- ##
==========================================
- Coverage 73.23% 69.49% -3.75%
==========================================
Files 102 102
Lines 8504 8516 +12
==========================================
- Hits 6228 5918 -310
- Misses 1801 2129 +328
+ Partials 475 469 -6
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:
|
perdasilva
force-pushed
the
fix-cer-archival-teardown
branch
from
06afd4a to
5ca3f6e
Compare
perdasilva
changed the title
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: - Move RevisionEngine creation before the teardown check so it is available for both teardown and reconcile paths - Pass RevisionEngine and boxcutter Revision into teardown() - Call revisionEngine.Teardown() for archived revisions and handle incomplete teardown (requeue after 5s) and errors (return error for controller retry) - Remove redundant lifecycle state check and fix swallowed teardown error - Add unit tests for incomplete teardown requeue and teardown error propagation (teardown coverage: 68.8% -> 93.8%) - Add dummy-configmap to v1.0.0 test bundle (absent in v1.2.0) and assert it is removed in the "Each update creates a new revision" e2e test Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
perdasilva
force-pushed
the
fix-cer-archival-teardown
branch
from
5ca3f6e to
6e1e134
Compare
There 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.
| revision, opts, err := c.toBoxcutterRevision(ctx, rev) | ||
| if err != nil { | ||
| setRetryingConditions(rev, err.Error()) | ||
| return ctrl.Result{}, fmt.Errorf("converting to boxcutter revision: %v", err) | ||
| } | ||
|
|
||
| // | ||
| // Teardown | ||
| // | ||
| if !rev.DeletionTimestamp.IsZero() || rev.Spec.LifecycleState == ocv1.ClusterExtensionRevisionLifecycleStateArchived { | ||
| return c.teardown(ctx, rev) | ||
| return c.teardown(ctx, revision, rev) | ||
| } |
There was a problem hiding this comment.
toBoxcutterRevision() is now executed before the teardown branch. For delete-only reconciles (DeletionTimestamp set but LifecycleState not Archived), teardown() doesn't use the boxcutter revision, yet a failure in listPreviousRevisions() (via TrackingCache.List) will now prevent finalizer removal and can block deletion. Consider moving the teardown check above toBoxcutterRevision(), or only building the boxcutter revision when reconciling or when doing archived teardown.
| revisionEngine, err := c.RevisionEngineFactory.CreateRevisionEngine(ctx, cer) | ||
| if err != nil { | ||
| setRetryingConditions(cer, fmt.Sprintf("error archiving: %v", err)) | ||
| return ctrl.Result{}, fmt.Errorf("failed to create revision engine: %v", err) | ||
| } | ||
| tdres, err := revisionEngine.Teardown(ctx, *revision) | ||
| if err != nil { | ||
| setRetryingConditions(cer, fmt.Sprintf("error archiving: %v", err)) | ||
| return ctrl.Result{}, fmt.Errorf("error tearing down revision: %v", err) | ||
| } | ||
| if tdres != nil && !tdres.IsComplete() { | ||
| setRetryingConditions(cer, "tearing down revision") | ||
| return ctrl.Result{RequeueAfter: 5 * time.Second}, nil | ||
| } | ||
| // Ensure conditions are set before removing the finalizer when archiving | ||
| if markAsArchived(cer) { | ||
| return ctrl.Result{}, nil | ||
| } |
There was a problem hiding this comment.
In teardown(), archived revisions will call revisionEngine.Teardown() even when the revision has already been marked Archived. Because markAsArchived() intentionally returns early when it updates conditions, a newly-archived revision will typically reconcile twice, causing Teardown() to run twice before the finalizer is removed. Consider skipping Teardown() when the Archived conditions are already present (and the only remaining step is finalizer removal) to avoid redundant/possibly non-idempotent teardown work.
| revisionEngine, err := c.RevisionEngineFactory.CreateRevisionEngine(ctx, cer) | |
| if err != nil { | |
| setRetryingConditions(cer, fmt.Sprintf("error archiving: %v", err)) | |
| return ctrl.Result{}, fmt.Errorf("failed to create revision engine: %v", err) | |
| } | |
| tdres, err := revisionEngine.Teardown(ctx, *revision) | |
| if err != nil { | |
| setRetryingConditions(cer, fmt.Sprintf("error archiving: %v", err)) | |
| return ctrl.Result{}, fmt.Errorf("error tearing down revision: %v", err) | |
| } | |
| if tdres != nil && !tdres.IsComplete() { | |
| setRetryingConditions(cer, "tearing down revision") | |
| return ctrl.Result{RequeueAfter: 5 * time.Second}, nil | |
| } | |
| // Ensure conditions are set before removing the finalizer when archiving | |
| if markAsArchived(cer) { | |
| return ctrl.Result{}, nil | |
| } | |
| // Only perform teardown if the Archived conditions are not yet present. | |
| // We use a deep copy to probe markAsArchived without mutating the original object. | |
| cerCopy := cer.DeepCopy() | |
| if markAsArchived(cerCopy) { | |
| revisionEngine, err := c.RevisionEngineFactory.CreateRevisionEngine(ctx, cer) | |
| if err != nil { | |
| setRetryingConditions(cer, fmt.Sprintf("error archiving: %v", err)) | |
| return ctrl.Result{}, fmt.Errorf("failed to create revision engine: %v", err) | |
| } | |
| tdres, err := revisionEngine.Teardown(ctx, *revision) | |
| if err != nil { | |
| setRetryingConditions(cer, fmt.Sprintf("error archiving: %v", err)) | |
| return ctrl.Result{}, fmt.Errorf("error tearing down revision: %v", err) | |
| } | |
| if tdres != nil && !tdres.IsComplete() { | |
| setRetryingConditions(cer, "tearing down revision") | |
| return ctrl.Result{RequeueAfter: 5 * time.Second}, nil | |
| } | |
| // Ensure conditions are set before removing the finalizer when archiving | |
| if markAsArchived(cer) { | |
| return ctrl.Result{}, nil | |
| } | |
| } |
1 participant
Description
revisionEngine.Teardown()when aClusterExtensionRevisionenters theArchivedlifecycle state, ensuring managed resources removed between bundle versions are cleaned up from the clusterRevisionEnginecreation before the teardown/reconcile branch so it is available for both pathsdummy-configmapto the v1.0.0 test bundle (absent in v1.2.0) and assert its removal in the "Each update creates a new revision" e2e testTest plan
Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion)teardown()coverage improved from 68.8% to 93.8%dummy-configmapis removed after archival🤖 Generated with Claude Code
Reviewer Checklist