Bug 2024048: Remove outdated subscription update logic to improve resolution delay

staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go

@@ -961,31 +961,34 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error {
 		// not-satisfiable error
 		if _, ok := err.(solver.NotSatisfiable); ok {
 			logger.WithError(err).Debug("resolution failed")
-			subs = o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "ConstraintsNotSatisfiable", err.Error(), true)
-			_, updateErr := o.updateSubscriptionStatuses(subs)
+			_, updateErr := o.updateSubscriptionStatuses(
+				o.setSubsCond(subs, v1alpha1.SubscriptionCondition{
+					Type:    v1alpha1.SubscriptionResolutionFailed,
+					Reason:  "ConstraintsNotSatisfiable",
+					Message: err.Error(),
+					Status:  corev1.ConditionTrue,
+				}))
 			if updateErr != nil {
 				logger.WithError(updateErr).Debug("failed to update subs conditions")
 				return updateErr
 			}
 			return nil
 		}
-		subs = o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "ErrorPreventedResolution", err.Error(), true)
-		_, updateErr := o.updateSubscriptionStatuses(subs)
+
+		_, updateErr := o.updateSubscriptionStatuses(
+			o.setSubsCond(subs, v1alpha1.SubscriptionCondition{
+				Type:    v1alpha1.SubscriptionResolutionFailed,
+				Reason:  "ErrorPreventedResolution",
+				Message: err.Error(),
+				Status:  corev1.ConditionTrue,
+			}))
 		if updateErr != nil {
 			logger.WithError(updateErr).Debug("failed to update subs conditions")
 			return updateErr
 		}
 		return err
 	}
 
-	defer func() {
-		subs = o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "", "", false)
-		_, updateErr := o.updateSubscriptionStatuses(subs)
-		if updateErr != nil {
-			logger.WithError(updateErr).Warn("failed to update subscription conditions")
-		}
-	}()
-
 	// create installplan if anything updated
 	if len(updatedSubs) > 0 {
 		logger.Debug("resolution caused subscription changes, creating installplan")
@@ -1016,17 +1019,36 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error {
 			return err
 		}
 		updatedSubs = o.setIPReference(updatedSubs, maxGeneration+1, installPlanReference)
-		for _, updatedSub := range updatedSubs {
-			for i, sub := range subs {
-				if sub.Name == updatedSub.Name && sub.Namespace == updatedSub.Namespace {
-					subs[i] = updatedSub
-				}
-			}
-		}
 	} else {
 		logger.Debugf("no subscriptions were updated")
 	}
 
+	// Remove resolutionfailed condition from subscriptions
+	subs = o.removeSubsCond(subs, v1alpha1.SubscriptionResolutionFailed)
+	newSub := true
+	for _, updatedSub := range updatedSubs {
+		updatedSub.Status.RemoveConditions(v1alpha1.SubscriptionResolutionFailed)
+		for i, sub := range subs {
+			if sub.Name == updatedSub.Name && sub.Namespace == updatedSub.Namespace {
+				subs[i] = updatedSub
+				newSub = false
+				break
+			}
+		}
+		if newSub {
+			subs = append(subs, updatedSub)
+			continue
+		}
+		newSub = true
+	}
+
+	// Update subscriptions with all changes so far
+	_, updateErr := o.updateSubscriptionStatuses(subs)
+	if updateErr != nil {
+		logger.WithError(updateErr).Warn("failed to update subscription conditions")
+		return updateErr
+	}
+
 	return nil
 }
 
@@ -1043,11 +1065,6 @@ func (o *Operator) syncSubscriptions(obj interface{}) error {
 }
 
 func (o *Operator) nothingToUpdate(logger *logrus.Entry, sub *v1alpha1.Subscription) bool {
-	// 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
@@ -1088,20 +1105,15 @@ func (o *Operator) ensureSubscriptionInstallPlanState(logger *logrus.Entry, sub
 	out.Status.CurrentCSV = out.Spec.StartingCSV
 	out.Status.LastUpdated = o.now()
 
-	updated, err := o.client.OperatorsV1alpha1().Subscriptions(sub.GetNamespace()).UpdateStatus(context.TODO(), out, metav1.UpdateOptions{})
-	if err != nil {
-		return nil, false, err
-	}
-
-	return updated, true, nil
+	return out, true, nil
 }
 
 func (o *Operator) ensureSubscriptionCSVState(logger *logrus.Entry, sub *v1alpha1.Subscription, querier resolver.SourceQuerier) (*v1alpha1.Subscription, bool, error) {
 	if sub.Status.CurrentCSV == "" {
 		return sub, false, nil
 	}
 
-	csv, err := o.client.OperatorsV1alpha1().ClusterServiceVersions(sub.GetNamespace()).Get(context.TODO(), sub.Status.CurrentCSV, metav1.GetOptions{})
+	_, err := o.client.OperatorsV1alpha1().ClusterServiceVersions(sub.GetNamespace()).Get(context.TODO(), sub.Status.CurrentCSV, metav1.GetOptions{})
 	out := sub.DeepCopy()
 	if err != nil {
 		logger.WithError(err).WithField("currentCSV", sub.Status.CurrentCSV).Debug("error fetching csv listed in subscription status")
@@ -1111,14 +1123,7 @@ func (o *Operator) ensureSubscriptionCSVState(logger *logrus.Entry, sub *v1alpha
 		if err := querier.Queryable(); err != nil {
 			return nil, false, err
 		}
-		b, _, _ := querier.FindReplacement(&csv.Spec.Version.Version, sub.Status.CurrentCSV, sub.Spec.Package, sub.Spec.Channel, registry.CatalogKey{Name: sub.Spec.CatalogSource, Namespace: sub.Spec.CatalogSourceNamespace})
-		if b != nil {
-			o.logger.Tracef("replacement %s bundle found for current bundle %s", b.CsvName, sub.Status.CurrentCSV)
-			out.Status.State = v1alpha1.SubscriptionStateUpgradeAvailable
-		} else {
-			out.Status.State = v1alpha1.SubscriptionStateAtLatest
-		}
-
+		out.Status.State = v1alpha1.SubscriptionStateAtLatest
 		out.Status.InstalledCSV = sub.Status.CurrentCSV
 	}
 
@@ -1236,23 +1241,45 @@ 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) []*v1alpha1.Subscription {
+// setSubsCond will set the condition to the subscription if it doesn't already
+// exist or if it is different
+// Only return the list of updated subscriptions
+func (o *Operator) setSubsCond(subs []*v1alpha1.Subscription, cond v1alpha1.SubscriptionCondition) []*v1alpha1.Subscription {
 	var (
 		lastUpdated = o.now()
+		subList     []*v1alpha1.Subscription
 	)
+
 	for _, sub := range subs {
+		subCond := sub.Status.GetCondition(cond.Type)
+		if subCond.Equals(cond) {
+			continue
+		}
 		sub.Status.LastUpdated = lastUpdated
+		sub.Status.SetCondition(cond)
+		subList = append(subList, sub)
+	}
+	return subList
+}
+
+// removeSubsCond will remove the condition to the subscription if it exists
+// Only return the list of updated subscriptions
+func (o *Operator) removeSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.SubscriptionConditionType) []*v1alpha1.Subscription {
+	var (
+		lastUpdated = o.now()
+	)
+	var subList []*v1alpha1.Subscription
+	for _, sub := range subs {
 		cond := sub.Status.GetCondition(condType)
-		cond.Reason = reason
-		cond.Message = message
-		if setTrue {
-			cond.Status = corev1.ConditionTrue
-		} else {
-			cond.Status = corev1.ConditionFalse
+		// if status is ConditionUnknown, the condition doesn't exist. Just skip
+		if cond.Status == corev1.ConditionUnknown {
+			continue
 		}
-		sub.Status.SetCondition(cond)
+		sub.Status.LastUpdated = lastUpdated
+		sub.Status.RemoveConditions(condType)
+		subList = append(subList, sub)
 	}
-	return subs
+	return subList
 }
 
 func (o *Operator) updateSubscriptionStatuses(subs []*v1alpha1.Subscription) ([]*v1alpha1.Subscription, error) {

staging/operator-lifecycle-manager/pkg/controller/operators/catalog/subscriptions_test.go

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

staging/operator-lifecycle-manager/pkg/controller/registry/resolver/querier.go

@@ -31,6 +31,7 @@ type SourceQuerier interface {
 	FindProvider(api opregistry.APIKey, initialSource registry.CatalogKey, excludedPackages map[string]struct{}) (*api.Bundle, *registry.CatalogKey, error)
 	FindBundle(pkgName, channelName, bundleName string, initialSource registry.CatalogKey) (*api.Bundle, *registry.CatalogKey, error)
 	FindLatestBundle(pkgName, channelName string, initialSource registry.CatalogKey) (*api.Bundle, *registry.CatalogKey, error)
+	// Deprecated: This FindReplacement function will be deprecated soon
 	FindReplacement(currentVersion *semver.Version, bundleName, pkgName, channelName string, initialSource registry.CatalogKey) (*api.Bundle, *registry.CatalogKey, error)
 	Queryable() error
 }
@@ -123,6 +124,7 @@ func (q *NamespaceSourceQuerier) FindLatestBundle(pkgName, channelName string, i
 	return nil, nil, fmt.Errorf("%s/%s not found in any available CatalogSource", pkgName, channelName)
 }
 
+// Deprecated: This FindReplacement function will be deprecated soon
 func (q *NamespaceSourceQuerier) FindReplacement(currentVersion *semver.Version, bundleName, pkgName, channelName string, initialSource registry.CatalogKey) (*api.Bundle, *registry.CatalogKey, error) {
 	errs := []error{}