🐛 OCPBUGS-77829: fix(Boxcutter): Fix ClusterExtension spec changes ignored while revision is rolling out

internal/operator-controller/controllers/boxcutter_reconcile_steps.go

@@ -62,10 +62,11 @@ func (d *BoxcutterRevisionStatesGetter) GetRevisionStates(ctx context.Context, e
 		//   is fairly decoupled from this code where we get the annotations back out. We may want to co-locate
 		//   the set/get logic a bit better to make it more maintainable and less likely to get out of sync.
 		rm := &RevisionMetadata{
-			RevisionName: rev.Name,
-			Package:      rev.Annotations[labels.PackageNameKey],
-			Image:        rev.Annotations[labels.BundleReferenceKey],
-			Conditions:   rev.Status.Conditions,
+			RevisionName:      rev.Name,
+			Package:           rev.Annotations[labels.PackageNameKey],
+			Image:             rev.Annotations[labels.BundleReferenceKey],
+			CatalogSpecDigest: rev.Annotations[labels.CatalogSpecDigestKey],
+			Conditions:        rev.Status.Conditions,
 			BundleMetadata: ocv1.BundleMetadata{
 				Name:    rev.Annotations[labels.BundleNameKey],
 				Version: rev.Annotations[labels.BundleVersionKey],
@@ -104,10 +105,11 @@ func ApplyBundleWithBoxcutter(apply func(ctx context.Context, contentFS fs.FS, e
 	return func(ctx context.Context, state *reconcileState, ext *ocv1.ClusterExtension) (*ctrl.Result, error) {
 		l := log.FromContext(ctx)
 		revisionAnnotations := map[string]string{
-			labels.BundleNameKey:      state.resolvedRevisionMetadata.Name,
-			labels.PackageNameKey:     state.resolvedRevisionMetadata.Package,
-			labels.BundleVersionKey:   state.resolvedRevisionMetadata.Version,
-			labels.BundleReferenceKey: state.resolvedRevisionMetadata.Image,
+			labels.BundleNameKey:        state.resolvedRevisionMetadata.Name,
+			labels.PackageNameKey:       state.resolvedRevisionMetadata.Package,
+			labels.BundleVersionKey:     state.resolvedRevisionMetadata.Version,
+			labels.BundleReferenceKey:   state.resolvedRevisionMetadata.Image,
+			labels.CatalogSpecDigestKey: CatalogSpecDigest(ext),
 		}
 		if state.resolvedRevisionMetadata.Release != nil {
 			revisionAnnotations[labels.BundleReleaseKey] = *state.resolvedRevisionMetadata.Release
@@ -154,11 +156,17 @@ func ApplyBundleWithBoxcutter(apply func(ctx context.Context, contentFS fs.FS, e
 					apimeta.SetStatusCondition(&rs.Conditions, *cnd)
 				}
 			}
-			// Mirror Progressing condition from the latest active revision
+			// Mirror Progressing from the latest rolling revision only while the installed
+			// revision does not already satisfy the current spec. Stale rolling revisions
+			// (e.g. a failed upgrade left behind after recovery) must not overwrite Succeeded.
 			if idx == len(state.revisionStates.RollingOut)-1 {
-				if pcnd := apimeta.FindStatusCondition(r.Conditions, ocv1.ClusterObjectSetTypeProgressing); pcnd != nil {
-					pcnd.ObservedGeneration = ext.GetGeneration()
-					apimeta.SetStatusCondition(&ext.Status.Conditions, *pcnd)
+				installedSatisfiesSpec := state.revisionStates.Installed != nil &&
+					versionMatchesSpec(state.revisionStates.Installed.Version, ext)
+				if !installedSatisfiesSpec {
+					if pcnd := apimeta.FindStatusCondition(r.Conditions, ocv1.ClusterObjectSetTypeProgressing); pcnd != nil {
+						pcnd.ObservedGeneration = ext.GetGeneration()
+						apimeta.SetStatusCondition(&ext.Status.Conditions, *pcnd)
+					}
 				}
 			}
 			ext.Status.ActiveRevisions = append(ext.Status.ActiveRevisions, rs)

internal/operator-controller/controllers/boxcutter_reconcile_steps_apply_test.go

@@ -23,6 +23,7 @@ import (
 	"testing/fstest"
 
 	"github.com/stretchr/testify/require"
+	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	ocv1 "github.com/operator-framework/operator-controller/api/v1"
@@ -148,3 +149,72 @@ func TestApplyBundleWithBoxcutter(t *testing.T) {
 		})
 	}
 }
+
+func TestApplyBundleWithBoxcutterInstalledSatisfiesSpecIgnoresStaleRollingProgressing(t *testing.T) {
+	ctx := context.Background()
+	ext := &ocv1.ClusterExtension{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:       "test-ext",
+			Generation: 3,
+		},
+		Spec: ocv1.ClusterExtensionSpec{
+			Source: ocv1.SourceConfig{
+				Catalog: &ocv1.CatalogFilter{
+					PackageName: "test",
+					Version:     "1.2.0",
+				},
+			},
+		},
+	}
+
+	state := &reconcileState{
+		revisionStates: &RevisionStates{
+			Installed: &RevisionMetadata{
+				RevisionName: "test-ext-3",
+				BundleMetadata: ocv1.BundleMetadata{
+					Name:    "test.v1.2.0",
+					Version: "1.2.0",
+				},
+				Conditions: []metav1.Condition{
+					{
+						Type:   ocv1.ClusterObjectSetTypeProgressing,
+						Status: metav1.ConditionTrue,
+						Reason: ocv1.ReasonSucceeded,
+					},
+				},
+			},
+			RollingOut: []*RevisionMetadata{{
+				RevisionName: "test-ext-2",
+				BundleMetadata: ocv1.BundleMetadata{
+					Name:    "test.v1.0.2",
+					Version: "1.0.2",
+				},
+				Conditions: []metav1.Condition{
+					{
+						Type:   ocv1.ClusterObjectSetTypeProgressing,
+						Status: metav1.ConditionTrue,
+						Reason: ocv1.ReasonRollingOut,
+					},
+				},
+			}},
+		},
+		resolvedRevisionMetadata: &RevisionMetadata{
+			BundleMetadata: ocv1.BundleMetadata{
+				Name:    "test.v1.2.0",
+				Version: "1.2.0",
+			},
+		},
+		imageFS: fstest.MapFS{},
+	}
+
+	stepFunc := ApplyBundleWithBoxcutter(func(_ context.Context, _ fs.FS, _ *ocv1.ClusterExtension, _, _ map[string]string) (bool, string, error) {
+		return true, "", nil
+	})
+	_, err := stepFunc(ctx, state, ext)
+	require.NoError(t, err)
+
+	pcnd := apimeta.FindStatusCondition(ext.Status.Conditions, ocv1.TypeProgressing)
+	require.NotNil(t, pcnd)
+	require.Equal(t, ocv1.ReasonSucceeded, pcnd.Reason,
+		"stale rolling revision must not overwrite Progressing when installed matches spec")
+}

internal/operator-controller/controllers/clusterextension_controller.go

@@ -499,9 +499,10 @@ func clusterExtensionRequestsForCatalog(c client.Reader, logger logr.Logger) crh
 }
 
 type RevisionMetadata struct {
-	RevisionName string
-	Package      string
-	Image        string
+	RevisionName      string
+	Package           string
+	Image             string
+	CatalogSpecDigest string
 	ocv1.BundleMetadata
 	Conditions []metav1.Condition
 }