OCPBUGS-82532: Skip adoption of missing or terminal generated-by installplans

pkg/controller/operators/catalog/operator.go

@@ -1668,11 +1668,29 @@ func (o *Operator) ensureSubscriptionInstallPlanState(logger *logrus.Entry, sub
 
 	ip, err := o.client.OperatorsV1alpha1().InstallPlans(sub.GetNamespace()).Get(context.TODO(), ipName, metav1.GetOptions{})
 	if err != nil {
+		if apierrors.IsNotFound(err) {
+			// The annotated installplan may have been GC'd. Not an error:
+			// there is nothing to adopt, and returning an error here blocks
+			// resolution for the whole namespace.
+			logger.WithField("installplan", ipName).Debug("generating installplan no longer exists, skipping adoption")
+			return sub, false, nil
+		}
 		logger.WithField("installplan", ipName).Warn("unable to get installplan from cache")
 		return nil, false, err
 	}
 	logger.WithField("installplan", ipName).Debug("found installplan that generated subscription")
 
+	// Only adopt installplans that are still in progress. A plan in a
+	// terminal phase is a historical record: adopting it would reset the
+	// subscription to UpgradePending with currentCSV=startingCSV, which no
+	// longer exists after upgrades, and resolution would never run again.
+	// Failed plans are still adopted when fail-forward is enabled, which
+	// depends on the subscription referencing them.
+	if ip.Status.Phase == v1alpha1.InstallPlanPhaseComplete || (ip.Status.Phase == v1alpha1.InstallPlanPhaseFailed && !failForwardEnabled) {
+		logger.WithField("installplan", ipName).Debug("generating installplan in terminal phase, skipping adoption")
+		return sub, false, nil
+	}
+
 	out := sub.DeepCopy()
 	ref, err := reference.GetReference(ip)
 	if err != nil {

pkg/controller/operators/catalog/operator_test.go

@@ -43,6 +43,7 @@ import (
 	"k8s.io/client-go/informers"
 	k8sfake "k8s.io/client-go/kubernetes/fake"
 	"k8s.io/client-go/rest"
+	clitesting "k8s.io/client-go/testing"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/client-go/util/workqueue"
@@ -2681,3 +2682,148 @@ func TestEnsureInstallPlanConcurrency(t *testing.T) {
 	createdIP := &ipList.Items[0]
 	require.Equal(t, gen, createdIP.Spec.Generation, "InstallPlan should have the correct generation")
 }
+
+func TestEnsureSubscriptionInstallPlanState(t *testing.T) {
+	namespace := "ns"
+	newSub := func(annotations map[string]string) *v1alpha1.Subscription {
+		return &v1alpha1.Subscription{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:        "sub",
+				Namespace:   namespace,
+				Annotations: annotations,
+			},
+			Spec: &v1alpha1.SubscriptionSpec{StartingCSV: "testop.v0.1.0"},
+		}
+	}
+	logger := logrus.NewEntry(logrus.New())
+
+	t.Run("GeneratedByPlanMissingIsNotAnError", func(t *testing.T) {
+		// OCPBUGS-82532: the plan named by the generated-by annotation may
+		// have been GC'd. This must not be an error: syncResolvingNamespace
+		// bails out on error before resolving, so no new installplan would
+		// ever be created for the namespace.
+		ctx, cancel := context.WithCancel(context.TODO())
+		defer cancel()
+		sub := newSub(map[string]string{generatedByKey: "install-gone"})
+		op, err := NewFakeOperator(ctx, namespace, []string{namespace})
+		require.NoError(t, err)
+
+		out, changed, err := op.ensureSubscriptionInstallPlanState(logger, sub, false)
+		require.NoError(t, err)
+		require.False(t, changed)
+		require.Equal(t, sub, out)
+	})
+
+	t.Run("GeneratedByPlanExistsIsAdopted", func(t *testing.T) {
+		// The generating plan exists and is still in progress: its
+		// reference is adopted into the subscription status.
+		ctx, cancel := context.WithCancel(context.TODO())
+		defer cancel()
+		ip := &v1alpha1.InstallPlan{
+			ObjectMeta: metav1.ObjectMeta{Name: "install-123", Namespace: namespace},
+		}
+		sub := newSub(map[string]string{generatedByKey: ip.GetName()})
+		op, err := NewFakeOperator(ctx, namespace, []string{namespace}, withClientObjs(ip))
+		require.NoError(t, err)
+
+		out, changed, err := op.ensureSubscriptionInstallPlanState(logger, sub, false)
+		require.NoError(t, err)
+		require.True(t, changed)
+		require.Equal(t, ip.GetName(), out.Status.InstallPlanRef.Name)
+		require.Equal(t, v1alpha1.SubscriptionState(v1alpha1.SubscriptionStateUpgradePending), out.Status.State)
+		require.Equal(t, sub.Spec.StartingCSV, out.Status.CurrentCSV)
+	})
+}
+
+func TestEnsureSubscriptionInstallPlanStateTransientError(t *testing.T) {
+	// Only NotFound is safe to skip; any other error fetching the
+	// generated-by plan must still fail the sync so it is retried.
+	namespace := "ns"
+	ctx, cancel := context.WithCancel(context.TODO())
+	defer cancel()
+	sub := &v1alpha1.Subscription{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        "sub",
+			Namespace:   namespace,
+			Annotations: map[string]string{generatedByKey: "install-123"},
+		},
+		Spec: &v1alpha1.SubscriptionSpec{StartingCSV: "testop.v0.1.0"},
+	}
+	transientErr := errors.New("transient apiserver error")
+	op, err := NewFakeOperator(ctx, namespace, []string{namespace},
+		withFakeClientOptions(func(c clientfake.ClientsetDecorator) {
+			c.PrependReactor("get", "installplans", func(clitesting.Action) (bool, runtime.Object, error) {
+				return true, nil, transientErr
+			})
+		}),
+	)
+	require.NoError(t, err)
+
+	out, changed, err := op.ensureSubscriptionInstallPlanState(logrus.NewEntry(logrus.New()), sub, false)
+	require.ErrorIs(t, err, transientErr)
+	require.False(t, changed)
+	require.Nil(t, out)
+}
+
+func TestEnsureSubscriptionInstallPlanStateTerminalPlanNotAdopted(t *testing.T) {
+	// OCPBUGS-82532: the generated-by annotation may name a plan that still
+	// exists but completed long ago. Adopting it resets the subscription to
+	// UpgradePending with currentCSV=startingCSV, which no longer exists
+	// after upgrades, and resolution never runs again. Terminal plans must
+	// not be adopted.
+	namespace := "ns"
+	for _, phase := range []v1alpha1.InstallPlanPhase{v1alpha1.InstallPlanPhaseComplete, v1alpha1.InstallPlanPhaseFailed} {
+		t.Run(string(phase), func(t *testing.T) {
+			ctx, cancel := context.WithCancel(context.TODO())
+			defer cancel()
+			ip := &v1alpha1.InstallPlan{
+				ObjectMeta: metav1.ObjectMeta{Name: "install-old", Namespace: namespace},
+				Status:     v1alpha1.InstallPlanStatus{Phase: phase},
+			}
+			sub := &v1alpha1.Subscription{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:        "sub",
+					Namespace:   namespace,
+					Annotations: map[string]string{generatedByKey: ip.GetName()},
+				},
+				Spec: &v1alpha1.SubscriptionSpec{StartingCSV: "testop.v0.1.0"},
+			}
+			op, err := NewFakeOperator(ctx, namespace, []string{namespace}, withClientObjs(ip))
+			require.NoError(t, err)
+
+			out, changed, err := op.ensureSubscriptionInstallPlanState(logrus.NewEntry(logrus.New()), sub, false)
+			require.NoError(t, err)
+			require.False(t, changed)
+			require.Equal(t, sub, out)
+		})
+	}
+}
+
+func TestEnsureSubscriptionInstallPlanStateFailedPlanAdoptedWithFailForward(t *testing.T) {
+	// With fail-forward enabled a Failed generating plan is still adopted:
+	// fail-forward depends on the subscription referencing the failed plan.
+	namespace := "ns"
+	ctx, cancel := context.WithCancel(context.TODO())
+	defer cancel()
+	ip := &v1alpha1.InstallPlan{
+		ObjectMeta: metav1.ObjectMeta{Name: "install-failed", Namespace: namespace},
+		Status:     v1alpha1.InstallPlanStatus{Phase: v1alpha1.InstallPlanPhaseFailed},
+	}
+	sub := &v1alpha1.Subscription{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        "sub",
+			Namespace:   namespace,
+			Annotations: map[string]string{generatedByKey: ip.GetName()},
+		},
+		Spec: &v1alpha1.SubscriptionSpec{StartingCSV: "testop.v0.1.0"},
+	}
+	op, err := NewFakeOperator(ctx, namespace, []string{namespace}, withClientObjs(ip))
+	require.NoError(t, err)
+
+	const failForwardEnabled = true
+	out, changed, err := op.ensureSubscriptionInstallPlanState(logrus.NewEntry(logrus.New()), sub, failForwardEnabled)
+	require.NoError(t, err)
+	require.True(t, changed)
+	require.Equal(t, ip.GetName(), out.Status.InstallPlanRef.Name)
+	require.Equal(t, v1alpha1.SubscriptionState(v1alpha1.SubscriptionStateFailed), out.Status.State)
+}