fix(sub): Only update subscriptions that have changes in status (#2399)

Currently, when resolution happens, every subscription will get
updated regardless if there are any changes applied or not. This
resolves into unnecessary API update calls.

This commit will filter out subscriptions that don't get changed
and only changed ones get updated.

Note: Remove an additional update API call from one of subscription
sync methods as well.

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

Author: dinhxuanvu

pkg/controller/operators/catalog/operator.go

@@ -962,31 +962,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")
@@ -1017,17 +1020,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
 }
 
@@ -1084,12 +1106,7 @@ 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 SourceQuerier) (*v1alpha1.Subscription, bool, error) {
@@ -1225,23 +1242,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) {

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: "",
-							},
-						},
 					},
 				},
 			},

test/e2e/subscription_e2e_test.go

@@ -2169,14 +2169,14 @@ var _ = Describe("Subscription", func() {
 
 				updateInternalCatalog(GinkgoT(), c, crc, catSrcName, generatedNamespace.GetName(), []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() {
+			It("the ResolutionFailed condition previously set in its status that indicated the resolution error is cleared off", func() {
 				Eventually(func() (corev1.ConditionStatus, error) {
 					sub, err := crc.OperatorsV1alpha1().Subscriptions(generatedNamespace.GetName()).Get(context.Background(), subName, metav1.GetOptions{})
 					if err != nil {
-						return corev1.ConditionUnknown, err
+						return corev1.ConditionFalse, err
 					}
 					return sub.Status.GetCondition(operatorsv1alpha1.SubscriptionResolutionFailed).Status, nil
-				}).Should(Equal(corev1.ConditionFalse))
+				}).Should(Equal(corev1.ConditionUnknown))
 			})
 		})
 	})