Bug 1763749: Prioritize APIs from same CatSrc #1091

awgreene · 2019-10-28T14:37:48Z

Problem: When OLM installs an operator that requires dependencies that
are provided via multiple operators in different CatalogSources, the API
is pulled from any of the CatalogSources that provide the API.

Solution: This commit introduces a change so that OLM will generate a
list of operators that depend on the API, randomly select one of their
sources, and prioritize checking in that CatalogSource for the API prior
to checking the remaining CatalogSource for the API.

This commit is different than the 4.3 fix because of a method signature in the e2e test.

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

Problem: When OLM installs an operator that requires dependencies that are provided via multiple operators in different CatalogSources, the API is pulled from any of the CatalogSources that provide the API. Solution: This commit introduces a change so that OLM will generate a list of operators that depend on the API, randomly select one of their sources, and prioritize checking in that CatalogSource for the API prior to checking the remaining CatalogSource for the API.

openshift-ci-robot · 2019-10-28T14:37:53Z

@awgreene: This pull request references Bugzilla bug 1763749, which is invalid:

expected dependent Bugzilla bug 1762769 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but it is ON_QA instead
expected dependent Bugzilla bug 1763750 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but it is NEW instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1763749: Prioritize APIs from same CatSrc

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.

njhale · 2019-10-28T22:49:47Z

/retest

njhale

This looks excellent wrt backporting the original fix! That being said, since I wasn't able to ask these questions on the original's PR, I've decided to pose them here:

njhale · 2019-10-29T03:18:21Z

pkg/controller/registry/resolver/evolver.go

+		// identify the initialSource
+		initialSource := CatalogKey{}
+		for _, operator := range e.gen.MissingAPIs()[*api] {
+			initialSource = operator.SourceInfo().Catalog


Correct me if I'm wrong, but doesn't this break down when there exists two or more concurrent Subscriptions that require the same API, but target different sources that contain operators providing that API?

You are correct that the current approach can introduce some randomness when the following conditions are met:

Operator A defines dependencies on APIs provided byOperator B and Operator C

Operator B and Operator C come from different CatalogSources

Both Operator B and Operator C define dependencies on the same API provided by Operator D.

In this situation - the existing solution does not guarantee that a specific CatalogSource will be prioritized consistently.

This is a known issue with the short-term solution and is expected to be addressed in a future solution. We are comfortable with supporting the existing approach due to the fact that this "bug" only hits a small number of use cases.

njhale · 2019-10-29T03:19:42Z

pkg/controller/registry/resolver/querier_test.go

 			},
 			out: out{
 				bundle: nil,
 				key:    nil,
 				err:    fmt.Errorf("group/version/kind (plural) not provided by a package in any CatalogSource"),
 			},
 		},
+		{


nit: It would be nice to add descriptive names to each test case.

njhale · 2019-10-29T03:23:01Z

pkg/controller/registry/resolver/querier_test.go

+				key:    &CatalogKey{Name: "test2", Namespace: "ns"},
+				err:    nil,
+			},
+		},


I think we should have a test to ensure that the correct bundle is returned when more than one source containing a provider is given along with the preferred source key.

This is covered in the E2E testcase. I can create a Unit test as well.

It's probably best to add it to master.

njhale · 2019-10-29T03:29:41Z

test/e2e/subscription_e2e_test.go

+
+	// Create a subscription that has a dependency
+	subscriptionName := genName("podconfig-sub-")
+	cleanupSubscription := createSubscriptionForCatalogWithSpec(t, crClient, testNamespace, subscriptionName, subSpec)


Could we add concurrent Subscriptions pointing to each catalog to test the aforementioned "break down"?

ecordell · 2019-10-31T12:14:19Z

/retest

ecordell · 2019-10-31T18:03:54Z

/lgtm
/approve

we need this in the next 4.2 and 4.1, comments can be addressed in a followup in master

openshift-ci-robot · 2019-10-31T18:04:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene, ecordell

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

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

Bowenislandsong · 2019-10-31T18:21:53Z

/bugzilla refresh

openshift-ci-robot · 2019-10-31T18:21:59Z

@Bowenislandsong: This pull request references Bugzilla bug 1763749, 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.

In response to this:

/bugzilla refresh

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.

crawford · 2019-11-04T16:54:43Z

AWS throttled our API requests.

/retest

ecordell · 2019-11-04T22:23:47Z

/retest

crawford · 2019-11-04T22:41:11Z

error simulating policy: SerializationError: failed to decode query XML error response\n\tstatus code: 500, request id: \ncaused by: EOF

Cool.

/retest

openshift-ci-robot · 2019-11-05T01:07:21Z

@awgreene: All pull requests linked via external trackers have merged. Bugzilla bug 1763749 has been moved to the MODIFIED state.

In response to this:

Bug 1763749: Prioritize APIs from same CatSrc

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.

awgreene · 2019-11-05T10:44:17Z

/cherry-pick release-4.1

openshift-cherrypick-robot · 2019-11-05T10:44:25Z

@awgreene: #1091 failed to apply on top of branch "release-4.1":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	test/e2e/subscription_e2e_test.go
A	test/e2e/user_defined_sa_test.go
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): test/e2e/user_defined_sa_test.go deleted in HEAD and modified in bug(dependencies) Prioritize APIs from same CatSrc. Version bug(dependencies) Prioritize APIs from same CatSrc of test/e2e/user_defined_sa_test.go left in tree.
Auto-merging test/e2e/subscription_e2e_test.go
CONFLICT (content): Merge conflict in test/e2e/subscription_e2e_test.go
Patch failed at 0001 bug(dependencies) Prioritize APIs from same CatSrc

In response to this:

/cherry-pick release-4.1

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.

openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Oct 28, 2019

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 28, 2019

openshift-ci-robot requested review from alecmerdler and ecordell October 28, 2019 14:38

njhale reviewed Oct 29, 2019

View reviewed changes

openshift-ci-robot assigned ecordell Oct 31, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 31, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 31, 2019

openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Oct 31, 2019

crawford added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Nov 1, 2019

openshift-merge-robot merged commit b0c1709 into operator-framework:release-4.2 Nov 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1763749: Prioritize APIs from same CatSrc #1091

Bug 1763749: Prioritize APIs from same CatSrc #1091

awgreene commented Oct 28, 2019

openshift-ci-robot commented Oct 28, 2019

njhale commented Oct 28, 2019

njhale left a comment

njhale Oct 29, 2019

awgreene Oct 29, 2019

njhale Oct 29, 2019

awgreene Oct 29, 2019

njhale Oct 29, 2019

awgreene Oct 29, 2019

njhale Oct 29, 2019

njhale Oct 29, 2019

ecordell commented Oct 31, 2019

ecordell commented Oct 31, 2019 •

edited

Loading

openshift-ci-robot commented Oct 31, 2019

Bowenislandsong commented Oct 31, 2019

openshift-ci-robot commented Oct 31, 2019

crawford commented Nov 4, 2019

ecordell commented Nov 4, 2019

crawford commented Nov 4, 2019

openshift-ci-robot commented Nov 5, 2019

awgreene commented Nov 5, 2019

openshift-cherrypick-robot commented Nov 5, 2019

Bug 1763749: Prioritize APIs from same CatSrc #1091

Bug 1763749: Prioritize APIs from same CatSrc #1091

Conversation

awgreene commented Oct 28, 2019

openshift-ci-robot commented Oct 28, 2019

njhale commented Oct 28, 2019

njhale 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

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

ecordell commented Oct 31, 2019

ecordell commented Oct 31, 2019 • edited Loading

openshift-ci-robot commented Oct 31, 2019

Bowenislandsong commented Oct 31, 2019

openshift-ci-robot commented Oct 31, 2019

crawford commented Nov 4, 2019

ecordell commented Nov 4, 2019

crawford commented Nov 4, 2019

openshift-ci-robot commented Nov 5, 2019

awgreene commented Nov 5, 2019

openshift-cherrypick-robot commented Nov 5, 2019

ecordell commented Oct 31, 2019 •

edited

Loading