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

dinhxuanvu · 2021-10-05T22:44:06Z

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 [email protected]

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

openshift-ci · 2021-10-05T22:44:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dinhxuanvu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

njhale · 2021-10-06T12:25:07Z

/hold

Before we merge this, let's see if anything more obvious stands out by refactoring the resolution syncing.

Here's a snippet from a convo we had in Slack.

I've been refactoring here and it's clear that there are more interactions going on here than meets the eye. I think the intent was to always update after resolution to make sure the LastUpdated field is set. I believe this field was compared against the last CatalogSource update to determine resolution freshness; i.e. do we need to re-resolve.

(see #2400)

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]>

timflannagan · 2021-11-19T21:01:20Z

pkg/controller/operators/catalog/operator.go

+// 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 {


This is more of a general question/comment than anything blocking: at a glance, this method currently resembles a Setter but we're passing the reference to the list of subscriptions, and outputting a list of subscriptions. Any idea on whether we're actually double dipping here - modifying the input and outputting a new subscription list? I guess that might not matter a whole ton as it's contingent on how the call site is handling this logic, and it's possible my brain power is limited going into EOD.

This is just a "lazy" solution in term of not wanting to deal with removing element(s) from existing list by slicing the array. I can change this to a solution that will do the slicing so that the existing modified list will be returned instead of a new one.

timflannagan · 2021-11-19T21:02:23Z

pkg/controller/operators/catalog/operator.go

 		if updateErr != nil {
 			logger.WithError(updateErr).Debug("failed to update subs conditions")
 			return updateErr
 		}
 		return err
 	}

-	defer func() {


+1 to removing this defer case. I think this reads a lot more clearer, and being more explicit in this code path likely helps limit side effects.

timflannagan · 2021-11-19T21:07:20Z

pkg/controller/operators/catalog/operator.go

 	} else {
 		logger.Debugf("no subscriptions were updated")
 	}

+	// Remove resolutionfailed condition from subscriptions
+	subs = o.removeSubsCond(subs, v1alpha1.SubscriptionResolutionFailed)


Just a quick note: it feels like this function may be a candidate for refactoring in the future. I don't think we need to tackle that kind of change in this PR but I wanted to get any thoughts on whether you had that same though when poking at this locally?

I'm open for refactoring but what do you have in mind in term of changes that can be made to this function? It is pretty minimal as it is so I'm not sure what else we can do simplify it further.

Just copy-pasting the comment chain on the other PR: #2429 (comment).

I'd like to hold off on refactoring and keeping the scope of these changes small as they're affecting upstream/downstream e2e runs, and combing down hot looping behavior in some edge cases.

timflannagan · 2021-11-19T21:14:18Z

pkg/controller/operators/catalog/operator.go

+		if subCond.Equals(cond) {
+			continue
+		}


It looks like this Equals method does account for the edge case where the existing and new status conditions are equal, and avoids bumping the subCond.LastTransitionTime unnecessarily, avoiding another potential hotlooping scenario. Any value in adding a debug logging here, or is that just overkill?

We can add a log here to indicate the condition already exists. I think it can be helpful for debugging reason.

dinhxuanvu

Responded.

dinhxuanvu · 2021-11-19T22:59:54Z

pkg/controller/operators/catalog/operator.go

 	} else {
 		logger.Debugf("no subscriptions were updated")
 	}

+	// Remove resolutionfailed condition from subscriptions
+	subs = o.removeSubsCond(subs, v1alpha1.SubscriptionResolutionFailed)


I'm open for refactoring but what do you have in mind in term of changes that can be made to this function? It is pretty minimal as it is so I'm not sure what else we can do simplify it further.

dinhxuanvu · 2021-11-19T23:07:06Z

pkg/controller/operators/catalog/operator.go

+		if subCond.Equals(cond) {
+			continue
+		}


We can add a log here to indicate the condition already exists. I think it can be helpful for debugging reason.

dinhxuanvu · 2021-11-19T23:09:08Z

pkg/controller/operators/catalog/operator.go

+// 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 {


This is just a "lazy" solution in term of not wanting to deal with removing element(s) from existing list by slicing the array. I can change this to a solution that will do the slicing so that the existing modified list will be returned instead of a new one.

timflannagan · 2021-11-29T17:43:57Z

/lgtm

timflannagan · 2021-11-29T18:34:48Z

Let's tackling any refactoring outside of this PR as this is blocked e2e runs.

/hold cancel

dinhxuanvu · 2021-11-29T21:05:38Z

/retest

openshift-bot · 2021-11-29T21:16:20Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-11-29T22:04:18Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

dinhxuanvu requested a review from timflannagan October 5, 2021 22:44

openshift-ci bot requested review from exdx and gallettilance October 5, 2021 22:44

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 5, 2021

dinhxuanvu force-pushed the sub-cond branch 2 times, most recently from 4b3573c to e337f68 Compare October 6, 2021 01:17

dinhxuanvu requested review from anik120 and removed request for gallettilance and exdx October 6, 2021 01:18

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 6, 2021

dinhxuanvu force-pushed the sub-cond branch 6 times, most recently from 6af6678 to 62baea0 Compare October 21, 2021 00:06

dinhxuanvu force-pushed the sub-cond branch from 62baea0 to a0d9ab1 Compare November 19, 2021 20:25

timflannagan reviewed Nov 19, 2021

View reviewed changes

dinhxuanvu commented Nov 19, 2021

View reviewed changes

dinhxuanvu mentioned this pull request Nov 29, 2021

fix(sub): Prevent unnecessary subscription update calls on condition changes #2429

Closed

5 tasks

openshift-ci bot assigned timflannagan Nov 29, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 29, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 29, 2021

openshift-merge-robot merged commit e6cc305 into operator-framework:master Nov 29, 2021

timflannagan mentioned this pull request Dec 1, 2021

"OperatorGroup static provider" #2408

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dinhxuanvu commented Oct 5, 2021

openshift-ci bot commented Oct 5, 2021

njhale commented Oct 6, 2021

timflannagan Nov 19, 2021

dinhxuanvu Nov 19, 2021

timflannagan Nov 19, 2021

timflannagan Nov 19, 2021

dinhxuanvu Nov 19, 2021

timflannagan Nov 29, 2021

timflannagan Nov 29, 2021

timflannagan Nov 19, 2021 •

edited

Loading

dinhxuanvu Nov 19, 2021

dinhxuanvu left a comment

dinhxuanvu Nov 19, 2021

dinhxuanvu Nov 19, 2021

dinhxuanvu Nov 19, 2021

timflannagan commented Nov 29, 2021

timflannagan commented Nov 29, 2021

dinhxuanvu commented Nov 29, 2021

openshift-bot commented Nov 29, 2021

openshift-bot commented Nov 29, 2021

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

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

Conversation

dinhxuanvu commented Oct 5, 2021

openshift-ci bot commented Oct 5, 2021

njhale commented Oct 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timflannagan Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dinhxuanvu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timflannagan commented Nov 29, 2021

timflannagan commented Nov 29, 2021

dinhxuanvu commented Nov 29, 2021

openshift-bot commented Nov 29, 2021

openshift-bot commented Nov 29, 2021

timflannagan Nov 19, 2021 •

edited

Loading