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.

Signed-off-by: Anik Bhattacharjee <[email protected]>

Author: anik120

pkg/controller/operators/catalog/operator.go

@@ -976,19 +976,31 @@ 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(updatedSubs, v1alpha1.SubscriptionResolutionFailed, "", "", false)
+		if updateErr != nil {
+			logger.WithError(updateErr).Debug("failed to update subs conditions")
+		}
+		updatedSubs = subs
+
+		// Also clear the condition from the subscriptions there were not included in the list of updatedSubs
+		allSubs, err := o.listSubscriptions(namespace)
+		if err != nil {
+			logger.WithError(err).Debug("couldn't list subscriptions")
+			return err
+		}
+		_, updateErr = o.setSubsCond(allSubs, v1alpha1.SubscriptionResolutionFailed, "", "", false)
 		if updateErr != nil {
 			logger.WithError(updateErr).Debug("failed to update subs conditions")
 		}
@@ -1269,7 +1281,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
@@ -1284,26 +1296,31 @@ 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 {
 				// 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
-				_, err = o.client.OperatorsV1alpha1().Subscriptions(s.Namespace).UpdateStatus(context.TODO(), latest, updateOpts)
-
+				sub = *latest
+				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 {
@@ -1315,7 +1332,7 @@ func (o *Operator) setSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.
 	}
 	wg.Wait()
 
-	return utilerrors.NewAggregate(errs)
+	return subs, utilerrors.NewAggregate(errs)
 }
 
 type UnpackedBundleReference struct {

pkg/controller/operators/catalog/subscriptions_test.go

@@ -155,6 +155,14 @@ func TestSyncSubscriptions(t *testing.T) {
 						},
 						LastUpdated:           now,
 						InstallPlanGeneration: 1,
+						Conditions: []v1alpha1.SubscriptionCondition{
+							{
+								Type:    "ResolutionFailed",
+								Status:  corev1.ConditionFalse,
+								Reason:  "",
+								Message: "",
+							},
+						},
 					},
 				},
 			},
@@ -298,6 +306,14 @@ func TestSyncSubscriptions(t *testing.T) {
 						},
 						LastUpdated:           now,
 						InstallPlanGeneration: 1,
+						Conditions: []v1alpha1.SubscriptionCondition{
+							{
+								Type:    "ResolutionFailed",
+								Status:  corev1.ConditionFalse,
+								Reason:  "",
+								Message: "",
+							},
+						},
 					},
 				},
 			},
@@ -446,6 +462,14 @@ func TestSyncSubscriptions(t *testing.T) {
 						},
 						InstallPlanGeneration: 1,
 						LastUpdated:           now,
+						Conditions: []v1alpha1.SubscriptionCondition{
+							{
+								Type:    "ResolutionFailed",
+								Status:  corev1.ConditionFalse,
+								Reason:  "",
+								Message: "",
+							},
+						},
 					},
 				},
 			},
@@ -599,6 +623,14 @@ func TestSyncSubscriptions(t *testing.T) {
 						},
 						LastUpdated:           now,
 						InstallPlanGeneration: 1,
+						Conditions: []v1alpha1.SubscriptionCondition{
+							{
+								Type:    "ResolutionFailed",
+								Status:  corev1.ConditionFalse,
+								Reason:  "",
+								Message: "",
+							},
+						},
 					},
 				},
 			},
@@ -775,6 +807,14 @@ func TestSyncSubscriptions(t *testing.T) {
 						},
 						LastUpdated:           now,
 						InstallPlanGeneration: 1,
+						Conditions: []v1alpha1.SubscriptionCondition{
+							{
+								Type:    "ResolutionFailed",
+								Status:  corev1.ConditionFalse,
+								Reason:  "",
+								Message: "",
+							},
+						},
 					},
 				},
 			},
@@ -958,6 +998,14 @@ func TestSyncSubscriptions(t *testing.T) {
 						},
 						LastUpdated:           now,
 						InstallPlanGeneration: 2,
+						Conditions: []v1alpha1.SubscriptionCondition{
+							{
+								Type:    "ResolutionFailed",
+								Status:  corev1.ConditionFalse,
+								Reason:  "",
+								Message: "",
+							},
+						},
 					},
 				},
 			},

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() {