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) +}