From bdc017aad4b5ba40d071ff95c8d58339cbdb6ad0 Mon Sep 17 00:00:00 2001 From: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Date: Sun, 11 Jan 2026 05:39:18 +0000 Subject: [PATCH 1/4] (fix) Helm to Boxcutter migration during OLM upgrade When upgrading OLM from standard (Helm runtime) to experimental (Boxcutter runtime), the BoxcutterStorageMigrator creates a ClusterExtensionRevision from the existing Helm release. However, the migrated revision was created without status conditions, causing a race condition where it wasn't recognized as "Installed". Assisted-by: CLAUDE --- .../operator-controller/applier/boxcutter.go | 131 +++++- .../applier/boxcutter_test.go | 407 +++++++++++++++++- .../operator-controller/applier/helm_test.go | 4 +- internal/operator-controller/labels/labels.go | 5 + 4 files changed, 534 insertions(+), 13 deletions(-) diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index f71650329..c5223fdf6 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -15,6 +15,7 @@ import ( "helm.sh/helm/v3/pkg/release" "helm.sh/helm/v3/pkg/storage/driver" apierrors "k8s.io/apimachinery/pkg/api/errors" + "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" @@ -244,8 +245,7 @@ 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. - return nil + return m.ensureMigratedRevisionStatus(ctx, existingRevisionList.Items) } ac, err := m.ActionClientGetter.ActionClientFor(ctx, ext) @@ -262,11 +262,36 @@ func (m *BoxcutterStorageMigrator) Migrate(ctx context.Context, ext *ocv1.Cluste return err } + // Only migrate from a Helm release that represents a deployed, working installation. + // If the latest revision is not deployed (e.g. FAILED), look through the history and + // select the most-recent deployed release instead. + if helmRelease == nil || helmRelease.Info == nil || helmRelease.Info.Status != release.StatusDeployed { + var err error + helmRelease, err = m.findLatestDeployedRelease(ac, ext.GetName()) + if err != nil { + return err + } + if helmRelease == nil { + // No deployed release found in history - skip migration. The ClusterExtension + // controller will handle this via normal rollout. + return nil + } + } + rev, err := m.RevisionGenerator.GenerateRevisionFromHelmRelease(ctx, helmRelease, ext, objectLabels) if err != nil { return err } + // Mark this revision as migrated from Helm so we can distinguish it from + // normal Boxcutter revisions. This label is critical for ensuring we only + // set Succeeded=True status on actually-migrated revisions, not on revision 1 + // created during normal Boxcutter operation. + if rev.Labels == nil { + rev.Labels = make(map[string]string) + } + rev.Labels[labels.MigratedFromHelmKey] = "true" + // 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) @@ -276,9 +301,107 @@ func (m *BoxcutterStorageMigrator) Migrate(ctx context.Context, ext *ocv1.Cluste return err } - // Re-fetch to get server-managed fields like Generation + // Set initial status on the migrated revision to mark it as succeeded. + // + // The revision must have a Succeeded=True status condition immediately after creation. + // + // A revision is only considered "Installed" (vs "RollingOut") when it has this condition. + // Without it, the system cannot determine what version is currently installed, which breaks: + // - Version resolution (can't compute upgrade paths from unknown starting point) + // - Status reporting (installed bundle appears as nil) + // - Subsequent upgrades (resolution fails without knowing current version) + // + // While the ClusterExtensionRevision controller would eventually reconcile and set this status, + // that creates a timing gap where the ClusterExtension reconciliation happens before the status + // is set, causing failures during the OLM upgrade window. + // + // Since we're creating this revision from a successfully deployed Helm release, we know it + // represents a working installation and can safely mark it as succeeded immediately. + return m.ensureRevisionStatus(ctx, rev) +} + +// ensureMigratedRevisionStatus checks if revision 1 exists and needs its status set. +// This handles the case where revision creation succeeded but status update failed. +// Returns nil if no action is needed. +func (m *BoxcutterStorageMigrator) ensureMigratedRevisionStatus(ctx context.Context, revisions []ocv1.ClusterExtensionRevision) error { + for i := range revisions { + if revisions[i].Spec.Revision != 1 { + continue + } + // Only process migrated revisions - skip normal Boxcutter revision 1 to avoid extra API calls. + if revisions[i].Labels[labels.MigratedFromHelmKey] != "true" { + return nil + } + // Skip if already succeeded - status is already set correctly. + if meta.IsStatusConditionTrue(revisions[i].Status.Conditions, ocv1.ClusterExtensionRevisionTypeSucceeded) { + return nil + } + return m.ensureRevisionStatus(ctx, &revisions[i]) + } + // No revision 1 found - migration not applicable (revisions created by normal operation). + return nil +} + +// findLatestDeployedRelease searches the Helm release history for the most recent deployed release. +// Returns nil if no deployed release is found. +func (m *BoxcutterStorageMigrator) findLatestDeployedRelease(ac helmclient.ActionInterface, name string) (*release.Release, error) { + history, err := ac.History(name) + if errors.Is(err, driver.ErrReleaseNotFound) { + // no Helm Release history -> no prior installation. + return nil, nil + } + if err != nil { + return nil, err + } + + var latestDeployed *release.Release + for _, rel := range history { + if rel == nil || rel.Info == nil { + continue + } + if rel.Info.Status != release.StatusDeployed { + continue + } + if latestDeployed == nil || rel.Version > latestDeployed.Version { + latestDeployed = rel + } + } + + return latestDeployed, nil +} + +// ensureRevisionStatus ensures the revision has the Succeeded status condition set. +// Returns nil if the status is already set or after successfully setting it. +// Only sets status on revisions that were actually migrated from Helm (marked with MigratedFromHelmKey label). +func (m *BoxcutterStorageMigrator) ensureRevisionStatus(ctx context.Context, rev *ocv1.ClusterExtensionRevision) error { + // Re-fetch to get latest version before checking status if err := m.Client.Get(ctx, client.ObjectKeyFromObject(rev), rev); err != nil { - return fmt.Errorf("getting created revision: %w", err) + return fmt.Errorf("getting existing revision for status check: %w", err) + } + + // Only set status if this revision was actually migrated from Helm. + // This prevents us from incorrectly marking normal Boxcutter revision 1 as succeeded + // when it's still in progress. + if rev.Labels[labels.MigratedFromHelmKey] != "true" { + return nil + } + + // Check if status is already set to Succeeded=True + if meta.IsStatusConditionTrue(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeSucceeded) { + return nil + } + + // Set the Succeeded status condition + meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{ + Type: ocv1.ClusterExtensionRevisionTypeSucceeded, + Status: metav1.ConditionTrue, + Reason: ocv1.ReasonSucceeded, + Message: "Revision succeeded - migrated from Helm release", + ObservedGeneration: rev.GetGeneration(), + }) + + if err := m.Client.Status().Update(ctx, rev); err != nil { + return fmt.Errorf("updating migrated revision status: %w", err) } return nil diff --git a/internal/operator-controller/applier/boxcutter_test.go b/internal/operator-controller/applier/boxcutter_test.go index 02194550f..51cecd262 100644 --- a/internal/operator-controller/applier/boxcutter_test.go +++ b/internal/operator-controller/applier/boxcutter_test.go @@ -19,6 +19,7 @@ import ( corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" + apimeta "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" @@ -1210,7 +1211,12 @@ func TestBoxcutterStorageMigrator(t *testing.T) { require.NoError(t, ocv1.AddToScheme(testScheme)) brb := &mockBundleRevisionBuilder{} - mag := &mockActionGetter{} + mag := &mockActionGetter{ + currentRel: &release.Release{ + Name: "test123", + Info: &release.Info{Status: release.StatusDeployed}, + }, + } client := &clientMock{} sm := &applier.BoxcutterStorageMigrator{ RevisionGenerator: brb, @@ -1230,8 +1236,11 @@ func TestBoxcutterStorageMigrator(t *testing.T) { On("Create", mock.Anything, mock.AnythingOfType("*v1.ClusterExtensionRevision"), mock.Anything). Once(). Run(func(args mock.Arguments) { - // Simulate real Kubernetes behavior: Create() populates server-managed fields + // Verify the migration marker label is set before creation rev := args.Get(1).(*ocv1.ClusterExtensionRevision) + require.Equal(t, "true", rev.Labels[labels.MigratedFromHelmKey], "Migration marker label should be set") + + // Simulate real Kubernetes behavior: Create() populates server-managed fields rev.Generation = 1 rev.ResourceVersion = "1" }). @@ -1251,6 +1260,21 @@ func TestBoxcutterStorageMigrator(t *testing.T) { require.NoError(t, err) client.AssertExpectations(t) + + // Verify the migrated revision has Succeeded=True status with Succeeded reason and a migration message + statusWriter := client.Status().(*statusWriterMock) + require.True(t, statusWriter.updateCalled, "Status().Update() should be called during migration") + require.NotNil(t, statusWriter.updatedObj, "Updated object should not be nil") + + rev, ok := statusWriter.updatedObj.(*ocv1.ClusterExtensionRevision) + require.True(t, ok, "Updated object should be a ClusterExtensionRevision") + + succeededCond := apimeta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeSucceeded) + require.NotNil(t, succeededCond, "Succeeded condition should be set") + assert.Equal(t, metav1.ConditionTrue, succeededCond.Status, "Succeeded condition should be True") + assert.Equal(t, ocv1.ReasonSucceeded, succeededCond.Reason, "Reason should be Succeeded") + assert.Equal(t, "Revision succeeded - migrated from Helm release", succeededCond.Message, "Message should indicate Helm migration") + assert.Equal(t, int64(1), succeededCond.ObservedGeneration, "ObservedGeneration should match revision generation") }) t.Run("does not create revision when revisions exist", func(t *testing.T) { @@ -1271,12 +1295,303 @@ func TestBoxcutterStorageMigrator(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "test123"}, } + existingRev := ocv1.ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-revision", + Generation: 2, + Labels: map[string]string{ + labels.MigratedFromHelmKey: "true", + }, + }, + Spec: ocv1.ClusterExtensionRevisionSpec{ + Revision: 1, // Migration creates revision 1 + }, + Status: ocv1.ClusterExtensionRevisionStatus{ + Conditions: []metav1.Condition{ + { + Type: ocv1.ClusterExtensionRevisionTypeSucceeded, + Status: metav1.ConditionTrue, + Reason: ocv1.ReasonSucceeded, + }, + }, + }, + } + + client. + On("List", mock.Anything, mock.AnythingOfType("*v1.ClusterExtensionRevisionList"), mock.Anything). + Run(func(args mock.Arguments) { + list := args.Get(1).(*ocv1.ClusterExtensionRevisionList) + list.Items = []ocv1.ClusterExtensionRevision{existingRev} + }). + Return(nil) + + err := sm.Migrate(t.Context(), ext, map[string]string{"my-label": "my-value"}) + require.NoError(t, err) + + client.AssertExpectations(t) + }) + + t.Run("sets status when revision exists but status is missing", 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{ + ObjectMeta: metav1.ObjectMeta{Name: "test123"}, + } + + existingRev := ocv1.ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-revision", + Generation: 2, + Labels: map[string]string{ + labels.MigratedFromHelmKey: "true", + }, + }, + Spec: ocv1.ClusterExtensionRevisionSpec{ + Revision: 1, // Migration creates revision 1 + }, + // Status is empty - simulating the case where creation succeeded but status update failed + } + + client. + On("List", mock.Anything, mock.AnythingOfType("*v1.ClusterExtensionRevisionList"), mock.Anything). + Run(func(args mock.Arguments) { + list := args.Get(1).(*ocv1.ClusterExtensionRevisionList) + list.Items = []ocv1.ClusterExtensionRevision{existingRev} + }). + Return(nil) + + client. + On("Get", mock.Anything, mock.Anything, mock.AnythingOfType("*v1.ClusterExtensionRevision"), mock.Anything). + Run(func(args mock.Arguments) { + rev := args.Get(2).(*ocv1.ClusterExtensionRevision) + *rev = existingRev + }). + Return(nil) + + err := sm.Migrate(t.Context(), ext, map[string]string{"my-label": "my-value"}) + require.NoError(t, err) + + client.AssertExpectations(t) + + // Verify the status was set + statusWriter := client.Status().(*statusWriterMock) + require.True(t, statusWriter.updateCalled, "Status().Update() should be called to set missing status") + require.NotNil(t, statusWriter.updatedObj, "Updated object should not be nil") + + rev, ok := statusWriter.updatedObj.(*ocv1.ClusterExtensionRevision) + require.True(t, ok, "Updated object should be a ClusterExtensionRevision") + + succeededCond := apimeta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeSucceeded) + require.NotNil(t, succeededCond, "Succeeded condition should be set") + assert.Equal(t, metav1.ConditionTrue, succeededCond.Status, "Succeeded condition should be True") + assert.Equal(t, ocv1.ReasonSucceeded, succeededCond.Reason, "Reason should be Succeeded") + }) + + t.Run("updates status from False to True for migrated 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{ + ObjectMeta: metav1.ObjectMeta{Name: "test123"}, + } + + // Revision with Succeeded=False (e.g., from a failed status update attempt) + // This is the specific scenario from Copilot comment #1 + existingRev := ocv1.ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-revision", + Generation: 2, + Labels: map[string]string{ + labels.MigratedFromHelmKey: "true", + }, + }, + Spec: ocv1.ClusterExtensionRevisionSpec{ + Revision: 1, + }, + Status: ocv1.ClusterExtensionRevisionStatus{ + Conditions: []metav1.Condition{ + { + Type: ocv1.ClusterExtensionRevisionTypeSucceeded, + Status: metav1.ConditionFalse, // Important: False, not missing + Reason: "InProgress", + }, + }, + }, + } + client. On("List", mock.Anything, mock.AnythingOfType("*v1.ClusterExtensionRevisionList"), mock.Anything). Run(func(args mock.Arguments) { list := args.Get(1).(*ocv1.ClusterExtensionRevisionList) - list.Items = []ocv1.ClusterExtensionRevision{ - {}, {}, // Existing revisions. + list.Items = []ocv1.ClusterExtensionRevision{existingRev} + }). + Return(nil) + + client. + On("Get", mock.Anything, mock.Anything, mock.AnythingOfType("*v1.ClusterExtensionRevision"), mock.Anything). + Run(func(args mock.Arguments) { + rev := args.Get(2).(*ocv1.ClusterExtensionRevision) + *rev = existingRev + }). + Return(nil) + + err := sm.Migrate(t.Context(), ext, map[string]string{"my-label": "my-value"}) + require.NoError(t, err) + + client.AssertExpectations(t) + + // Verify the status was updated from False to True + statusWriter := client.Status().(*statusWriterMock) + require.True(t, statusWriter.updateCalled, "Status().Update() should be called to update False to True") + require.NotNil(t, statusWriter.updatedObj, "Updated object should not be nil") + + rev, ok := statusWriter.updatedObj.(*ocv1.ClusterExtensionRevision) + require.True(t, ok, "Updated object should be a ClusterExtensionRevision") + + succeededCond := apimeta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeSucceeded) + require.NotNil(t, succeededCond, "Succeeded condition should be set") + assert.Equal(t, metav1.ConditionTrue, succeededCond.Status, "Succeeded condition should be updated to True") + assert.Equal(t, ocv1.ReasonSucceeded, succeededCond.Reason, "Reason should be Succeeded") + }) + + t.Run("does not set status on non-migrated revision 1", 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{ + ObjectMeta: metav1.ObjectMeta{Name: "test123"}, + } + + // Revision 1 created by normal Boxcutter operation (no migration label) + // This simulates the first rollout - status should NOT be set as it may still be in progress + existingRev := ocv1.ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-revision", + Generation: 2, + // No migration label - this is a normal Boxcutter revision + }, + Spec: ocv1.ClusterExtensionRevisionSpec{ + Revision: 1, + }, + } + + client. + On("List", mock.Anything, mock.AnythingOfType("*v1.ClusterExtensionRevisionList"), mock.Anything). + Run(func(args mock.Arguments) { + list := args.Get(1).(*ocv1.ClusterExtensionRevisionList) + list.Items = []ocv1.ClusterExtensionRevision{existingRev} + }). + Return(nil) + + err := sm.Migrate(t.Context(), ext, map[string]string{"my-label": "my-value"}) + require.NoError(t, err) + + client.AssertExpectations(t) + + // Verify the status was NOT set for non-migrated revision + statusWriter := client.Status().(*statusWriterMock) + require.False(t, statusWriter.updateCalled, "Status().Update() should NOT be called for non-migrated revisions") + }) + + t.Run("migrates from most recent deployed release when latest is failed", func(t *testing.T) { + testScheme := runtime.NewScheme() + require.NoError(t, ocv1.AddToScheme(testScheme)) + + brb := &mockBundleRevisionBuilder{} + mag := &mockActionGetter{ + currentRel: &release.Release{ + Name: "test123", + Version: 3, + Info: &release.Info{Status: release.StatusFailed}, + }, + history: []*release.Release{ + { + Name: "test123", + Version: 3, + Info: &release.Info{Status: release.StatusFailed}, + }, + { + Name: "test123", + Version: 2, + Info: &release.Info{Status: release.StatusDeployed}, + }, + { + Name: "test123", + Version: 1, + Info: &release.Info{Status: release.StatusSuperseded}, + }, + }, + } + client := &clientMock{} + sm := &applier.BoxcutterStorageMigrator{ + RevisionGenerator: brb, + ActionClientGetter: mag, + Client: client, + Scheme: testScheme, + } + + ext := &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{Name: "test123"}, + } + + client. + On("List", mock.Anything, mock.AnythingOfType("*v1.ClusterExtensionRevisionList"), mock.Anything). + Return(nil) + + client. + On("Create", mock.Anything, mock.AnythingOfType("*v1.ClusterExtensionRevision"), mock.Anything). + Once(). + Run(func(args mock.Arguments) { + // Verify the migration marker label is set before creation + rev := args.Get(1).(*ocv1.ClusterExtensionRevision) + require.Equal(t, "true", rev.Labels[labels.MigratedFromHelmKey], "Migration marker label should be set") + + // Simulate real Kubernetes behavior: Create() populates server-managed fields + rev.Generation = 1 + rev.ResourceVersion = "1" + }). + Return(nil) + + client. + On("Get", mock.Anything, mock.Anything, mock.AnythingOfType("*v1.ClusterExtensionRevision"), mock.Anything). + Run(func(args mock.Arguments) { + rev := args.Get(2).(*ocv1.ClusterExtensionRevision) + rev.ObjectMeta.Name = "test-revision" + rev.ObjectMeta.Generation = 1 + rev.ObjectMeta.ResourceVersion = "1" + rev.Labels = map[string]string{ + labels.MigratedFromHelmKey: "true", } }). Return(nil) @@ -1285,6 +1600,70 @@ func TestBoxcutterStorageMigrator(t *testing.T) { require.NoError(t, err) client.AssertExpectations(t) + + // Verify the correct release (version 2, deployed) was used instead of version 3 (failed) + require.NotNil(t, brb.helmReleaseUsed, "GenerateRevisionFromHelmRelease should have been called") + assert.Equal(t, 2, brb.helmReleaseUsed.Version, "Should use version 2 (deployed), not version 3 (failed)") + assert.Equal(t, release.StatusDeployed, brb.helmReleaseUsed.Info.Status, "Should use deployed release") + + // Verify the migrated revision has Succeeded=True status + statusWriter := client.Status().(*statusWriterMock) + require.True(t, statusWriter.updateCalled, "Status().Update() should be called during migration") + require.NotNil(t, statusWriter.updatedObj, "Updated object should not be nil") + + rev, ok := statusWriter.updatedObj.(*ocv1.ClusterExtensionRevision) + require.True(t, ok, "Updated object should be a ClusterExtensionRevision") + + succeededCond := apimeta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeSucceeded) + require.NotNil(t, succeededCond, "Succeeded condition should be set") + assert.Equal(t, metav1.ConditionTrue, succeededCond.Status, "Succeeded condition should be True") + }) + + t.Run("does not create revision when helm release is not deployed and no deployed history", func(t *testing.T) { + testScheme := runtime.NewScheme() + require.NoError(t, ocv1.AddToScheme(testScheme)) + + brb := &mockBundleRevisionBuilder{} + mag := &mockActionGetter{ + currentRel: &release.Release{ + Name: "test123", + Info: &release.Info{Status: release.StatusFailed}, + }, + history: []*release.Release{ + { + Name: "test123", + Version: 2, + Info: &release.Info{Status: release.StatusFailed}, + }, + { + Name: "test123", + Version: 1, + Info: &release.Info{Status: release.StatusFailed}, + }, + }, + } + client := &clientMock{} + sm := &applier.BoxcutterStorageMigrator{ + RevisionGenerator: brb, + ActionClientGetter: mag, + Client: client, + Scheme: testScheme, + } + + ext := &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{Name: "test123"}, + } + + client. + On("List", mock.Anything, mock.AnythingOfType("*v1.ClusterExtensionRevisionList"), mock.Anything). + Return(nil) + + err := sm.Migrate(t.Context(), ext, map[string]string{"my-label": "my-value"}) + require.NoError(t, err) + + client.AssertExpectations(t) + // brb.GenerateRevisionFromHelmRelease should NOT have been called + require.False(t, brb.generateRevisionFromHelmReleaseCalled, "GenerateRevisionFromHelmRelease should NOT be called when no deployed release exists") }) t.Run("does not create revision when no helm release", func(t *testing.T) { @@ -1320,7 +1699,9 @@ func TestBoxcutterStorageMigrator(t *testing.T) { // mockBundleRevisionBuilder is a mock implementation of the ClusterExtensionRevisionGenerator for testing. type mockBundleRevisionBuilder struct { - makeRevisionFunc func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotation map[string]string) (*ocv1.ClusterExtensionRevision, error) + makeRevisionFunc func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotation map[string]string) (*ocv1.ClusterExtensionRevision, error) + generateRevisionFromHelmReleaseCalled bool + helmReleaseUsed *release.Release } func (m *mockBundleRevisionBuilder) GenerateRevision(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) { @@ -1332,6 +1713,8 @@ func (m *mockBundleRevisionBuilder) GenerateRevisionFromHelmRelease( helmRelease *release.Release, ext *ocv1.ClusterExtension, objectLabels map[string]string, ) (*ocv1.ClusterExtensionRevision, error) { + m.generateRevisionFromHelmReleaseCalled = true + m.helmReleaseUsed = helmRelease return &ocv1.ClusterExtensionRevision{ ObjectMeta: metav1.ObjectMeta{ Name: "test-revision", @@ -1345,6 +1728,7 @@ func (m *mockBundleRevisionBuilder) GenerateRevisionFromHelmRelease( type clientMock struct { mock.Mock + statusWriter *statusWriterMock } func (m *clientMock) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { @@ -1363,15 +1747,22 @@ func (m *clientMock) Create(ctx context.Context, obj client.Object, opts ...clie } func (m *clientMock) Status() client.StatusWriter { - return &statusWriterMock{mock: &m.Mock} + if m.statusWriter == nil { + m.statusWriter = &statusWriterMock{mock: &m.Mock} + } + return m.statusWriter } type statusWriterMock struct { - mock *mock.Mock + mock *mock.Mock + updatedObj client.Object + updateCalled bool } func (s *statusWriterMock) Update(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error { - // Status updates are expected during migration - return success by default + // Capture the status update for test verification + s.updatedObj = obj + s.updateCalled = true return nil } diff --git a/internal/operator-controller/applier/helm_test.go b/internal/operator-controller/applier/helm_test.go index a1332812d..25fad4d66 100644 --- a/internal/operator-controller/applier/helm_test.go +++ b/internal/operator-controller/applier/helm_test.go @@ -98,6 +98,7 @@ func (mockHelmReleaseToObjectsConverter) GetObjectsFromRelease(*release.Release) type mockActionGetter struct { actionClientForErr error getClientErr error + historyErr error installErr error dryRunInstallErr error upgradeErr error @@ -105,6 +106,7 @@ type mockActionGetter struct { reconcileErr error desiredRel *release.Release currentRel *release.Release + history []*release.Release } func (mag *mockActionGetter) ActionClientFor(ctx context.Context, obj client.Object) (helmclient.ActionInterface, error) { @@ -116,7 +118,7 @@ func (mag *mockActionGetter) Get(name string, opts ...helmclient.GetOption) (*re } func (mag *mockActionGetter) History(name string, opts ...helmclient.HistoryOption) ([]*release.Release, error) { - return nil, mag.getClientErr + return mag.history, mag.historyErr } func (mag *mockActionGetter) Install(name, namespace string, chrt *chart.Chart, vals map[string]interface{}, opts ...helmclient.InstallOption) (*release.Release, error) { diff --git a/internal/operator-controller/labels/labels.go b/internal/operator-controller/labels/labels.go index 52fa6e2b1..16f45ecbb 100644 --- a/internal/operator-controller/labels/labels.go +++ b/internal/operator-controller/labels/labels.go @@ -39,4 +39,9 @@ const ( // so that the effective ServiceAccount identity used for // ClusterExtensionRevision operations is preserved. ServiceAccountNamespaceKey = "olm.operatorframework.io/service-account-namespace" + + // MigratedFromHelmKey is the label key used to mark ClusterExtensionRevisions + // that were created during migration from Helm releases. This label is used + // to distinguish migrated revisions from those created by normal Boxcutter operation. + MigratedFromHelmKey = "olm.operatorframework.io/migrated-from-helm" ) From aed04b43fa5cdd6f868d31b51207188b49e67381 Mon Sep 17 00:00:00 2001 From: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Date: Fri, 30 Jan 2026 17:06:41 +0000 Subject: [PATCH 2/4] Update internal/operator-controller/applier/boxcutter_test.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- internal/operator-controller/applier/boxcutter_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/operator-controller/applier/boxcutter_test.go b/internal/operator-controller/applier/boxcutter_test.go index 51cecd262..2e0cd4814 100644 --- a/internal/operator-controller/applier/boxcutter_test.go +++ b/internal/operator-controller/applier/boxcutter_test.go @@ -1416,8 +1416,8 @@ func TestBoxcutterStorageMigrator(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "test123"}, } - // Revision with Succeeded=False (e.g., from a failed status update attempt) - // This is the specific scenario from Copilot comment #1 + // Migrated revision with Succeeded=False (e.g., from a previous failed status update attempt) + // This simulates a revision whose Succeeded condition should be corrected from False to True during migration. existingRev := ocv1.ClusterExtensionRevision{ ObjectMeta: metav1.ObjectMeta{ Name: "test-revision", From d9cc479532dcb3ab30a8b6f6e790b1de0082484d Mon Sep 17 00:00:00 2001 From: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Date: Fri, 30 Jan 2026 17:20:49 +0000 Subject: [PATCH 3/4] Update internal/operator-controller/applier/boxcutter.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- internal/operator-controller/applier/boxcutter.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index c5223fdf6..f9f9cc128 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -328,14 +328,12 @@ func (m *BoxcutterStorageMigrator) ensureMigratedRevisionStatus(ctx context.Cont if revisions[i].Spec.Revision != 1 { continue } - // Only process migrated revisions - skip normal Boxcutter revision 1 to avoid extra API calls. - if revisions[i].Labels[labels.MigratedFromHelmKey] != "true" { - return nil - } // Skip if already succeeded - status is already set correctly. if meta.IsStatusConditionTrue(revisions[i].Status.Conditions, ocv1.ClusterExtensionRevisionTypeSucceeded) { return nil } + // Ensure revision 1 status is set correctly, including for previously migrated + // revisions that may not carry the MigratedFromHelm label. return m.ensureRevisionStatus(ctx, &revisions[i]) } // No revision 1 found - migration not applicable (revisions created by normal operation). From 353a41e253b40d2dba07e5356a7fedc10817926d Mon Sep 17 00:00:00 2001 From: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Date: Fri, 30 Jan 2026 17:38:41 +0000 Subject: [PATCH 4/4] Add test requested by copilot --- internal/operator-controller/applier/boxcutter_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/internal/operator-controller/applier/boxcutter_test.go b/internal/operator-controller/applier/boxcutter_test.go index 2e0cd4814..6c27b13bd 100644 --- a/internal/operator-controller/applier/boxcutter_test.go +++ b/internal/operator-controller/applier/boxcutter_test.go @@ -1514,6 +1514,16 @@ func TestBoxcutterStorageMigrator(t *testing.T) { }). Return(nil) + // The migration flow calls Get() to re-fetch the revision before checking its status. + // Even for non-migrated revisions, Get() is called to determine if status needs to be set. + client. + On("Get", mock.Anything, mock.Anything, mock.AnythingOfType("*v1.ClusterExtensionRevision"), mock.Anything). + Run(func(args mock.Arguments) { + rev := args.Get(2).(*ocv1.ClusterExtensionRevision) + *rev = existingRev + }). + Return(nil) + err := sm.Migrate(t.Context(), ext, map[string]string{"my-label": "my-value"}) require.NoError(t, err)