[release-4.5] Bug 1862407: fix(resolver): be smarter about the way downgrades are attempted #1689

ecordell · 2020-07-29T19:24:00Z

"downgrade" here refers to downgrading potential updates back to what
has already been installed.

this fixes issues where OLM would otherwise not have been able to
determine a safe set of operators to update to - the case where, after
tentative updates to the current generation, there are required apis
missing from the set.

the previous algorithm solves this by downgrading the updates that
require the missing apis, which has the potential to downgrade back
to the initial set of operators.

in the case where the api was provided by some operator in the previous
generation, it would be possible to not downgrade all requirers, but
rather downgrade the operator that previously provided the api.

this commit implements an exhaustive search of potential downgrades for
a generation, and uses one without missing required apis if one is found

if none is found, the old behavior takes place, and the set may still
contract back to what is currently installed.

the new resolver avoids this issue entirely, so to avoid complicating
the implementation a limit of 64 updated operators per generation has
been added.

the implementation uses a bitmask to select either the old or new
operator from the set. finding all possible combinations of old/new
versions is then a simple integer count from 0 to the max.

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 /docs
Commit messages sensible and descriptive

kevinrizza · 2020-07-29T19:42:27Z

pkg/controller/registry/resolver/evolver.go

@@ -153,7 +162,65 @@ func (e *NamespaceGenerationEvolver) queryForRequiredAPIs() error {
 	return nil
 }

+func (e *NamespaceGenerationEvolver) downgradeUpdates() error {
+	// no need to attempt downgrades
+	if len(e.gen.MissingAPIs()) == 0 {


Super nit: the caller for this function might be more obvious if this check happened outside the method itself.

this is just keeping in line with the rest of the object, which has a series of methods which may or may not affect the generation - adding the check in Evolve would make it read weirdly, IMO

pkg/controller/registry/resolver/evolver.go

openshift-ci-robot · 2020-07-29T20:01:09Z

@ecordell: This pull request references Bugzilla bug 1861690, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1861690: fix(resolver): be smarter about the way downgrades are attempted

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Bowenislandsong · 2020-07-29T20:35:01Z

This PR failed tests for 1 times with 1 individual failed tests and 4 skipped tests. A test is considered flaky if failed on multiple commits.

totaltestcount: 1
flaketestcount: 1
skippedtestcount: 4
flaketests:

classname: End-to-end
name: 'Subscription creation using existing CSV'
counts: 1
details:

count: 1
error: |4-

/home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/subscription_e2e_test.go:70

	Error Trace:	subscription_e2e_test.go:77
	            				runner.go:113
	            				runner.go:64
	            				it_node.go:26
	            				spec.go:215
	            				spec.go:138
	            				spec_runner.go:200
	            				spec_runner.go:170
	            				spec_runner.go:66
	            				suite.go:62
	            				ginkgo_dsl.go:226
	            				ginkgo_dsl.go:214
	            				e2e_test.go:54
	Error:      	Received unexpected error:
	            	E2E bug detected: configmaps "mock-ocs" already exists

/home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/vendor/github.com/stretchr/testify/require/require.go:1005

meandurationsec: 0.055855
skippedtests:

classname: End-to-end
name: 'Catalog image update'
counts: 1
details: []
meandurationsec: 0.163795
classname: End-to-end
name: 'Subscriptions create required objects from Catalogs Given a Namespace
when a CatalogSource is created with a bundle that contains prometheus objects
creating a subscription using the CatalogSource should have created the expected
prometheus objects'
counts: 1
details: []
meandurationsec: 2.183249
classname: End-to-end
name: 'Subscription updates existing install plan'
counts: 1
details: []
meandurationsec: 0.024478
classname: End-to-end
name: 'Subscriptions create required objects from Catalogs Given a Namespace
when a CatalogSource is created with a bundle that contains prometheus objects
creating a subscription using the CatalogSource should install the operator
successfully'
counts: 1
details: []
meandurationsec: 2.116935

dinhxuanvu · 2020-07-29T20:58:21Z

The PR looks good to me. Plus, I tested it against OCS and it works.
/lgtm

ecordell

/hold

Noticed a bug from refactoring - will fix in the morning

ecordell · 2020-07-30T10:34:48Z

/hold cancel

ecordell · 2020-07-30T11:28:59Z

/retest

ecordell · 2020-07-30T11:30:56Z

/hold

dinhxuanvu · 2020-07-30T15:40:31Z

/hold cancel

dinhxuanvu · 2020-07-30T15:41:54Z

/retest

openshift-bot · 2020-07-30T15:51:19Z

/retest