feat(sub): Indicate resolution conflicts on Subscription statuses #2269

anik120 · 2021-07-19T21:25:41Z

Description of the change:

Resolution considers everything in a given namespace as a unit, so conflicts aren't
generated on a per-Subscription basis. However, as the main entry point to resolution,
users first look at the Subscription to understand resolution failures. The set of conflicts
today is written to Events with type ResolutionFailed. This PR projects resolution errors onto
all subscriptions on the namespace for any resolution failure.

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

anik120 · 2021-07-19T21:27:30Z

/hold

Made changes to the subscription api inside the vendor folder for reviews. PR for api change is here: operator-framework/api#138

will unhold once this and the api repo PR is lgtmed and changes from api is vendored in here.

anik120 · 2021-07-19T21:29:13Z

pkg/controller/operators/catalog/operator.go

 		return err
+	} else {


Question: This is under the assumption that if there is a terminal resolution error, user intervention can resolve the error and get the subscriptions in the objects back to a good state. Is that a correct assumption?

cluster admins can always nuke everything and recreate after the fact. there are also (rather gnarly) workarounds to pop an InstallPlan back into a non-terminal state for a retry (e.g. manually patch InstallPlan status).

anik120 · 2021-07-21T15:03:45Z

/hold cancel

njhale

Looking great so far.

pkg/controller/operators/catalog/operator.go

test/e2e/subscription_e2e_test.go

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

njhale

Nice!

My only real concern is in the lack of unit tests.

anik120 · 2021-07-21T19:04:34Z

@njhale there are tests here that tests that the method returns the correct value/error when resolver error occurs.
Example the NotSatisfiableError test checks that the sovler.NotSatisfiable error is returned when a constraint is not satisfyiable. And there's another unit test to test any resolver error that is not a constraint not satisfyiable error. So we have unit tests that covers all scenarios for resolver error during namespace sync, this PR just bubbles the errors up to the subscriptions, which needed the e2e test to test

njhale · 2021-07-21T20:03:31Z

@njhale there are tests here that tests that the method returns the correct value/error when resolver error occurs.
Example the NotSatisfiableError test checks that the sovler.NotSatisfiable error is returned when a constraint is not satisfyiable. And there's another unit test to test any resolver error that is not a constraint not satisfyiable error. So we have unit tests that covers all scenarios for resolver error during namespace sync, this PR just bubbles the errors up to the subscriptions, which needed the e2e test to test

hmm, I was thinking of the class of test that uses a mock client and tests a wider range of I/O (i.e. not just checking the error) -- but since the e2e test is so similar, I don't think it's worth holding so close to freeze.

/approve

openshift-ci · 2021-07-21T20:03:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anik120, njhale

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~~ [njhale]

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

kevinrizza · 2021-07-22T13:04:27Z

pkg/controller/operators/catalog/operator.go

@@ -909,9 +909,22 @@ 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, "Constraint not satisfyable", err.Error(), true)


nit: spelling error. this should say Constraint not satisfiable

kevinrizza · 2021-07-22T13:05:47Z

minus that one nit, this looks good to me

kevinrizza · 2021-07-22T13:09:00Z

/lgtm

benluddy · 2021-07-22T13:50:09Z

pkg/controller/operators/catalog/operator.go

@@ -909,9 +909,22 @@ 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, "Constraint not satisfiable", err.Error(), true)


Isn't Reason usually intended to be machine-readable?

anik120 · 2021-07-22T13:57:20Z

/hold

Resolution considers everything in a given namespace as a unit, so conflicts aren't generated on a per-Subscription basis. However, as the main entry point to resolution, users first look at the Subscription to understand resolution failures. The set of conflicts today is written to Events with type ResolutionFailed. This PR projects resolution errors onto all subscriptions on the namespace for any resolution failure. Signed-off-by: Anik Bhattacharjee <[email protected]>

anik120 · 2021-07-22T14:09:05Z

/hold cancel

benluddy · 2021-07-22T14:53:30Z

/lgtm

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.

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

* fix(sub): Reset ResolutionFailed cond when error is resolved In #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]> * refractor updateStatusConditions to split calls to api server into it's own task Signed-off-by: Anik Bhattacharjee <[email protected]> * Reduce number of times subscription statues are updated. The statuses of the subscriptions are updated multiple number of times while syncing the resolving namespace. This commit switches to preserving the state of the subscription statues instead, and only updating the statuses on cluster only when it's neccessary. Signed-off-by: Anik Bhattacharjee <[email protected]> * remove unnecessary continue from loop Signed-off-by: Anik Bhattacharjee <[email protected]> * pass reference of sub to goroutine Signed-off-by: Anik Bhattacharjee <[email protected]>

openshift-ci bot requested review from ankitathomas and benluddy July 19, 2021 21:25

anik120 force-pushed the sub-resolution-error branch from 8ed8f6a to 66c9499 Compare July 19, 2021 21:26

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

anik120 commented Jul 19, 2021

View reviewed changes

anik120 force-pushed the sub-resolution-error branch from 66c9499 to bc1d243 Compare July 21, 2021 15:02

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

njhale reviewed Jul 21, 2021

View reviewed changes

anik120 force-pushed the sub-resolution-error branch 2 times, most recently from 04ceee0 to ef12368 Compare July 21, 2021 15:52

vendor of/api v0.10.2

ac82fec

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

anik120 force-pushed the sub-resolution-error branch from ef12368 to 7447829 Compare July 21, 2021 16:02

njhale reviewed Jul 21, 2021

View reviewed changes

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

kevinrizza reviewed Jul 22, 2021

View reviewed changes

anik120 force-pushed the sub-resolution-error branch from 7447829 to 01054fd Compare July 22, 2021 13:07

openshift-ci bot assigned kevinrizza Jul 22, 2021

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

benluddy reviewed Jul 22, 2021

View reviewed changes

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

anik120 force-pushed the sub-resolution-error branch from 01054fd to 4a2c5c0 Compare July 22, 2021 14:08

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 22, 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 Jul 22, 2021

openshift-ci bot assigned benluddy Jul 22, 2021

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

openshift-merge-robot merged commit 3223fce into operator-framework:master Jul 22, 2021

anik120 mentioned this pull request Jul 28, 2021

fix(sub): Reset ResolutionFailed cond when error is resolved #2296

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sub): Indicate resolution conflicts on Subscription statuses #2269

feat(sub): Indicate resolution conflicts on Subscription statuses #2269

anik120 commented Jul 19, 2021

anik120 commented Jul 19, 2021 •

edited

Loading

anik120 Jul 19, 2021

njhale Jul 21, 2021

anik120 commented Jul 21, 2021

njhale left a comment

njhale left a comment

anik120 commented Jul 21, 2021

njhale commented Jul 21, 2021

openshift-ci bot commented Jul 21, 2021

kevinrizza Jul 22, 2021

kevinrizza commented Jul 22, 2021

kevinrizza commented Jul 22, 2021

benluddy Jul 22, 2021

kevinrizza Jul 22, 2021

anik120 commented Jul 22, 2021

anik120 commented Jul 22, 2021

benluddy commented Jul 22, 2021

feat(sub): Indicate resolution conflicts on Subscription statuses #2269

feat(sub): Indicate resolution conflicts on Subscription statuses #2269

Conversation

anik120 commented Jul 19, 2021

anik120 commented Jul 19, 2021 • edited Loading

anik120 Jul 19, 2021

Choose a reason for hiding this comment

njhale Jul 21, 2021

Choose a reason for hiding this comment

anik120 commented Jul 21, 2021

njhale left a comment

Choose a reason for hiding this comment

njhale left a comment

Choose a reason for hiding this comment

anik120 commented Jul 21, 2021

njhale commented Jul 21, 2021

openshift-ci bot commented Jul 21, 2021

kevinrizza Jul 22, 2021

Choose a reason for hiding this comment

kevinrizza commented Jul 22, 2021

kevinrizza commented Jul 22, 2021

benluddy Jul 22, 2021

Choose a reason for hiding this comment

kevinrizza Jul 22, 2021

Choose a reason for hiding this comment

anik120 commented Jul 22, 2021

anik120 commented Jul 22, 2021

benluddy commented Jul 22, 2021

anik120 commented Jul 19, 2021 •

edited

Loading