(chore): Use label-based cache for revision lookups instead of explicit 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

Author: camilamacedo86

cmd/operator-controller/main.go

@@ -588,6 +588,7 @@ func setupBoxcutter(
 		Client:             mgr.GetClient(),
 		ActionClientGetter: acg,
 		RevisionGenerator:  rg,
+		Scheme:             mgr.GetScheme(),
 	}
 
 	baseDiscoveryClient, err := discovery.NewDiscoveryClientForConfig(mgr.GetConfig())

internal/operator-controller/applier/boxcutter.go

@@ -153,17 +153,37 @@ func (r *SimpleRevisionGenerator) buildClusterExtensionRevision(
 	}
 }
 
+// BoxcutterStorageMigrator migrates ClusterExtensions from Helm-based storage to Boxcutter's
+// ClusterExtensionRevision storage.
+//
+// Scenario: Given a ClusterExtension, determine if migration from Helm storage is needed
+//   - When ClusterExtensionRevisions already exist, no migration needed (return early)
+//   - When a Helm release exists but no revisions, create ClusterExtensionRevision from Helm release
+//   - When no Helm release exists and no revisions, this is a fresh install (no migration needed)
+//
+// The migrated revision includes:
+//   - Owner label (olm.operatorframework.io/owner) for label-based lookups
+//   - OwnerReference to parent ClusterExtension for proper garbage collection
+//   - All objects from the Helm release manifest
+//
+// This ensures migrated revisions have the same structure as revisions created by normal
+// Boxcutter flow, enabling successful upgrades after migration.
 type BoxcutterStorageMigrator struct {
 	ActionClientGetter helmclient.ActionClientGetter
 	RevisionGenerator  ClusterExtensionRevisionGenerator
 	Client             boxcutterStorageMigratorClient
+	Scheme             *runtime.Scheme // Required for setting ownerReferences on migrated revisions
 }
 
 type boxcutterStorageMigratorClient interface {
 	List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error
 	Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error
 }
 
+// Migrate performs a one-time migration from Helm storage to ClusterExtensionRevision storage.
+//
+// This enables upgrades from older operator-controller versions that used Helm for state management.
+// The migration is idempotent - it checks for existing revisions before attempting to migrate.
 func (m *BoxcutterStorageMigrator) Migrate(ctx context.Context, ext *ocv1.ClusterExtension, objectLabels map[string]string) error {
 	existingRevisionList := ocv1.ClusterExtensionRevisionList{}
 	if err := m.Client.List(ctx, &existingRevisionList, client.MatchingLabels{
@@ -172,29 +192,37 @@ func (m *BoxcutterStorageMigrator) Migrate(ctx context.Context, ext *ocv1.Cluste
 		return fmt.Errorf("listing ClusterExtensionRevisions before attempting migration: %w", err)
 	}
 	if len(existingRevisionList.Items) != 0 {
-		// No migration needed.
+		// No migration needed - already using ClusterExtensionRevision storage.
 		return nil
 	}
 
+	// Try to find a Helm release to migrate from
 	ac, err := m.ActionClientGetter.ActionClientFor(ctx, ext)
 	if err != nil {
 		return err
 	}
 
 	helmRelease, err := ac.Get(ext.GetName())
 	if errors.Is(err, driver.ErrReleaseNotFound) {
-		// no Helm Release -> no prior installation.
+		// No Helm release exists - this is a fresh installation, no migration needed.
 		return nil
 	}
 	if err != nil {
 		return err
 	}
 
+	// Create a ClusterExtensionRevision from the Helm release
 	rev, err := m.RevisionGenerator.GenerateRevisionFromHelmRelease(helmRelease, ext, objectLabels)
 	if err != nil {
 		return err
 	}
 
+	// Set ownerReference to ensure proper lifecycle management and garbage collection.
+	// This makes the migrated revision consistent with revisions created by normal Boxcutter flow.
+	if err := controllerutil.SetControllerReference(ext, rev, m.Scheme); err != nil {
+		return fmt.Errorf("set ownerref: %w", err)
+	}
+
 	if err := m.Client.Create(ctx, rev); err != nil {
 		return err
 	}

internal/operator-controller/applier/boxcutter_test.go

@@ -808,13 +808,17 @@ func TestBoxcutter_Apply(t *testing.T) {
 
 func TestBoxcutterStorageMigrator(t *testing.T) {
 	t.Run("creates revision", func(t *testing.T) {
+		testScheme := runtime.NewScheme()
+		require.NoError(t, ocv1.AddToScheme(testScheme))
+
 		brb := &mockBundleRevisionBuilder{}
 		mag := &mockActionGetter{}
 		client := &clientMock{}
 		sm := &applier.BoxcutterStorageMigrator{
 			RevisionGenerator:  brb,
 			ActionClientGetter: mag,
 			Client:             client,
+			Scheme:             testScheme,
 		}
 
 		ext := &ocv1.ClusterExtension{
@@ -836,13 +840,17 @@ func TestBoxcutterStorageMigrator(t *testing.T) {
 	})
 
 	t.Run("does not create revision when revisions exist", func(t *testing.T) {
+		testScheme := runtime.NewScheme()
+		require.NoError(t, ocv1.AddToScheme(testScheme))
+
 		brb := &mockBundleRevisionBuilder{}
 		mag := &mockActionGetter{}
 		client := &clientMock{}
 		sm := &applier.BoxcutterStorageMigrator{
 			RevisionGenerator:  brb,
 			ActionClientGetter: mag,
 			Client:             client,
+			Scheme:             testScheme,
 		}
 
 		ext := &ocv1.ClusterExtension{
@@ -866,6 +874,9 @@ func TestBoxcutterStorageMigrator(t *testing.T) {
 	})
 
 	t.Run("does not create revision when no helm release", func(t *testing.T) {
+		testScheme := runtime.NewScheme()
+		require.NoError(t, ocv1.AddToScheme(testScheme))
+
 		brb := &mockBundleRevisionBuilder{}
 		mag := &mockActionGetter{
 			getClientErr: driver.ErrReleaseNotFound,
@@ -875,6 +886,7 @@ func TestBoxcutterStorageMigrator(t *testing.T) {
 			RevisionGenerator:  brb,
 			ActionClientGetter: mag,
 			Client:             client,
+			Scheme:             testScheme,
 		}
 
 		ext := &ocv1.ClusterExtension{
@@ -905,7 +917,15 @@ func (m *mockBundleRevisionBuilder) GenerateRevisionFromHelmRelease(
 	helmRelease *release.Release, ext *ocv1.ClusterExtension,
 	objectLabels map[string]string,
 ) (*ocv1.ClusterExtensionRevision, error) {
-	return nil, nil
+	return &ocv1.ClusterExtensionRevision{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "test-revision",
+			Labels: map[string]string{
+				controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
+			},
+		},
+		Spec: ocv1.ClusterExtensionRevisionSpec{},
+	}, nil
 }
 
 type clientMock struct {

internal/operator-controller/controllers/clusterextensionrevision_controller.go

@@ -16,7 +16,6 @@ import (
 	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -116,7 +115,17 @@ func checkForUnexpectedClusterExtensionRevisionFieldChange(a, b ocv1.ClusterExte
 func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev *ocv1.ClusterExtensionRevision) (ctrl.Result, error) {
 	l := log.FromContext(ctx)
 
-	revision, opts, previous := toBoxcutterRevision(rev)
+	revision, opts, err := c.toBoxcutterRevision(ctx, rev)
+	if err != nil {
+		meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
+			Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
+			Status:             metav1.ConditionFalse,
+			Reason:             ocv1.ClusterExtensionRevisionReasonReconcileFailure,
+			Message:            err.Error(),
+			ObservedGeneration: rev.Generation,
+		})
+		return ctrl.Result{}, fmt.Errorf("converting to boxcutter revision: %v", err)
+	}
 
 	if !rev.DeletionTimestamp.IsZero() || rev.Spec.LifecycleState == ocv1.ClusterExtensionRevisionLifecycleStateArchived {
 		return c.teardown(ctx, rev, revision)
@@ -212,7 +221,11 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
 
 	//nolint:nestif
 	if rres.IsComplete() {
-		// Archive other revisions.
+		// Archive previous revisions
+		previous, err := c.ListPreviousRevisions(ctx, rev)
+		if err != nil {
+			return ctrl.Result{}, fmt.Errorf("listing previous revisions: %v", err)
+		}
 		for _, a := range previous {
 			patch := []byte(`{"spec":{"lifecycleState":"Archived"}}`)
 			if err := c.Client.Patch(ctx, a, client.RawPatch(types.MergePatchType, patch)); err != nil {
@@ -428,14 +441,56 @@ func (c *ClusterExtensionRevisionReconciler) removeFinalizer(ctx context.Context
 	return nil
 }
 
-func toBoxcutterRevision(rev *ocv1.ClusterExtensionRevision) (*boxcutter.Revision, []boxcutter.RevisionReconcileOption, []client.Object) {
-	previous := make([]client.Object, 0, len(rev.Spec.Previous))
-	for _, specPrevious := range rev.Spec.Previous {
-		prev := &unstructured.Unstructured{}
-		prev.SetName(specPrevious.Name)
-		prev.SetUID(specPrevious.UID)
-		prev.SetGroupVersionKind(ocv1.GroupVersion.WithKind(ocv1.ClusterExtensionRevisionKind))
-		previous = append(previous, prev)
+// ListPreviousRevisions returns active revisions belonging to the same ClusterExtension.
+//
+// Scenario: Given a ClusterExtensionRevision, find all related active revisions
+//   - When the revision has no owner label, return empty list (revision not properly labeled)
+//   - When looking up by owner label, filter out the current revision itself
+//   - When a revision is archived or being deleted, exclude it from results
+//   - Otherwise include active and paused revisions with matching owner
+//
+// This method uses label-based lookups instead of the revision's Spec.Previous field to find
+// related revisions. This approach leverages the cache and is more efficient than tracking
+// explicit revision chains.
+//
+// Used by boxcutter for:
+//   - Collision detection during rollout
+//   - Finding revisions to archive after successful deployment
+func (c *ClusterExtensionRevisionReconciler) ListPreviousRevisions(ctx context.Context, rev *ocv1.ClusterExtensionRevision) ([]client.Object, error) {
+	ownerLabel, ok := rev.Labels[ClusterExtensionRevisionOwnerLabel]
+	if !ok {
+		// No owner label means this revision isn't properly labeled - return empty list
+		return nil, nil
+	}
+
+	revList := &ocv1.ClusterExtensionRevisionList{}
+	if err := c.Client.List(ctx, revList, client.MatchingLabels{
+		ClusterExtensionRevisionOwnerLabel: ownerLabel,
+	}); err != nil {
+		return nil, fmt.Errorf("listing revisions: %w", err)
+	}
+
+	previous := make([]client.Object, 0, len(revList.Items))
+	for i := range revList.Items {
+		r := &revList.Items[i]
+		if r.Name == rev.Name {
+			continue
+		}
+		// Skip archived or deleting revisions
+		if r.Spec.LifecycleState == ocv1.ClusterExtensionRevisionLifecycleStateArchived ||
+			!r.DeletionTimestamp.IsZero() {
+			continue
+		}
+		previous = append(previous, r)
+	}
+
+	return previous, nil
+}
+
+func (c *ClusterExtensionRevisionReconciler) toBoxcutterRevision(ctx context.Context, rev *ocv1.ClusterExtensionRevision) (*boxcutter.Revision, []boxcutter.RevisionReconcileOption, error) {
+	previous, err := c.ListPreviousRevisions(ctx, rev)
+	if err != nil {
+		return nil, nil, fmt.Errorf("listing previous revisions: %w", err)
 	}
 
 	opts := []boxcutter.RevisionReconcileOption{
@@ -476,7 +531,7 @@ func toBoxcutterRevision(rev *ocv1.ClusterExtensionRevision) (*boxcutter.Revisio
 	if rev.Spec.LifecycleState == ocv1.ClusterExtensionRevisionLifecycleStatePaused {
 		opts = append(opts, boxcutter.WithPaused{})
 	}
-	return r, opts, previous
+	return r, opts, nil
 }
 
 var (