Bug 1875247: Add package check for the channel head querying #1748

jianzhangbjz · 2020-09-03T09:08:04Z

Description of the change:

OLM supports multi packages in a channel, so OLM should check the head bundle based on the package. Fix bug: https://bugzilla.redhat.com/show_bug.cgi?id=1875247
Any comments are welcome! Thanks!

Motivation for the change:
Supports multi packages for the channel head querying

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

openshift-ci-robot · 2020-09-03T09:08:13Z

Hi @jianzhangbjz. Thanks for your PR.

I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

jianzhangbjz · 2020-09-03T09:09:09Z

/assign @kevinrizza

kevinrizza · 2020-09-03T15:16:21Z

/ok-to-test

kevinrizza · 2020-09-03T15:13:52Z

pkg/controller/registry/resolver/resolver.go

+		}
+
+	} else if len(headCandidates) < 1 {
+		return nil, fmt.Errorf("Not found the head for channel")


nit: can this error message be head of channel not found?

Sure, thanks!

kevinrizza · 2020-09-03T15:16:11Z

pkg/controller/registry/resolver/resolver.go

-		next, ok := replaces[current]
-		if !ok {
-			break
+	for _, v := range headCandidates {


Is this change just for the case where headCandidates == nil? Is it necessary since above we are just already returning if that's the case?

Not really, this change is for the situation of the len(headCandidates) > 1. If there are multi packages in a channel, there will multi headCandidates, return all of them here.

jianzhangbjz · 2020-09-09T02:21:51Z

/retest

of-deploy-bot · 2020-09-14T08:19:59Z

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

totaltestcount: 1
failedtestcount: 1
flaketestcount: 2
skippedtestcount: 4
flaketests:

classname: End-to-end
name: 'Install Plan with CSVs across multiple catalog sources'
counts: 3
details:

count: 3
error: |4-

/home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/installplan_e2e_test.go:45

	Error Trace:	installplan_e2e_test.go:122
	            				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:
	            	timed out waiting for the condition

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

meandurationsec: 251.25599466666665

classname: End-to-end
name: 'CRD Versions allows a CRD upgrade that doesn''t cause data loss'
counts: 1
details:

count: 1
error: |4-

/home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/crd_e2e_test.go:345
Unexpected error:
    <*errors.errorString | 0xc0001f02a0>: {
        s: "timed out waiting for the condition",
    }
    timed out waiting for the condition
occurred
/home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/crd_e2e_test.go:495

meandurationsec: 314.557376
skippedtests:

classname: End-to-end
name: 'Catalog image update'
counts: 1
details: []
meandurationsec: 0.025911
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: 15.113692
classname: End-to-end
name: 'Subscription updates existing install plan'
counts: 1
details: []
meandurationsec: 0.013553
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: 22.080489

jianzhangbjz · 2020-09-15T03:36:04Z

/retitle Bug 1875247: Add package check for the channel head querying

openshift-ci-robot · 2020-09-15T03:36:13Z

@jianzhangbjz: Re-titling can only be requested by trusted users, like repository collaborators.

In response to this:

/retitle Bug 1875247: Add package check for the channel head querying

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 · 2020-09-15T03:39:05Z

@jianzhangbjz: This pull request references Bugzilla bug 1875247, which is invalid:

expected the bug to target the "4.6.0" release, but it targets "4.7.0" 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 1875247: Add package check for the channel head querying

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.

benluddy

Thanks for opening this PR.

I don't think we should be checking at runtime for a condition that should never occur -- that is, the arguments to sortChannel should all be in the same package and channel.

Improving the errors that are returned when there are more or fewer than 1 head candidates would be great, though. Do you mind updating this change so that it only affects the errors that are returned?

benluddy · 2020-09-15T14:26:14Z

pkg/controller/registry/resolver/resolver.go

+			if v.sourceInfo.Package != p {
+				p = v.sourceInfo.Package
+			} else {
+				// if find multi heads in the same package, return error


This would only fire if two consecutive candidates were from the same package ([A A B] vs. [A B A]), which I don't think is the intention.

@benluddy Thanks for your review and suggestions! I agreed. Update the error logs.

ecordell · 2020-09-16T16:59:02Z

I think most of this was addressed with #1735, do we want to keep this open to address error messages / etc?

jianzhangbjz · 2020-09-18T10:31:23Z

@ecordell @benluddy Thanks! I removed other parts and updated the error logs output.

jianzhangbjz · 2020-09-18T10:35:24Z

/bugzilla refresh

openshift-ci-robot · 2020-09-18T10:35:27Z

@jianzhangbjz: This pull request references Bugzilla bug 1875247, which is invalid:

expected the bug to target the "4.6.0" release, but it targets "4.7.0" 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:

/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.

jianzhangbjz · 2020-09-18T10:35:58Z

/bugzilla refresh

openshift-ci-robot · 2020-09-18T10:36:06Z

@jianzhangbjz: This pull request references Bugzilla bug 1875247, 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 NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

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.

pkg/controller/registry/resolver/resolver.go

Co-authored-by: Ben Luddy <ben.luddy@gmail.com>

benluddy · 2020-09-23T15:52:21Z

/approve

benluddy · 2020-09-23T15:54:02Z

/bugzilla refresh

openshift-ci-robot · 2020-09-23T15:54:06Z

@benluddy: This pull request references Bugzilla bug 1875247, which is invalid:

expected the bug to target the "4.6.0" release, but it targets "4.7.0" 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:

/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.

benluddy · 2020-09-23T15:58:49Z

/retest

Looks good to me. Thanks for the contribution! This is just waiting for a second review and for the master branch to unfreeze (should happen in about a week).

openshift-ci-robot · 2020-09-23T19:12:20Z

@jianzhangbjz: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-olm	`3120953`	link	`/test e2e-aws-olm`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

benluddy · 2020-10-14T17:21:44Z

/bugzilla refresh

openshift-ci-robot · 2020-10-14T17:21:49Z

@benluddy: This pull request references Bugzilla bug 1875247, which is valid.

3 validation(s) were run on this bug

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

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.

benluddy · 2020-10-14T17:22:34Z

/retest

ecordell

/lgtm

openshift-ci-robot · 2020-10-14T18:39:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benluddy, ecordell, jianzhangbjz

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

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

openshift-bot · 2020-10-14T20:02:45Z

/retest

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

openshift-ci-robot · 2020-10-15T01:11:39Z

@jianzhangbjz: All pull requests linked via external trackers have merged:

operator-framework/operator-lifecycle-manager#1748

Bugzilla bug 1875247 has been moved to the MODIFIED state.

In response to this:

Bug 1875247: Add package check for the channel head querying

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 requested review from ecordell and njhale September 3, 2020 09:08

openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 3, 2020

openshift-ci-robot assigned kevinrizza Sep 3, 2020

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 3, 2020

kevinrizza reviewed Sep 3, 2020

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 13, 2020

jianzhangbjz force-pushed the channel branch from e7d29fe to 7f62712 Compare September 14, 2020 07:11

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 14, 2020

jianzhangbjz changed the title ~~[bz 1875247]Add package check for the channel head querying~~ Bug 1875247: Add package check for the channel head querying Sep 15, 2020

openshift-ci-robot added the bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. label Sep 15, 2020

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

benluddy requested changes Sep 15, 2020

View reviewed changes

update error logs for channel head querying

1429f13

jianzhangbjz force-pushed the channel branch from 7f62712 to 1429f13 Compare September 18, 2020 10:27

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 Sep 18, 2020

benluddy reviewed Sep 18, 2020

View reviewed changes

pkg/controller/registry/resolver/resolver.go Outdated Show resolved Hide resolved

Update pkg/controller/registry/resolver/resolver.go

3120953

Co-authored-by: Ben Luddy <ben.luddy@gmail.com>

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 23, 2020

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

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 14, 2020

ecordell approved these changes Oct 14, 2020

View reviewed changes

openshift-ci-robot assigned ecordell Oct 14, 2020

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

openshift-merge-robot merged commit dd94c7c into operator-framework:master Oct 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1875247: Add package check for the channel head querying #1748

Bug 1875247: Add package check for the channel head querying #1748

jianzhangbjz commented Sep 3, 2020

openshift-ci-robot commented Sep 3, 2020

jianzhangbjz commented Sep 3, 2020

kevinrizza commented Sep 3, 2020

kevinrizza Sep 3, 2020

jianzhangbjz Sep 4, 2020

kevinrizza Sep 3, 2020

jianzhangbjz Sep 4, 2020

jianzhangbjz commented Sep 9, 2020

of-deploy-bot commented Sep 14, 2020

jianzhangbjz commented Sep 15, 2020

openshift-ci-robot commented Sep 15, 2020

openshift-ci-robot commented Sep 15, 2020

benluddy left a comment

benluddy Sep 15, 2020

jianzhangbjz Sep 18, 2020

ecordell commented Sep 16, 2020

jianzhangbjz commented Sep 18, 2020

jianzhangbjz commented Sep 18, 2020

openshift-ci-robot commented Sep 18, 2020

jianzhangbjz commented Sep 18, 2020

openshift-ci-robot commented Sep 18, 2020

benluddy commented Sep 23, 2020

benluddy commented Sep 23, 2020

openshift-ci-robot commented Sep 23, 2020

benluddy commented Sep 23, 2020

openshift-ci-robot commented Sep 23, 2020

benluddy commented Oct 14, 2020

openshift-ci-robot commented Oct 14, 2020

benluddy commented Oct 14, 2020

ecordell left a comment

openshift-ci-robot commented Oct 14, 2020

openshift-bot commented Oct 14, 2020

openshift-ci-robot commented Oct 15, 2020

Bug 1875247: Add package check for the channel head querying #1748

Bug 1875247: Add package check for the channel head querying #1748

Conversation

jianzhangbjz commented Sep 3, 2020

openshift-ci-robot commented Sep 3, 2020

jianzhangbjz commented Sep 3, 2020

kevinrizza commented Sep 3, 2020

kevinrizza Sep 3, 2020

Choose a reason for hiding this comment

jianzhangbjz Sep 4, 2020

Choose a reason for hiding this comment

kevinrizza Sep 3, 2020

Choose a reason for hiding this comment

jianzhangbjz Sep 4, 2020

Choose a reason for hiding this comment

jianzhangbjz commented Sep 9, 2020

of-deploy-bot commented Sep 14, 2020

jianzhangbjz commented Sep 15, 2020

openshift-ci-robot commented Sep 15, 2020

openshift-ci-robot commented Sep 15, 2020

benluddy left a comment

Choose a reason for hiding this comment

benluddy Sep 15, 2020

Choose a reason for hiding this comment

jianzhangbjz Sep 18, 2020

Choose a reason for hiding this comment

ecordell commented Sep 16, 2020

jianzhangbjz commented Sep 18, 2020

jianzhangbjz commented Sep 18, 2020

openshift-ci-robot commented Sep 18, 2020

jianzhangbjz commented Sep 18, 2020

openshift-ci-robot commented Sep 18, 2020

benluddy commented Sep 23, 2020

benluddy commented Sep 23, 2020

openshift-ci-robot commented Sep 23, 2020

benluddy commented Sep 23, 2020

openshift-ci-robot commented Sep 23, 2020

benluddy commented Oct 14, 2020

openshift-ci-robot commented Oct 14, 2020

benluddy commented Oct 14, 2020

ecordell left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Oct 14, 2020

openshift-bot commented Oct 14, 2020

openshift-ci-robot commented Oct 15, 2020