Repository: operator-framework/operator-lifecycle-manager
Pull request #1894 (2020-12-07 17:38:56)

[release-4.5] Bug 1905137: Recreate pending installplan if deleted before approval
<!--

Before making a PR, please read our contributing guidelines https://github.com/operator-framework/operator-lifecycle-manager/blob/master/CONTRIBUTING.md

Note: Make sure your branch is rebased to the latest upstream master.

-->

**Description of the change:**


**Motivation for the change:**

**Reviewer Checklist**
- [ ] Implementation matches the proposed design, or proposal is updated to match implementation
- [ ] Sufficient unit test coverage 
- [ ] Sufficient end-to-end test coverage
- [ ] Docs updated or added to `/docs` 
- [ ] Commit messages sensible and descriptive
fix(ip): Recreate pending installplan if deleted before approval

At the moment, OLM will not recreate the pending-approval
installplan. The subscription will be stuck in UpgradePending forever
as a result. This PR will enable installplan recreation if it is deleted
before approval.

Signed-off-by: Vu Dinh <vdinh@redhat.com>

<!--

Note: If this PR is fixing an issue make sure to add a note saying:
Closes #<ISSUE_NUMBER>

-->



Patch:
diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go
index 1dcad9007..537eb46ed 100644
--- a/pkg/controller/operators/catalog/operator.go
+++ b/pkg/controller/operators/catalog/operator.go
@@ -936,7 +936,7 @@ func (o *Operator) nothingToUpdate(logger *logrus.Entry, sub *v1alpha1.Subscript
 }
 
 func (o *Operator) ensureSubscriptionInstallPlanState(logger *logrus.Entry, sub *v1alpha1.Subscription) (*v1alpha1.Subscription, bool, error) {
-	if sub.Status.InstallPlanRef != nil {
+	if sub.Status.InstallPlanRef != nil || sub.Status.Install != nil {
 		return sub, false, nil
 	}
 
diff --git a/pkg/controller/operators/catalog/subscription/state.go b/pkg/controller/operators/catalog/subscription/state.go
index 5717b24e6..f5da96614 100644
--- a/pkg/controller/operators/catalog/subscription/state.go
+++ b/pkg/controller/operators/catalog/subscription/state.go
@@ -385,12 +385,22 @@ func (i *installPlanReferencedState) InstallPlanNotFound(now *metav1.Time, clien
 	// Remove pending and failed conditions
 	out.Status.RemoveConditions(v1alpha1.SubscriptionInstallPlanPending, v1alpha1.SubscriptionInstallPlanFailed)
 
-	// Set missing condition to true
-	missingCond := out.Status.GetCondition(v1alpha1.SubscriptionInstallPlanMissing)
-	missingCond.Status = corev1.ConditionTrue
-	missingCond.Reason = v1alpha1.ReferencedInstallPlanNotFound
-	missingCond.LastTransitionTime = now
-	out.Status.SetCondition(missingCond)
+	// If the installplan is missing when subscription is in pending upgrade,
+	// clear the installplan ref so the resolution can happen again
+	if in.Status.State == v1alpha1.SubscriptionStateUpgradePending {
+		out.Status.InstallPlanRef = nil
+		out.Status.Install = nil
+		out.Status.CurrentCSV = ""
+		out.Status.State = v1alpha1.SubscriptionStateNone
+		out.Status.LastUpdated = *now
+	} else {
+		// Set missing condition to true
+		cond := out.Status.GetCondition(v1alpha1.SubscriptionInstallPlanMissing)
+		cond.Status = corev1.ConditionTrue
+		cond.Reason = v1alpha1.ReferencedInstallPlanNotFound
+		cond.LastTransitionTime = now
+		out.Status.SetCondition(cond)
+	}
 
 	// Build missing state
 	missingState := &installPlanMissingState{


Test patch:
diff --git a/pkg/controller/operators/catalog/subscription/reconciler_test.go b/pkg/controller/operators/catalog/subscription/reconciler_test.go
index 285e51c7d..54ed929c5 100644
--- a/pkg/controller/operators/catalog/subscription/reconciler_test.go
+++ b/pkg/controller/operators/catalog/subscription/reconciler_test.go
@@ -1171,6 +1171,63 @@ func TestInstallPlanReconcile(t *testing.T) {
 				}),
 			},
 		},
+		{
+			description: "SubscriptionExistsToInstallPlanNotFound/SubscriptionStateUpgradePending/Changes",
+			fields: fields{
+				config: &fakeReconcilerConfig{
+					now: nowFunc,
+					existingObjs: existingObjs{
+						clientObjs: []runtime.Object{
+							&v1alpha1.Subscription{
+								ObjectMeta: metav1.ObjectMeta{
+									Name:      "sub",
+									Namespace: "ns",
+								},
+								Status: v1alpha1.SubscriptionStatus{
+									InstallPlanRef: &corev1.ObjectReference{
+										Namespace: "ns",
+										Name:      "ip",
+									},
+									LastUpdated: earlier,
+									State:       v1alpha1.SubscriptionStateUpgradePending,
+								},
+							},
+						},
+					},
+				},
+			},
+			args: args{
+				in: newSubscriptionExistsState(&v1alpha1.Subscription{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "sub",
+						Namespace: "ns",
+					},
+					Status: v1alpha1.SubscriptionStatus{
+						InstallPlanRef: &corev1.ObjectReference{
+							Namespace: "ns",
+							Name:      "ip",
+						},
+						LastUpdated: earlier,
+						Conditions: []v1alpha1.SubscriptionCondition{
+							planPendingCondition(corev1.ConditionTrue, string(v1alpha1.InstallPlanPhaseInstalling), "", &earlier),
+						},
+						State: v1alpha1.SubscriptionStateUpgradePending,
+					},
+				}),
+			},
+			want: want{
+				out: newInstallPlanMissingState(&v1alpha1.Subscription{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "sub",
+						Namespace: "ns",
+					},
+					Status: v1alpha1.SubscriptionStatus{
+						LastUpdated: now,
+						State:       v1alpha1.SubscriptionStateNone,
+					},
+				}),
+			},
+		},
 		{
 			description: "SubscriptionExistsToInstallPlanPending/Installing/Conditions/NoChanges",
 			fields: fields{
diff --git a/test/e2e/subscription_e2e_test.go b/test/e2e/subscription_e2e_test.go
index e84acf4a5..59dabefc8 100644
--- a/test/e2e/subscription_e2e_test.go
+++ b/test/e2e/subscription_e2e_test.go
@@ -202,10 +202,38 @@ var _ = Describe("Subscription", func() {
 		require.Equal(GinkgoT(), v1alpha1.ApprovalManual, installPlan.Spec.Approval)
 		require.Equal(GinkgoT(), v1alpha1.InstallPlanPhaseRequiresApproval, installPlan.Status.Phase)
 
-		installPlan.Spec.Approved = true
-		_, err = crc.OperatorsV1alpha1().InstallPlans(testNamespace).Update(context.TODO(), installPlan, metav1.UpdateOptions{})
+		// Delete the current installplan
+		err = crc.OperatorsV1alpha1().InstallPlans(testNamespace).Delete(context.Background(), installPlan.Name, metav1.DeleteOptions{})
 		require.NoError(GinkgoT(), err)
 
+		var ipName string
+		Eventually(func() bool {
+			fetched, err := crc.OperatorsV1alpha1().Subscriptions(testNamespace).Get(context.TODO(), "manual-subscription", metav1.GetOptions{})
+			if err != nil {
+				return false
+			}
+			if fetched.Status.Install != nil {
+				ipName = fetched.Status.Install.Name
+				return fetched.Status.Install.Name != installPlan.Name
+			}
+			return false
+		}, 5*time.Minute, 10*time.Second).Should(BeTrue())
+
+		// Fetch new installplan
+		newInstallPlan, err := fetchInstallPlan(GinkgoT(), crc, ipName, buildInstallPlanPhaseCheckFunc(v1alpha1.InstallPlanPhaseRequiresApproval))
+		require.NoError(GinkgoT(), err)
+		require.NotNil(GinkgoT(), newInstallPlan)
+
+		require.NotEqual(GinkgoT(), installPlan.Name, newInstallPlan.Name, "expected new installplan recreated")
+		require.Equal(GinkgoT(), v1alpha1.ApprovalManual, newInstallPlan.Spec.Approval)
+		require.Equal(GinkgoT(), v1alpha1.InstallPlanPhaseRequiresApproval, newInstallPlan.Status.Phase)
+
+		// Set the InstallPlan's approved to True
+		Eventually(Apply(newInstallPlan, func(p *v1alpha1.InstallPlan) error {
+			p.Spec.Approved = true
+			return nil
+		})).Should(Succeed())
+
 		subscription, err = fetchSubscription(crc, testNamespace, "manual-subscription", subscriptionStateAtLatestChecker)
 		require.NoError(GinkgoT(), err)
 		require.NotNil(GinkgoT(), subscription)