Repository: operator-framework/operator-lifecycle-manager
Pull request #2399 (2021-10-05 22:44:06)

fix(sub): Only update subscriptions that have changes in status
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.

Signed-off-by: Vu Dinh <vudinh@outlook.com>

<!--

Before making a PR, please read our contributing guidelines https://github.com/operator-framework/operator-lifecycle-manager/blob/master/CONTRIBUTING.md

Note: Make sure your branch is rebased to the latest upstream master.

-->

**Description of the change:**


**Motivation for the change:**

**Reviewer Checklist**
- [ ] Implementation matches the proposed design, or proposal is updated to match implementation
- [ ] Sufficient unit test coverage
- [ ] Sufficient end-to-end test coverage
- [ ] Docs updated or added to `/doc`
- [ ] Commit messages sensible and descriptive


<!--

Note: If this PR is fixing an issue make sure to add a note saying:
Closes #<ISSUE_NUMBER>

-->


openshift-ci[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/operator-framework/operator-lifecycle-manager/pull/2399#" title="Author self-approved">dinhxuanvu</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=operator-framework%2Foperator-lifecycle-manager).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/operator-framework/operator-lifecycle-manager/blob/master/OWNERS)~~ [dinhxuanvu]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
timflannagan: /lgtm
timflannagan: Let's tackling any refactoring outside of this PR as this is blocked e2e runs.

/hold cancel
dinhxuanvu: /retest
openshift-bot: /retest-required

Please review the [full test history](https://prow.ci.openshift.org/pr-history/?org=operator-framework&repo=operator-lifecycle-manager&pr=2399) for this PR and help us cut down flakes.

Patch:
diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go
index a66712d87..d152b399a 100644
--- a/pkg/controller/operators/catalog/operator.go
+++ b/pkg/controller/operators/catalog/operator.go
@@ -962,16 +962,27 @@ 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
@@ -979,14 +990,6 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error {
 		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) {


Test patch:
diff --git a/pkg/controller/operators/catalog/subscriptions_test.go b/pkg/controller/operators/catalog/subscriptions_test.go
index 4d35a8039..15e44269f 100644
--- a/pkg/controller/operators/catalog/subscriptions_test.go
+++ b/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: "",
-							},
-						},
 					},
 				},
 			},
diff --git a/test/e2e/subscription_e2e_test.go b/test/e2e/subscription_e2e_test.go
index 978fe2b57..3d4f16f58 100644
--- a/test/e2e/subscription_e2e_test.go
+++ b/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))
 			})
 		})
 	})