From 5581579cc9f0c2f7939bce87fac575b39daf796d Mon Sep 17 00:00:00 2001 From: Vu Dinh Date: Thu, 29 Oct 2020 18:18:36 -0400 Subject: [PATCH 1/2] 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 --- pkg/controller/operators/catalog/operator.go | 48 ++++++++++++++++++-- test/e2e/subscription_e2e_test.go | 30 +++++++++++- 2 files changed, 72 insertions(+), 6 deletions(-) diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go index f8664c6677..521b23e277 100644 --- a/pkg/controller/operators/catalog/operator.go +++ b/pkg/controller/operators/catalog/operator.go @@ -831,6 +831,12 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error { return err } + ips, err := o.listInstallPlansMap(namespace) + if err != nil { + logger.WithError(err).Debug("couldn't list installplan") + return err + } + // TODO: parallel maxGeneration := 0 subscriptionUpdated := false @@ -847,7 +853,7 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error { } // ensure the installplan reference is correct - sub, changedIP, err := o.ensureSubscriptionInstallPlanState(logger, sub) + sub, changedIP, err := o.ensureSubscriptionInstallPlanState(logger, sub, ips) if err != nil { logger.Debugf("error ensuring installplan state: %v", err) return err @@ -952,9 +958,29 @@ func (o *Operator) nothingToUpdate(logger *logrus.Entry, sub *v1alpha1.Subscript return false } -func (o *Operator) ensureSubscriptionInstallPlanState(logger *logrus.Entry, sub *v1alpha1.Subscription) (*v1alpha1.Subscription, bool, error) { - if sub.Status.InstallPlanRef != nil { - return sub, false, nil +// checkMissingInstallPlan checks if the installplan is missing or not when +// the subscription is in pending upgrade state +func (o *Operator) checkMissingInstallPlan(sub *v1alpha1.Subscription, ips map[string]struct{}) (*v1alpha1.Subscription, bool, error) { + _, ok := ips[sub.Status.InstallPlanRef.Name] + if !ok && sub.Status.State == v1alpha1.SubscriptionStateUpgradePending { + out := sub.DeepCopy() + out.Status.InstallPlanRef = nil + out.Status.Install = nil + out.Status.CurrentCSV = "" + out.Status.State = v1alpha1.SubscriptionStateNone + out.Status.LastUpdated = o.now() + updated, err := o.client.OperatorsV1alpha1().Subscriptions(sub.GetNamespace()).UpdateStatus(context.TODO(), out, metav1.UpdateOptions{}) + if err != nil { + return out, false, nil + } + return updated, true, nil + } + return sub, false, nil +} + +func (o *Operator) ensureSubscriptionInstallPlanState(logger *logrus.Entry, sub *v1alpha1.Subscription, ips map[string]struct{}) (*v1alpha1.Subscription, bool, error) { + if sub.Status.InstallPlanRef != nil || sub.Status.Install != nil { + return o.checkMissingInstallPlan(sub, ips) } logger.Debug("checking for existing installplan") @@ -2020,6 +2046,20 @@ func (o *Operator) listInstallPlans(namespace string) (ips []*v1alpha1.InstallPl return } +func (o *Operator) listInstallPlansMap(namespace string) (ips map[string]struct{}, err error) { + list, err := o.client.OperatorsV1alpha1().InstallPlans(namespace).List(context.TODO(), metav1.ListOptions{}) + if err != nil { + return + } + + ips = make(map[string]struct{}) + for i := range list.Items { + ips[list.Items[i].GetName()] = struct{}{} + } + + return +} + // competingCRDOwnersExist returns true if there exists a CSV that owns at least one of the given CSVs owned CRDs (that's not the given CSV) func competingCRDOwnersExist(namespace string, csv *v1alpha1.ClusterServiceVersion, existingOwners map[string][]string) (bool, error) { // Attempt to find a pre-existing owner in the namespace for any owned crd diff --git a/test/e2e/subscription_e2e_test.go b/test/e2e/subscription_e2e_test.go index eaf3ae577e..df4cac4c05 100644 --- a/test/e2e/subscription_e2e_test.go +++ b/test/e2e/subscription_e2e_test.go @@ -217,8 +217,34 @@ 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.Background(), 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) + + newInstallPlan.Spec.Approved = true + _, err = crc.OperatorsV1alpha1().InstallPlans(testNamespace).Update(context.Background(), newInstallPlan, metav1.UpdateOptions{}) require.NoError(GinkgoT(), err) subscription, err = fetchSubscription(crc, testNamespace, "manual-subscription", subscriptionStateAtLatestChecker) From 3db342eae2530177fc47b7ba85774ace17211478 Mon Sep 17 00:00:00 2001 From: Vu Dinh Date: Wed, 18 Nov 2020 01:45:23 -0500 Subject: [PATCH 2/2] Using subscription reconciliation to clean installplan status instead Signed-off-by: Vu Dinh --- pkg/controller/operators/catalog/operator.go | 46 +-------------- .../catalog/subscription/reconciler_test.go | 57 +++++++++++++++++++ .../operators/catalog/subscription/state.go | 22 +++++-- test/e2e/subscription_e2e_test.go | 7 ++- 4 files changed, 82 insertions(+), 50 deletions(-) diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go index 521b23e277..f8053d6585 100644 --- a/pkg/controller/operators/catalog/operator.go +++ b/pkg/controller/operators/catalog/operator.go @@ -831,12 +831,6 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error { return err } - ips, err := o.listInstallPlansMap(namespace) - if err != nil { - logger.WithError(err).Debug("couldn't list installplan") - return err - } - // TODO: parallel maxGeneration := 0 subscriptionUpdated := false @@ -853,7 +847,7 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error { } // ensure the installplan reference is correct - sub, changedIP, err := o.ensureSubscriptionInstallPlanState(logger, sub, ips) + sub, changedIP, err := o.ensureSubscriptionInstallPlanState(logger, sub) if err != nil { logger.Debugf("error ensuring installplan state: %v", err) return err @@ -958,29 +952,9 @@ func (o *Operator) nothingToUpdate(logger *logrus.Entry, sub *v1alpha1.Subscript return false } -// checkMissingInstallPlan checks if the installplan is missing or not when -// the subscription is in pending upgrade state -func (o *Operator) checkMissingInstallPlan(sub *v1alpha1.Subscription, ips map[string]struct{}) (*v1alpha1.Subscription, bool, error) { - _, ok := ips[sub.Status.InstallPlanRef.Name] - if !ok && sub.Status.State == v1alpha1.SubscriptionStateUpgradePending { - out := sub.DeepCopy() - out.Status.InstallPlanRef = nil - out.Status.Install = nil - out.Status.CurrentCSV = "" - out.Status.State = v1alpha1.SubscriptionStateNone - out.Status.LastUpdated = o.now() - updated, err := o.client.OperatorsV1alpha1().Subscriptions(sub.GetNamespace()).UpdateStatus(context.TODO(), out, metav1.UpdateOptions{}) - if err != nil { - return out, false, nil - } - return updated, true, nil - } - return sub, false, nil -} - -func (o *Operator) ensureSubscriptionInstallPlanState(logger *logrus.Entry, sub *v1alpha1.Subscription, ips map[string]struct{}) (*v1alpha1.Subscription, bool, error) { +func (o *Operator) ensureSubscriptionInstallPlanState(logger *logrus.Entry, sub *v1alpha1.Subscription) (*v1alpha1.Subscription, bool, error) { if sub.Status.InstallPlanRef != nil || sub.Status.Install != nil { - return o.checkMissingInstallPlan(sub, ips) + return sub, false, nil } logger.Debug("checking for existing installplan") @@ -2046,20 +2020,6 @@ func (o *Operator) listInstallPlans(namespace string) (ips []*v1alpha1.InstallPl return } -func (o *Operator) listInstallPlansMap(namespace string) (ips map[string]struct{}, err error) { - list, err := o.client.OperatorsV1alpha1().InstallPlans(namespace).List(context.TODO(), metav1.ListOptions{}) - if err != nil { - return - } - - ips = make(map[string]struct{}) - for i := range list.Items { - ips[list.Items[i].GetName()] = struct{}{} - } - - return -} - // competingCRDOwnersExist returns true if there exists a CSV that owns at least one of the given CSVs owned CRDs (that's not the given CSV) func competingCRDOwnersExist(namespace string, csv *v1alpha1.ClusterServiceVersion, existingOwners map[string][]string) (bool, error) { // Attempt to find a pre-existing owner in the namespace for any owned crd diff --git a/pkg/controller/operators/catalog/subscription/reconciler_test.go b/pkg/controller/operators/catalog/subscription/reconciler_test.go index 285e51c7d5..54ed929c51 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/pkg/controller/operators/catalog/subscription/state.go b/pkg/controller/operators/catalog/subscription/state.go index 5717b24e63..f5da96614a 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{ diff --git a/test/e2e/subscription_e2e_test.go b/test/e2e/subscription_e2e_test.go index df4cac4c05..b1b30eed6c 100644 --- a/test/e2e/subscription_e2e_test.go +++ b/test/e2e/subscription_e2e_test.go @@ -243,7 +243,12 @@ var _ = Describe("Subscription", func() { require.Equal(GinkgoT(), v1alpha1.ApprovalManual, newInstallPlan.Spec.Approval) require.Equal(GinkgoT(), v1alpha1.InstallPlanPhaseRequiresApproval, newInstallPlan.Status.Phase) - newInstallPlan.Spec.Approved = true + // Set the InstallPlan's approved to True + Eventually(Apply(newInstallPlan, func(p *v1alpha1.InstallPlan) error { + p.Spec.Approved = true + return nil + })).Should(Succeed()) + _, err = crc.OperatorsV1alpha1().InstallPlans(testNamespace).Update(context.Background(), newInstallPlan, metav1.UpdateOptions{}) require.NoError(GinkgoT(), err)