Using subscription change to drive the installplan recreation

Signed-off-by: Vu Dinh <[email protected]>

Author: dinhxuanvu

pkg/controller/operators/catalog/operator.go

@@ -21,7 +21,6 @@ import (
 	apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	"k8s.io/apiextensions-apiserver/pkg/apiserver/validation"
 	extinf "k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions"
-	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/labels"
@@ -832,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
@@ -848,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
@@ -941,30 +946,41 @@ func (o *Operator) syncSubscriptions(obj interface{}) error {
 }
 
 func (o *Operator) nothingToUpdate(logger *logrus.Entry, sub *v1alpha1.Subscription) bool {
-	if sub.Status.InstallPlanRef != nil && sub.Status.State == v1alpha1.SubscriptionStateUpgradePending {
-		if o.isInstallPlanMissing(sub) {
-			return false
-		}
-		logger.Debugf("skipping update: installplan already created")
-		return true
-	}
 	// Only sync if catalog has been updated since last sync time
 	if o.sourcesLastUpdate.Before(sub.Status.LastUpdated.Time) && sub.Status.State != v1alpha1.SubscriptionStateNone && sub.Status.State != v1alpha1.SubscriptionStateUpgradeAvailable {
 		logger.Debugf("skipping update: no new updates to catalog since last sync at %s", sub.Status.LastUpdated.String())
 		return true
 	}
+	if sub.Status.InstallPlanRef != nil && sub.Status.State == v1alpha1.SubscriptionStateUpgradePending {
+		logger.Debugf("skipping update: installplan already created")
+		return true
+	}
 	return false
 }
 
-// isInstallPlanMissing checks if the installplan is missing or not
-func (o *Operator) isInstallPlanMissing(sub *v1alpha1.Subscription) bool {
-	_, err := o.client.OperatorsV1alpha1().InstallPlans(sub.GetNamespace()).Get(context.TODO(), sub.Status.Install.Name, metav1.GetOptions{})
-	return k8serrors.IsNotFound(err)
+// 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) (*v1alpha1.Subscription, bool, error) {
-	if sub.Status.InstallPlanRef != 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")
@@ -2030,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

pkg/controller/registry/resolver/step_resolver.go

@@ -188,7 +188,7 @@ func (r *OperatorStepResolver) ResolveSteps(namespace string, _ SourceQuerier) (
 		}
 
 		// add steps for subscriptions for bundles that were added through resolution
-		if existingSubscription != nil && (existingSubscription.Status.CurrentCSV != op.Identifier() || r.requireInstallPlanRecreate(existingSubscription)) {
+		if existingSubscription != nil && existingSubscription.Status.CurrentCSV != op.Identifier() {
 			// update existing subscription status
 			existingSubscription.Status.CurrentCSV = op.Identifier()
 			updatedSubs = append(updatedSubs, existingSubscription)
@@ -200,18 +200,6 @@ func (r *OperatorStepResolver) ResolveSteps(namespace string, _ SourceQuerier) (
 	return steps, bundleLookups, updatedSubs, nil
 }
 
-// requireInstallPlanRecreate checks for installplan present while subscription
-// is in pending state. If installplan is missing, return true. Otherwise, false.
-func (r *OperatorStepResolver) requireInstallPlanRecreate(sub *v1alpha1.Subscription) bool {
-	if sub.Status.InstallPlanRef != nil && sub.Status.State == v1alpha1.SubscriptionStateUpgradePending {
-		_, err := r.client.OperatorsV1alpha1().InstallPlans(sub.GetNamespace()).Get(context.TODO(), sub.Status.Install.Name, metav1.GetOptions{})
-		if errors.IsNotFound(err) {
-			return true
-		}
-	}
-	return false
-}
-
 func (r *OperatorStepResolver) hasExistingCurrentCSV(sub *v1alpha1.Subscription) (bool, error) {
 	if sub.Status.CurrentCSV == "" {
 		return false, nil

test/e2e/subscription_e2e_test.go

@@ -194,7 +194,7 @@ var _ = Describe("Subscription", func() {
 	})
 
 	// If installPlanApproval is set to manual, the installplans created should be created with approval: manual
-	It("creation manual approval", func() {
+	FIt("creation manual approval", func() {
 
 		c := newKubeClient()
 		crc := newCRClient()
@@ -222,14 +222,17 @@ var _ = Describe("Subscription", func() {
 		require.NoError(GinkgoT(), err)
 
 		var ipName string
-		Eventually(func() (string, error) {
+		Eventually(func() bool {
 			fetched, err := crc.OperatorsV1alpha1().Subscriptions(testNamespace).Get(context.TODO(), "manual-subscription", metav1.GetOptions{})
 			if err != nil {
-				return "", err
+				return false
+			}
+			if fetched.Status.Install != nil {
+				ipName = fetched.Status.Install.Name
+				return fetched.Status.Install.Name != installPlan.Name
 			}
-			ipName = fetched.Status.Install.Name
-			return ipName, nil
-		}).ShouldNot(Equal(installPlan.Name))
+			return false
+		}, 5*time.Minute, 10*time.Second).Should(BeTrue())
 
 		// Fetch new installplan
 		newInstallPlan, err := fetchInstallPlan(GinkgoT(), crc, ipName, buildInstallPlanPhaseCheckFunc(v1alpha1.InstallPlanPhaseRequiresApproval))