⚠️ (Boxcutter Runtime) Revision discovery now uses label-based cache lookups, removing Spec.Previous and preventing orphaned revisions on delete. #2315
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors how previous revisions are discovered during ClusterExtensionRevision reconciliation. Instead of relying on a static Spec.Previous field, the controller now dynamically queries for previous revisions using the owner label, enabling more flexible revision management.
Key changes:
- Introduced
ListPreviousRevisions()method to dynamically discover sibling revisions - Updated
toBoxcutterRevision()to callListPreviousRevisions()and return errors - Enhanced test coverage with comprehensive test cases for the new listing logic
- Improved test names and added scenario comments for better readability
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/operator-controller/controllers/clusterextensionrevision_controller.go | Added ListPreviousRevisions() method for dynamic revision discovery, refactored toBoxcutterRevision() signature to return errors, removed unused unstructured import |
| internal/operator-controller/controllers/clusterextensionrevision_controller_test.go | Added comprehensive tests for ListPreviousRevisions(), updated test names to be more descriptive, added scenario comments, updated mockTrackingCache to delegate Get/List to actual client, removed obsolete Spec.Previous test setup, added owner label to test revision helper |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
camilamacedo86
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2315 +/- ##
==========================================
- Coverage 74.32% 74.18% -0.15%
==========================================
Files 92 92
Lines 7226 7256 +30
==========================================
+ Hits 5371 5383 +12
- Misses 1428 1437 +9
- Partials 427 436 +9
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:
|
camilamacedo86
force-pushed
the
boxcutter-use-labels-cache
branch
2 times, most recently
from
9c8f9af to
6026ae8
Compare
camilamacedo86
changed the title
camilamacedo86
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
camilamacedo86
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
camilamacedo86
force-pushed
the
boxcutter-use-labels-cache
branch
4 times, most recently
from
c067348 to
6076304
Compare
camilamacedo86
force-pushed
the
boxcutter-use-labels-cache
branch
from
6076304 to
5a89bed
Compare
camilamacedo86
force-pushed
the
boxcutter-use-labels-cache
branch
from
5a89bed to
fbf27d6
Compare
camilamacedo86
force-pushed
the
boxcutter-use-labels-cache
branch
from
fbf27d6 to
c7ea251
Compare
camilamacedo86
force-pushed
the
boxcutter-use-labels-cache
branch
from
c7ea251 to
ef2c006
Compare
| } | ||
| // Only archive revisions with lower revision numbers (actual previous revisions) | ||
| if r.Spec.Revision >= rev.Spec.Revision { | ||
| continue |
There was a problem hiding this comment.
We still populate Spec.Previous even though we now use labels for discovery because:
-
- API Compatibility - It's a published field in the CRD and marked immutable. Removing it would be a breaking change.
-
- Garbage Collection -
setPreviousRevisions()uses it to enforce the 5 revision limit and delete older archived revisions.
- Garbage Collection -
-
- Audit Trail - Provides a historical record of the revision chain, useful for debugging and understanding upgrade history.
There was a problem hiding this comment.
I'm not sure about this one. I think we can violate i since we're still in experimental mode and breaking changes are allowed, we should probably update ii to use the List function anyway, and lastly, revisions are numbered, so the chain should be self-evident, no?
There was a problem hiding this comment.
I think you are 100% right and we can keep things simple here by removing it.
|
Hi @pedjak, We are changing the public API: If this had already been promoted to GA, we wouldn’t be able to do that. It currently fails the go-apidiff check, which confirms it’s a user-facing change. Anything that affects what users see or experience is considered a user-facing change. In contrast, changes that only modify how we do things internally—without altering how information is presented, stored, or accessed on the cluster—are not user-facing. So it should actually have a Additionally, this change resolves a user-facing issue by ensuring that orphaned revisions are no longer left behind on the cluster. Thank you 🙏 |
pedjak
left a comment
pedjak
left a comment
There was a problem hiding this comment.
/lgtm a few questions left author to decide if they should be addressed in this PR or follow-ups.
| } | ||
|
|
||
| revList := &ocv1.ClusterExtensionRevisionList{} | ||
| if err := c.TrackingCache.List(ctx, revList, client.MatchingLabels{ |
There was a problem hiding this comment.
what is the advantage of using c.TrackingCache.List() vs. c.Client.List()?
There was a problem hiding this comment.
Good question. TrackingCache.List() comes from Boxcutter’s managedcache (https://pkg.go.dev/pkg.package-operator.run/[email protected]/managedcache
). It tracks object-to-revision relationships, which the standard c.Client doesn’t. This is mostly experimental — we could use the default controller-runtime client instead.
Since this is in tech preview, I’m was looking for us to use it for now, but we should keep an eye on the usual trade-offs: double caching, some memory overhead, possible some maintenance complexity. If the tracking features don’t end up paying off, we can switch to c.Client.List() later.
My idea was try out it now, and see if we found bugs, allow we epxlore more in follow-ups and add some E2E coverage around it. Let me know if this makes sense.
@thetechnick have you any reason for we do not use it?
| previous = append(previous, prev) | ||
| // listPreviousRevisions returns active revisions belonging to the same ClusterExtension with lower revision numbers. | ||
| // Filters out the current revision, archived revisions, deleting revisions, and revisions with equal or higher numbers. | ||
| func (c *ClusterExtensionRevisionReconciler) listPreviousRevisions(ctx context.Context, rev *ocv1.ClusterExtensionRevision) ([]client.Object, error) { |
There was a problem hiding this comment.
any reason why we do not return []*ocv1.ClusterExtensionRevision?
There was a problem hiding this comment.
Changed ! I think is better your proposal
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pedjak, perdasilva The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
camilamacedo86
force-pushed
the
boxcutter-use-labels-cache
branch
from
5161d30 to
3d11630
Compare
camilamacedo86
force-pushed
the
boxcutter-use-labels-cache
branch
from
3d11630 to
b0f47d1
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…it chains Instead of tracking revision history through Spec.Previous fields, we now find related revisions using labels and the controller-runtime cache. This is more efficient and works better with controller-runtime's caching layer. To support this change, the Helm-to-Boxcutter migration now sets ownerReferences on migrated revisions, ensuring they work identically to newly created revisions. Assisted-by: Cursor
camilamacedo86
force-pushed
the
boxcutter-use-labels-cache
branch
from
b0f47d1 to
056baee
Compare
|
/hold Doing some tests locally |
openshift-ci
bot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/override go-apidiff/go-apidiff |
|
@tmshort: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override go-apidiff |
|
@tmshort: Overrode contexts on behalf of tmshort: go-apidiff In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override go-apidiff |
|
@tmshort: Overrode contexts on behalf of tmshort: go-apidiff In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-merge-bot
bot
merged commit 472e8a2
into
operator-framework:main
4 participants
Revisions are now discovered using label-based cache lookups instead of explicit
Spec.Previouschains.This makes lookups faster and ensures proper cleanup when a ClusterExtension is deleted.
What Changed
API (Breaking - OK in experimental mode):
Spec.Previousfield from ClusterExtensionRevisionClusterExtensionRevisionPrevioustypeDiscovery:
olm.operatorframework.io/ownervia TrackingCacheMigration:
ownerReferencesso revisions are garbage collected with their ClusterExtensionAvailable=Unknown, lets revision controller verify cluster stateGarbage Collection:
garbageCollectOldRevisions,ClusterExtensionRevisionRetentionLimit)Why It's Better
Performance:
Reliability:
Simplicity:
Migration Behavior
When upgrading from Helm storage:
Available=UnknownstatusAvailable=TrueandSucceeded=TrueDemo