fix(sub): Reset ResolutionFailed cond when error is resolved

In operator-framework#2269, a new condition was introduced for Subscription to indicate any
dependency resolution error in it's message. However, when the case of
the error was resolved, the condition status (true) and the message
was sticking around. This PR fixes the issue, and makes sure that the
condition status is set to false, and the message and reason of the
condition are cleared off.

Author: anik120

pkg/controller/operators/catalog/operator.go

@@ -935,22 +935,23 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error {
 		// not-satisfiable error
 		if _, ok := err.(solver.NotSatisfiable); ok {
 			logger.WithError(err).Debug("resolution failed")
-			updateErr := o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "ConstraintsNotSatisfiable", err.Error(), true)
+			_, updateErr := o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "ConstraintsNotSatisfiable", err.Error(), true)
 			if updateErr != nil {
 				logger.WithError(updateErr).Debug("failed to update subs conditions")
 			}
 			return nil
 		}
-		updateErr := o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "ErrorPreventedResolution", err.Error(), true)
+		_, updateErr := o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "ErrorPreventedResolution", err.Error(), true)
 		if updateErr != nil {
 			logger.WithError(updateErr).Debug("failed to update subs conditions")
 		}
 		return err
 	} else {
-		updateErr := o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "", "", false)
+		subs, updateErr := o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "", "", false)
 		if updateErr != nil {
 			logger.WithError(updateErr).Debug("failed to update subs conditions")
 		}
+		updatedSubs = subs
 	}
 
 	// create installplan if anything updated
@@ -1228,7 +1229,7 @@ func (o *Operator) createInstallPlan(namespace string, gen int, subs []*v1alpha1
 	return reference.GetReference(res)
 }
 
-func (o *Operator) setSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.SubscriptionConditionType, reason, message string, setTrue bool) error {
+func (o *Operator) setSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.SubscriptionConditionType, reason, message string, setTrue bool) ([]*v1alpha1.Subscription, error) {
 	var (
 		errs        []error
 		mu          sync.Mutex
@@ -1243,38 +1244,42 @@ func (o *Operator) setSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.
 		cond.Reason = reason
 		cond.Message = message
 		if setTrue {
+			if cond.Status == corev1.ConditionTrue {
+				continue
+			}
 			cond.Status = corev1.ConditionTrue
 		} else {
+			if cond.Status == corev1.ConditionFalse {
+				continue
+			}
 			cond.Status = corev1.ConditionFalse
 		}
 		sub.Status.SetCondition(cond)
 
 		wg.Add(1)
-		go func(s v1alpha1.Subscription) {
+		go func(sub v1alpha1.Subscription) {
 			defer wg.Done()
 
 			update := func() error {
+				mu.Lock()
+				defer mu.Unlock()
 				// Update the status of the latest revision
-				latest, err := o.client.OperatorsV1alpha1().Subscriptions(s.GetNamespace()).Get(context.TODO(), s.GetName(), getOpts)
+				latest, err := o.client.OperatorsV1alpha1().Subscriptions(sub.GetNamespace()).Get(context.TODO(), sub.GetName(), getOpts)
 				if err != nil {
 					return err
 				}
-
-				latest.Status = s.Status
+				latest.Status = sub.Status
 				_, err = o.client.OperatorsV1alpha1().Subscriptions(sub.Namespace).UpdateStatus(context.TODO(), latest, updateOpts)
-
 				return err
 			}
 			if err := retry.RetryOnConflict(retry.DefaultRetry, update); err != nil {
-				mu.Lock()
-				defer mu.Unlock()
 				errs = append(errs, err)
 			}
 		}(*sub)
 	}
 	wg.Wait()
 
-	return utilerrors.NewAggregate(errs)
+	return subs, utilerrors.NewAggregate(errs)
 }
 
 type UnpackedBundleReference struct {

test/e2e/subscription_e2e_test.go

@@ -2113,8 +2113,11 @@ var _ = Describe("Subscription", func() {
 			teardown   func()
 			cleanup    func()
 			packages   []registry.PackageManifest
-			subName    = genName("test-subscription")
-			catSrcName = genName("test-catalog")
+			crd        = newCRD(genName("foo-"))
+			csvA       operatorsv1alpha1.ClusterServiceVersion
+			csvB       operatorsv1alpha1.ClusterServiceVersion
+			subName    = genName("test-subscription-")
+			catSrcName = genName("test-catalog-")
 		)
 
 		BeforeEach(func() {
@@ -2130,10 +2133,9 @@ var _ = Describe("Subscription", func() {
 					DefaultChannelName: "alpha",
 				},
 			}
-			crd := newCRD(genName("foo"))
-			csv := newCSV("csvA", testNamespace, "", semver.MustParse("1.0.0"), nil, []apiextensions.CustomResourceDefinition{crd}, nil)
+			csvA = newCSV("csvA", testNamespace, "", semver.MustParse("1.0.0"), nil, []apiextensions.CustomResourceDefinition{crd}, nil)
 
-			_, teardown = createInternalCatalogSource(c, ctx.Ctx().OperatorClient(), catSrcName, testNamespace, packages, nil, []operatorsv1alpha1.ClusterServiceVersion{csv})
+			_, teardown = createInternalCatalogSource(c, ctx.Ctx().OperatorClient(), catSrcName, testNamespace, packages, nil, []operatorsv1alpha1.ClusterServiceVersion{csvA})
 
 			// Ensure that the catalog source is resolved before we create a subscription.
 			_, err := fetchCatalogSourceOnStatus(crc, catSrcName, testNamespace, catalogSourceRegistryPodSynced)
@@ -2157,6 +2159,31 @@ var _ = Describe("Subscription", func() {
 			}).Should(Equal(corev1.ConditionTrue))
 		})
 
+		When("the required API is made available", func() {
+			BeforeEach(func() {
+				newPkg := registry.PackageManifest{
+					PackageName: "PackageB",
+					Channels: []registry.PackageChannel{
+						{Name: "alpha", CurrentCSVName: "csvB"},
+					},
+					DefaultChannelName: "alpha",
+				}
+				packages = append(packages, newPkg)
+
+				csvB = newCSV("csvB", testNamespace, "", semver.MustParse("1.0.0"), []apiextensions.CustomResourceDefinition{crd}, nil, nil)
+
+				updateInternalCatalog(GinkgoT(), c, crc, catSrcName, testNamespace, []apiextensions.CustomResourceDefinition{crd}, []operatorsv1alpha1.ClusterServiceVersion{csvA, csvB}, packages)
+			})
+			It("the ResolutionFailed condition previously set in it's status that indicated the resolution error is cleared off", func() {
+				Eventually(func() (corev1.ConditionStatus, error) {
+					sub, err := crc.OperatorsV1alpha1().Subscriptions(testNamespace).Get(context.Background(), subName, metav1.GetOptions{})
+					if err != nil {
+						return corev1.ConditionUnknown, err
+					}
+					return sub.Status.GetCondition(operatorsv1alpha1.SubscriptionResolutionFailed).Status, nil
+				}).Should(Equal(corev1.ConditionFalse))
+			})
+		})
 	})
 
 	When("an unannotated ClusterServiceVersion exists with an associated Subscription", func() {