(chore): Use label-based cache for revision lookups instead of explicit chains (#2315)

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

@@ -583,6 +583,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

@@ -31,7 +31,7 @@ import (
 )
 
 const (
-	ClusterExtensionRevisionPreviousLimit = 5
+	ClusterExtensionRevisionRetentionLimit = 5
 )
 
 type ClusterExtensionRevisionGenerator interface {
@@ -187,22 +187,33 @@ func (r *SimpleRevisionGenerator) buildClusterExtensionRevision(
 			},
 		},
 		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{
@@ -234,9 +245,34 @@ 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 to get server-managed fields like Generation
+	if err := m.Client.Get(ctx, client.ObjectKeyFromObject(rev), rev); err != nil {
+		return fmt.Errorf("getting 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(&rev.Status.Conditions, metav1.Condition{
+		Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
+		Status:             metav1.ConditionUnknown,
+		Reason:             ocv1.ClusterExtensionRevisionReasonMigrated,
+		Message:            "Migrated from Helm storage, awaiting cluster state verification",
+		ObservedGeneration: rev.Generation,
+	})
+
+	if err := m.Client.Status().Update(ctx, rev); err != nil {
+		return fmt.Errorf("updating migrated revision status: %w", err)
+	}
+
 	return nil
 }
 
@@ -296,7 +332,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
 
@@ -346,8 +381,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 {
@@ -372,28 +407,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
 }