From 46859908491533957870eea9c7cf8457f02c2a46 Mon Sep 17 00:00:00 2001 From: Harald Klein Date: Wed, 29 Jul 2026 16:15:09 +0200 Subject: [PATCH] Skip adoption of stale generated-by installplans ensureSubscriptionInstallPlanState adopts the installplan that the olm.generated-by annotation names when a subscription has no installplan reference. The code does not check the state of that installplan. On a cluster that upgraded after the subscription was created, adoption blocks installplan creation for the whole namespace in two ways: - The installplan is gone. Garbage collection keeps only the 5 newest installplans, so this is the normal state on a long-lived cluster. The NotFound error from the lookup aborts syncResolvingNamespace before resolution. As a result, the catalog-operator never creates an installplan again for any subscription in the namespace. - The installplan is complete, and the CSV that it recorded is gone. Adoption resets the subscription to UpgradePending with currentCSV=startingCSV. After an intermediate upgrade, that CSV no longer exists. The sync never persists this change and stops before resolution on every iteration. Deletion of an unapproved installplan triggers both paths on any namespace with dependency-generated subscriptions, for example ODF. Treat a missing installplan as "nothing to adopt". Skip adoption of a terminal installplan only when spec.startingCSV no longer names an existing CSV. This condition protects new dependency subscriptions: they sync after their generating installplan completes, and adoption of that installplan links them to their installed CSV. The catalog-operator still adopts failed installplans when fail-forward is enabled, because fail-forward depends on the subscription reference to the failed installplan. Fixes OCPBUGS-82532 Co-Authored-By: Claude Signed-off-by: Harald Klein --- pkg/controller/operators/catalog/operator.go | 30 ++ .../operators/catalog/operator_test.go | 261 ++++++++++++++++++ 2 files changed, 291 insertions(+) diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go index a8e3677446..57e0c971d7 100644 --- a/pkg/controller/operators/catalog/operator.go +++ b/pkg/controller/operators/catalog/operator.go @@ -1668,11 +1668,41 @@ 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") + // A plan in a terminal phase is only worth adopting right after it + // created this subscription, while spec.startingCSV still names an + // existing CSV: adoption then links the subscription to the CSV the + // plan installed. Once that CSV is gone the plan is a historical + // record, and adopting it would reset the subscription to + // UpgradePending with a currentCSV that no longer exists, blocking + // resolution. 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) { + startingCSVExists := false + if sub.Spec.StartingCSV != "" { + _, err := o.client.OperatorsV1alpha1().ClusterServiceVersions(sub.GetNamespace()).Get(context.TODO(), sub.Spec.StartingCSV, metav1.GetOptions{}) + if err != nil && !apierrors.IsNotFound(err) { + return nil, false, err + } + startingCSVExists = err == nil + } + if !startingCSVExists { + logger.WithField("installplan", ipName).Debug("generating installplan in terminal phase and startingCSV not present, 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..0bc262872b 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,263 @@ 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, with the startingCSV removed by + // subsequent upgrades. Adopting it resets the subscription to + // UpgradePending with a currentCSV that no longer exists, and + // resolution never runs again. Terminal plans must not be adopted + // when their startingCSV is gone. + 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) +} + +func TestEnsureSubscriptionInstallPlanStateCompletePlanAdoptedWhenStartingCSVExists(t *testing.T) { + // A dependency subscription created by an installplan may get its first + // sync after that plan has already completed. Adoption is what links the + // subscription to the CSV the plan installed, so a Complete plan must + // still be adopted while spec.startingCSV names an existing CSV. + // Caught by e2e: dependency subscriptions never reached AtLatestKnown + // when adoption of Complete plans was skipped unconditionally. + namespace := "ns" + ctx, cancel := context.WithCancel(context.TODO()) + defer cancel() + ip := &v1alpha1.InstallPlan{ + ObjectMeta: metav1.ObjectMeta{Name: "install-done", Namespace: namespace}, + Status: v1alpha1.InstallPlanStatus{Phase: v1alpha1.InstallPlanPhaseComplete}, + } + csv := &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{Name: "testdep.v0.1.0", Namespace: namespace}, + } + sub := &v1alpha1.Subscription{ + ObjectMeta: metav1.ObjectMeta{ + Name: "sub", + Namespace: namespace, + Annotations: map[string]string{generatedByKey: ip.GetName()}, + }, + Spec: &v1alpha1.SubscriptionSpec{StartingCSV: csv.GetName()}, + } + op, err := NewFakeOperator(ctx, namespace, []string{namespace}, withClientObjs(ip, csv)) + require.NoError(t, err) + + out, changed, err := op.ensureSubscriptionInstallPlanState(logrus.NewEntry(logrus.New()), sub, false) + require.NoError(t, err) + require.True(t, changed) + require.Equal(t, ip.GetName(), out.Status.InstallPlanRef.Name) + require.Equal(t, csv.GetName(), out.Status.CurrentCSV) +} + +func TestEnsureSubscriptionInstallPlanStateNoopCases(t *testing.T) { + // Subscriptions with an existing installplan reference, and subscriptions + // without a generated-by annotation, pass through unchanged. + namespace := "ns" + for _, tc := range []struct { + name string + sub *v1alpha1.Subscription + }{ + { + name: "RefAlreadySet", + sub: &v1alpha1.Subscription{ + ObjectMeta: metav1.ObjectMeta{ + Name: "sub", + Namespace: namespace, + Annotations: map[string]string{generatedByKey: "install-123"}, + }, + Spec: &v1alpha1.SubscriptionSpec{}, + Status: v1alpha1.SubscriptionStatus{ + InstallPlanRef: &corev1.ObjectReference{Name: "install-123", Namespace: namespace}, + Install: &v1alpha1.InstallPlanReference{Name: "install-123"}, + }, + }, + }, + { + name: "NoAnnotation", + sub: &v1alpha1.Subscription{ + ObjectMeta: metav1.ObjectMeta{Name: "sub", Namespace: namespace}, + Spec: &v1alpha1.SubscriptionSpec{}, + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + ctx, cancel := context.WithCancel(context.TODO()) + defer cancel() + op, err := NewFakeOperator(ctx, namespace, []string{namespace}) + require.NoError(t, err) + + out, changed, err := op.ensureSubscriptionInstallPlanState(logrus.NewEntry(logrus.New()), tc.sub, false) + require.NoError(t, err) + require.False(t, changed) + require.Equal(t, tc.sub, out) + }) + } +} + +func TestEnsureSubscriptionInstallPlanStateTerminalPlanCSVLookupTransientError(t *testing.T) { + // A non-NotFound error on the startingCSV lookup inside the terminal-plan + // check must fail the sync so it is retried. + namespace := "ns" + ctx, cancel := context.WithCancel(context.TODO()) + defer cancel() + ip := &v1alpha1.InstallPlan{ + ObjectMeta: metav1.ObjectMeta{Name: "install-done", Namespace: namespace}, + Status: v1alpha1.InstallPlanStatus{Phase: v1alpha1.InstallPlanPhaseComplete}, + } + 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"}, + } + transientErr := errors.New("transient apiserver error") + op, err := NewFakeOperator(ctx, namespace, []string{namespace}, withClientObjs(ip), + withFakeClientOptions(func(c clientfake.ClientsetDecorator) { + c.PrependReactor("get", "clusterserviceversions", 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) +}