(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

api/v1/clusterextensionrevision_types.go

@@ -19,7 +19,6 @@ package v1
 import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/types"
 )
 
 const (
@@ -40,6 +39,7 @@ const (
 	ClusterExtensionRevisionReasonIncomplete                = "Incomplete"
 	ClusterExtensionRevisionReasonProgressing               = "Progressing"
 	ClusterExtensionRevisionReasonArchived                  = "Archived"
+	ClusterExtensionRevisionReasonMigrated                  = "Migrated"
 )
 
 // ClusterExtensionRevisionSpec defines the desired state of ClusterExtensionRevision.
@@ -66,10 +66,6 @@ type ClusterExtensionRevisionSpec struct {
 	// +listMapKey=name
 	// +optional
 	Phases []ClusterExtensionRevisionPhase `json:"phases,omitempty"`
-	// Previous references previous revisions that objects can be adopted from.
-	//
-	// +kubebuilder:validation:XValidation:rule="self == oldSelf", message="previous is immutable"
-	Previous []ClusterExtensionRevisionPrevious `json:"previous,omitempty"`
 }
 
 // ClusterExtensionRevisionLifecycleState specifies the lifecycle state of the ClusterExtensionRevision.
@@ -130,13 +126,6 @@ const (
 	CollisionProtectionNone CollisionProtection = "None"
 )
 
-type ClusterExtensionRevisionPrevious struct {
-	// +kubebuilder:validation:Required
-	Name string `json:"name"`
-	// +kubebuilder:validation:Required
-	UID types.UID `json:"uid"`
-}
-
 // ClusterExtensionRevisionStatus defines the observed state of a ClusterExtensionRevision.
 type ClusterExtensionRevisionStatus struct {
 	// +listType=map

api/v1/zz_generated.deepcopy.go

@@ -586,6 +586,7 @@ func setupBoxcutter(
 	ceReconciler.RevisionStatesGetter = &controllers.BoxcutterRevisionStatesGetter{Reader: mgr.GetClient()}
 	ceReconciler.StorageMigrator = &applier.BoxcutterStorageMigrator{
 		Client:             mgr.GetClient(),
+		Scheme:             mgr.GetScheme(),
 		ActionClientGetter: acg,
 		RevisionGenerator:  rg,
 	}

cmd/operator-controller/main.go

@@ -111,27 +111,6 @@ spec:
                 x-kubernetes-validations:
                 - message: phases is immutable
                   rule: self == oldSelf || oldSelf.size() == 0
-              previous:
-                description: Previous references previous revisions that objects can
-                  be adopted from.
-                items:
-                  properties:
-                    name:
-                      type: string
-                    uid:
-                      description: |-
-                        UID is a type that holds unique ID values, including UUIDs.  Because we
-                        don't ONLY use UUIDs, this is an alias to string.  Being a type captures
-                        intent and helps make sure that UIDs and names do not get conflated.
-                      type: string
-                  required:
-                  - name
-                  - uid
-                  type: object
-                type: array
-                x-kubernetes-validations:
-                - message: previous is immutable
-                  rule: self == oldSelf
               revision:
                 description: |-
                   Revision is a sequence number representing a specific revision of the ClusterExtension instance.

helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml

@@ -32,7 +32,7 @@ import (
 )
 
 const (
-	ClusterExtensionRevisionPreviousLimit = 5
+	ClusterExtensionRevisionRetentionLimit = 5
 )
 
 type ClusterExtensionRevisionGenerator interface {
@@ -93,7 +93,7 @@ func (r *SimpleRevisionGenerator) GenerateRevisionFromHelmRelease(
 func (r *SimpleRevisionGenerator) GenerateRevision(
 	ctx context.Context,
 	bundleFS fs.FS, ext *ocv1.ClusterExtension,
-	objectLabels, revisionAnnotations map[string]string,
+	objectLabels, storageLabels map[string]string,
 ) (*ocv1.ClusterExtensionRevision, error) {
 	// extract plain manifests
 	plain, err := r.ManifestProvider.Get(bundleFS, ext)
@@ -133,11 +133,11 @@ func (r *SimpleRevisionGenerator) GenerateRevision(
 		})
 	}
 
-	if revisionAnnotations == nil {
-		revisionAnnotations = map[string]string{}
+	if storageLabels == nil {
+		storageLabels = map[string]string{}
 	}
 
-	return r.buildClusterExtensionRevision(objs, ext, revisionAnnotations), nil
+	return r.buildClusterExtensionRevision(objs, ext, storageLabels), nil
 }
 
 // sanitizedUnstructured takes an unstructured obj, removes status if present, and returns a sanitized copy containing only the allowed metadata entries set below.
@@ -185,32 +185,47 @@ func sanitizedUnstructured(ctx context.Context, unstr *unstructured.Unstructured
 func (r *SimpleRevisionGenerator) buildClusterExtensionRevision(
 	objects []ocv1.ClusterExtensionRevisionObject,
 	ext *ocv1.ClusterExtension,
-	annotations map[string]string,
+	storageLabels map[string]string,
 ) *ocv1.ClusterExtensionRevision {
+	// Merge storage labels with owner label
+	allLabels := make(map[string]string, len(storageLabels)+1)
+	allLabels[controllers.ClusterExtensionRevisionOwnerLabel] = ext.Name
+	for k, v := range storageLabels {
+		allLabels[k] = v
+	}
+
 	return &ocv1.ClusterExtensionRevision{
 		ObjectMeta: metav1.ObjectMeta{
-			Annotations: annotations,
-			Labels: map[string]string{
-				controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
-			},
+			Labels: allLabels,
 		},
 		Spec: ocv1.ClusterExtensionRevisionSpec{
-			Phases: PhaseSort(objects),
+			// Explicitly set LifecycleState to Active. While the CRD has a default,
+			// being explicit here ensures all code paths are clear and doesn't rely
+			// on API server defaulting behavior.
+			LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateActive,
+			Phases:         PhaseSort(objects),
 		},
 	}
 }
 
+// BoxcutterStorageMigrator migrates ClusterExtensions from Helm-based storage to
+// ClusterExtensionRevision storage, enabling upgrades from older operator-controller versions.
 type BoxcutterStorageMigrator struct {
 	ActionClientGetter helmclient.ActionClientGetter
 	RevisionGenerator  ClusterExtensionRevisionGenerator
 	Client             boxcutterStorageMigratorClient
+	Scheme             *runtime.Scheme
 }
 
 type boxcutterStorageMigratorClient interface {
 	List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error
+	Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error
 	Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error
+	Status() client.StatusWriter
 }
 
+// Migrate creates a ClusterExtensionRevision from an existing Helm release if no revisions exist yet.
+// The migration is idempotent and skipped if revisions already exist or no Helm release is found.
 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{
@@ -242,9 +257,35 @@ func (m *BoxcutterStorageMigrator) Migrate(ctx context.Context, ext *ocv1.Cluste
 		return err
 	}
 
+	// Set ownerReference for proper garbage collection when the ClusterExtension is deleted.
+	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
 	}
+
+	// Re-fetch the created revision to ensure we have the latest Generation from the API server.
+	fetchedRev := &ocv1.ClusterExtensionRevision{}
+	if err := m.Client.Get(ctx, client.ObjectKey{Name: rev.Name}, fetchedRev); err != nil {
+		return fmt.Errorf("fetching created revision: %w", err)
+	}
+
+	// Set Available=Unknown so the revision controller will verify cluster state through probes.
+	// During migration, ClusterExtension will briefly show as not installed until verification completes.
+	meta.SetStatusCondition(&fetchedRev.Status.Conditions, metav1.Condition{
+		Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
+		Status:             metav1.ConditionUnknown,
+		Reason:             ocv1.ClusterExtensionRevisionReasonMigrated,
+		Message:            "Migrated from Helm storage, awaiting cluster state verification",
+		ObservedGeneration: fetchedRev.Generation,
+	})
+
+	if err := m.Client.Status().Update(ctx, fetchedRev); err != nil {
+		return fmt.Errorf("updating migrated revision status: %w", err)
+	}
+
 	return nil
 }
 
@@ -256,8 +297,8 @@ type Boxcutter struct {
 	FieldOwner        string
 }
 
-func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (bool, string, error) {
-	return bc.apply(ctx, contentFS, ext, objectLabels, revisionAnnotations)
+func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, storageLabels map[string]string) (bool, string, error) {
+	return bc.apply(ctx, contentFS, ext, objectLabels, storageLabels)
 }
 
 func (bc *Boxcutter) getObjects(rev *ocv1.ClusterExtensionRevision) []client.Object {
@@ -281,9 +322,9 @@ func (bc *Boxcutter) createOrUpdate(ctx context.Context, obj client.Object) erro
 	return bc.Client.Patch(ctx, obj, client.Apply, client.FieldOwner(bc.FieldOwner), client.ForceOwnership)
 }
 
-func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (bool, string, error) {
+func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, storageLabels map[string]string) (bool, string, error) {
 	// Generate desired revision
-	desiredRevision, err := bc.RevisionGenerator.GenerateRevision(ctx, contentFS, ext, objectLabels, revisionAnnotations)
+	desiredRevision, err := bc.RevisionGenerator.GenerateRevision(ctx, contentFS, ext, objectLabels, storageLabels)
 	if err != nil {
 		return false, "", err
 	}
@@ -304,7 +345,6 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust
 	if len(existingRevisions) > 0 {
 		// try first to update the current revision.
 		currentRevision = &existingRevisions[len(existingRevisions)-1]
-		desiredRevision.Spec.Previous = currentRevision.Spec.Previous
 		desiredRevision.Spec.Revision = currentRevision.Spec.Revision
 		desiredRevision.Name = currentRevision.Name
 
@@ -317,6 +357,14 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust
 		case err == nil:
 			// inplace patch was successful, no changes in phases
 			state = StateUnchanged
+			// Re-fetch the revision to ensure we have the latest labels (storage metadata) and
+			// server-managed fields (resourceVersion, generation) after the in-place patch.
+			// This is important for subsequent logic that may depend on these updated values.
+			updated := &ocv1.ClusterExtensionRevision{}
+			if err := bc.Client.Get(ctx, client.ObjectKey{Name: currentRevision.Name}, updated); err != nil {
+				return false, "", fmt.Errorf("fetching updated revision: %w", err)
+			}
+			currentRevision = updated
 		default:
 			return false, "", fmt.Errorf("patching %s Revision: %w", desiredRevision.Name, err)
 		}
@@ -354,8 +402,8 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust
 		desiredRevision.Name = fmt.Sprintf("%s-%d", ext.Name, revisionNumber)
 		desiredRevision.Spec.Revision = revisionNumber
 
-		if err = bc.setPreviousRevisions(ctx, desiredRevision, prevRevisions); err != nil {
-			return false, "", fmt.Errorf("garbage collecting old Revisions: %w", err)
+		if err = bc.garbageCollectOldRevisions(ctx, prevRevisions); err != nil {
+			return false, "", fmt.Errorf("garbage collecting old revisions: %w", err)
 		}
 
 		if err := bc.createOrUpdate(ctx, desiredRevision); err != nil {
@@ -380,28 +428,21 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust
 	return true, "", nil
 }
 
-// setPreviousRevisions populates spec.previous of latestRevision, trimming the list of previous _archived_ revisions down to
-// ClusterExtensionRevisionPreviousLimit or to the first _active_ revision and deletes trimmed revisions from the cluster.
-// NOTE: revisionList must be sorted in chronographical order, from oldest to latest.
-func (bc *Boxcutter) setPreviousRevisions(ctx context.Context, latestRevision *ocv1.ClusterExtensionRevision, revisionList []ocv1.ClusterExtensionRevision) error {
-	// Pre-allocate with capacity limit to reduce allocations
-	trimmedPrevious := make([]ocv1.ClusterExtensionRevisionPrevious, 0, ClusterExtensionRevisionPreviousLimit)
+// garbageCollectOldRevisions deletes archived revisions beyond ClusterExtensionRevisionRetentionLimit.
+// Active revisions are never deleted. revisionList must be sorted oldest to newest.
+func (bc *Boxcutter) garbageCollectOldRevisions(ctx context.Context, revisionList []ocv1.ClusterExtensionRevision) error {
 	for index, r := range revisionList {
-		if index < len(revisionList)-ClusterExtensionRevisionPreviousLimit && r.Spec.LifecycleState == ocv1.ClusterExtensionRevisionLifecycleStateArchived {
-			// Delete oldest CREs from the cluster and list to reach ClusterExtensionRevisionPreviousLimit or latest active revision
+		// Only delete archived revisions that are beyond the limit
+		if index < len(revisionList)-ClusterExtensionRevisionRetentionLimit && r.Spec.LifecycleState == ocv1.ClusterExtensionRevisionLifecycleStateArchived {
 			if err := bc.Client.Delete(ctx, &ocv1.ClusterExtensionRevision{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: r.Name,
 				},
 			}); err != nil && !apierrors.IsNotFound(err) {
-				return fmt.Errorf("deleting previous archived Revision: %w", err)
+				return fmt.Errorf("deleting archived revision: %w", err)
 			}
-		} else {
-			// All revisions within the limit or still active are preserved
-			trimmedPrevious = append(trimmedPrevious, ocv1.ClusterExtensionRevisionPrevious{Name: r.Name, UID: r.GetUID()})
 		}
 	}
-	latestRevision.Spec.Previous = trimmedPrevious
 	return nil
 }