From f3aec062a43008988164d765c197539b8db1031e Mon Sep 17 00:00:00 2001 From: Harald Klein Date: Wed, 29 Jul 2026 16:15:09 +0200 Subject: [PATCH] Skip adoption of missing or terminal generated-by installplans ensureSubscriptionInstallPlanState adopts the installplan named by the olm.generated-by annotation whenever a subscription has no installplan reference. That is only valid while the generating plan is still in progress. Once the plan has been GC'd or has reached a terminal phase, adoption wedges the namespace: - plan missing: the NotFound error aborts syncResolvingNamespace before resolution, so no installplan is ever created again for any subscription in the namespace. - plan complete: the adoption resets the subscription to UpgradePending with currentCSV=startingCSV, which no longer exists after upgrades. The change is never persisted and the sync bails out before resolution on every iteration. Both are hit by deleting an unapproved installplan (a documented way to get a plan for a newer version when one is pending), on any namespace with dependency-generated subscriptions, e.g. ODF. Treat a missing plan as "nothing to adopt" and skip adoption of plans in a terminal phase so resolution can create a fresh installplan. Failed plans are still adopted when fail-forward is enabled, which depends on the subscription referencing them. Fixes OCPBUGS-82532 Co-Authored-By: Claude Signed-off-by: Harald Klein --- pkg/controller/operators/catalog/operator.go | 18 +++ .../operators/catalog/operator_test.go | 146 ++++++++++++++++++ 2 files changed, 164 insertions(+) diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go index a8e3677446..119adac719 100644 --- a/pkg/controller/operators/catalog/operator.go +++ b/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 { diff --git a/pkg/controller/operators/catalog/operator_test.go b/pkg/controller/operators/catalog/operator_test.go index ac0a3346a1..ae70e1d246 100644 --- a/pkg/controller/operators/catalog/operator_test.go +++ b/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) +}