Bug 1779313: Enable multiple namespaces sync if catsrc is updated in global ns #1125

dinhxuanvu · 2019-11-14T07:28:54Z

Currently, if CatalogSource is updated and connection is Ready,
only the namespace where CatalogSource resides gets resynced. Now, a list
of namespaces that contain Subscriptions that use updated CatalogSource
will get resynced instead.

Signed-off-by: Vu Dinh vdinh@redhat.com

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

dinhxuanvu · 2019-11-14T14:48:37Z

/retest

awgreene · 2019-11-14T15:17:05Z

@dinhxuanvu
error: endpoint "https://registry.svc.ci.openshift.org" does not support v2 API

dinhxuanvu · 2019-11-14T15:26:42Z

/retest

njhale · 2019-11-14T18:36:48Z

@dinhxuanvu could you open a BZ for this?

dinhxuanvu · 2019-11-14T18:38:06Z

@njhale This PR is to address the issue on the existing bug that we were talking about during standup early on this week. I haven't tagged the bug on the title yet.

dinhxuanvu · 2019-11-14T19:56:31Z

/retest

openshift-ci-robot · 2019-11-15T03:37:42Z

@dinhxuanvu: This pull request references Bugzilla bug 1749031, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1749031: Enable multiple namespaces sync if catsrc is updated in global ns

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/lib/index/catalog.go

dinhxuanvu · 2019-11-20T03:32:26Z

/retest

openshift-ci-robot · 2019-11-21T18:11:56Z

@dinhxuanvu: This pull request references Bugzilla bug 1775323, 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:

Bug 1775323: Enable multiple namespaces sync if catsrc is updated in global ns

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-11-21T18:15:44Z

/bugzilla refresh

openshift-ci-robot · 2019-11-21T18:15:47Z

@njhale: This pull request references Bugzilla bug 1775323, which is valid.

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.

dinhxuanvu · 2019-11-22T08:29:31Z

/retest

dinhxuanvu · 2019-11-22T13:11:14Z

/retest

pkg/lib/index/catalog.go

pkg/controller/operators/catalog/operator.go

dinhxuanvu · 2019-11-26T16:43:41Z

/retest

Signed-off-by: Vu Dinh <vdinh@redhat.com>

dinhxuanvu · 2019-12-03T15:30:33Z

/retest

jpeeler · 2019-12-03T16:31:14Z

/hold
I think a 4.4 bug needs to be made for this.

dinhxuanvu · 2019-12-03T16:51:35Z

/retest

openshift-ci-robot · 2019-12-03T18:14:03Z

@dinhxuanvu: This pull request references Bugzilla bug 1779313, 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:

Bug 1779313: Enable multiple namespaces sync if catsrc is updated in global ns

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.

jpeeler · 2019-12-03T18:14:15Z

/hold cancel

jpeeler · 2019-12-03T18:18:33Z

/lgtm

openshift-ci-robot · 2019-12-03T18:18:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu, jpeeler

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

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

openshift-ci-robot · 2019-12-03T18:29:12Z

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

In response to this:

Bug 1779313: Enable multiple namespaces sync if catsrc is updated in global ns

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

Looks like @jpeeler beat me to the punch!

I mostly had nits. I'm leaving them here in case you're interested @dinhxuanvu.

njhale · 2019-12-03T18:27:29Z

pkg/lib/index/catalog.go

@@ -0,0 +1,53 @@
+package indexer


nit: although there are some exceptions, this probably shouldn't be in the lib folder since it contains OLM specific logic.

njhale · 2019-12-03T18:29:56Z

pkg/lib/index/catalog.go

+	}
+
+	if sub.Spec.CatalogSource != "" && sub.Spec.CatalogSourceNamespace != "" {
+		return []string{sub.Spec.CatalogSource + "/" + sub.Spec.CatalogSourceNamespace}, nil


nit: the common convention is namespace/name.

njhale · 2019-12-03T18:36:57Z

pkg/lib/index/catalog.go

+
+// CatalogSubscriberNamespaces returns the list of namespace (as a map with namespace as key)
+// which has Suscriptions(s) that subscribe(s) to a given CatalogSource (name/namespace)
+func CatalogSubscriberNamespaces(indexers map[string]cache.Indexer, name, namespace string) (map[string]struct{}, error) {


nit: the result map isn't being used as a map outside of this function -- seems kinda leaky

njhale · 2019-12-03T18:45:04Z

test/e2e/catalog_e2e_test.go

+	// Locally it is `olm`
+	namespaces, _ := c.KubernetesInterface().CoreV1().Namespaces().List(metav1.ListOptions{})
+	for _, ns := range namespaces.Items {
+		if ns.GetName() == "openshift-marketplace" {


nit: it would be nice to take this as a test argument so we can avoid making configuration assumptions in our test logic.

njhale · 2019-12-03T18:48:14Z

test/e2e/catalog_e2e_test.go

+		CatalogSourceNamespace: globalNS,
+		Package:                mainPackageName,
+		Channel:                stableChannel,
+		StartingCSV:            mainCSV.GetName(),


nit: no need to specify a StartingCSV or a manual approval strategy

njhale · 2019-12-03T18:49:49Z

test/e2e/catalog_e2e_test.go

+	installPlanName := subscription.Status.Install.Name
+	requiresApprovalChecker := buildInstallPlanPhaseCheckFunc(v1alpha1.InstallPlanPhaseRequiresApproval)
+	fetchedInstallPlan, err := fetchInstallPlan(t, crc, installPlanName, requiresApprovalChecker)
+	require.NoError(t, err)
+
+	fetchedInstallPlan.Spec.Approved = true
+	_, err = crc.OperatorsV1alpha1().InstallPlans(testNamespace).Update(fetchedInstallPlan)
+	require.NoError(t, err)


Since you only put the manifests for the first CSV into the catalog, there's no need to step through updates using a manual approval strategy.

dinhxuanvu · 2019-12-03T19:10:59Z

/cherry-pick release-4.3

openshift-cherrypick-robot · 2019-12-03T19:11:07Z

@dinhxuanvu: new pull request created: #1166

In response to this:

/cherry-pick release-4.3

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.

dinhxuanvu · 2019-12-04T17:54:26Z

/cherry-pick release-4.2

openshift-cherrypick-robot · 2019-12-04T17:54:30Z

@dinhxuanvu: failed to push cherry-picked changes in GitHub: pushing failed, output: "remote: Not Found\nfatal: repository 'https://openshift-cherrypick-robot:CENSORED@github.com/openshift-cherrypick-robot/operator-framework/operator-lifecycle-manager/' not found\n", error: exit status 128

In response to this:

/cherry-pick release-4.2

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 November 14, 2019 07:28

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

dinhxuanvu force-pushed the sync-improve branch from 4e9df29 to 6f17273 Compare November 14, 2019 15:21

dinhxuanvu force-pushed the sync-improve branch from 6f17273 to cfd087c Compare November 15, 2019 03:24

dinhxuanvu changed the title ~~Enable multiple namespaces sync if catsrc is updated in global ns~~ Bug 1749031: Enable multiple namespaces sync if catsrc is updated in global ns Nov 15, 2019

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

exdx reviewed Nov 18, 2019

View reviewed changes

pkg/lib/index/catalog.go Show resolved Hide resolved

dinhxuanvu force-pushed the sync-improve branch from cfd087c to 1502b75 Compare November 19, 2019 19:48

dinhxuanvu force-pushed the sync-improve branch from 9dd29fc to 693a312 Compare November 20, 2019 07:00

njhale changed the title ~~Bug 1749031: Enable multiple namespaces sync if catsrc is updated in global ns~~ Bug 1775323: Enable multiple namespaces sync if catsrc is updated in global ns Nov 21, 2019

dinhxuanvu force-pushed the sync-improve branch 4 times, most recently from 9d95057 to b42f7d7 Compare November 22, 2019 08:20

ecordell reviewed Nov 22, 2019

View reviewed changes

pkg/lib/index/catalog.go Outdated Show resolved Hide resolved

pkg/controller/operators/catalog/operator.go Show resolved Hide resolved

dinhxuanvu force-pushed the sync-improve branch from 9d9901c to 5bb767a Compare December 2, 2019 21:51

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 2, 2019

dinhxuanvu force-pushed the sync-improve branch 2 times, most recently from 5bb767a to 9be6708 Compare December 3, 2019 05:20

Change index func to return list of ns instead

45f0689

Signed-off-by: Vu Dinh <vdinh@redhat.com>

dinhxuanvu force-pushed the sync-improve branch from 9be6708 to 45f0689 Compare December 3, 2019 06:41

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 3, 2019

jpeeler changed the title ~~Bug 1775323: Enable multiple namespaces sync if catsrc is updated in global ns~~ Bug 1779313: Enable multiple namespaces sync if catsrc is updated in global ns Dec 3, 2019

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 3, 2019

openshift-ci-robot assigned jpeeler Dec 3, 2019

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

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

openshift-merge-robot merged commit 8d52709 into operator-framework:master Dec 3, 2019

njhale reviewed Dec 3, 2019

View reviewed changes

openshift-cherrypick-robot mentioned this pull request Dec 3, 2019

[release-4.3] Bug 1775323: Enable multiple namespaces sync if catsrc is updated in global ns #1166

Merged

This was referenced Dec 4, 2019

[release-4.2] Bug 1749031: Enable multiple namespaces sync if catsrc is updated in global ns #1169

Merged

[release-4.1] Bug 1775322: Enable multiple namespaces sync if catsrc is updated in global ns #1178

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1779313: Enable multiple namespaces sync if catsrc is updated in global ns #1125

Bug 1779313: Enable multiple namespaces sync if catsrc is updated in global ns #1125

dinhxuanvu commented Nov 14, 2019

dinhxuanvu commented Nov 14, 2019

awgreene commented Nov 14, 2019

dinhxuanvu commented Nov 14, 2019

njhale commented Nov 14, 2019

dinhxuanvu commented Nov 14, 2019

dinhxuanvu commented Nov 14, 2019

openshift-ci-robot commented Nov 15, 2019

dinhxuanvu commented Nov 20, 2019

openshift-ci-robot commented Nov 21, 2019

njhale commented Nov 21, 2019

openshift-ci-robot commented Nov 21, 2019

dinhxuanvu commented Nov 22, 2019

dinhxuanvu commented Nov 22, 2019

dinhxuanvu commented Nov 26, 2019

dinhxuanvu commented Dec 3, 2019

jpeeler commented Dec 3, 2019

dinhxuanvu commented Dec 3, 2019

openshift-ci-robot commented Dec 3, 2019

jpeeler commented Dec 3, 2019

jpeeler commented Dec 3, 2019

openshift-ci-robot commented Dec 3, 2019

openshift-ci-robot commented Dec 3, 2019

njhale left a comment

njhale Dec 3, 2019

njhale Dec 3, 2019

njhale Dec 3, 2019

njhale Dec 3, 2019

njhale Dec 3, 2019

njhale Dec 3, 2019

dinhxuanvu commented Dec 3, 2019

openshift-cherrypick-robot commented Dec 3, 2019

dinhxuanvu commented Dec 4, 2019

openshift-cherrypick-robot commented Dec 4, 2019

Bug 1779313: Enable multiple namespaces sync if catsrc is updated in global ns #1125

Bug 1779313: Enable multiple namespaces sync if catsrc is updated in global ns #1125

Conversation

dinhxuanvu commented Nov 14, 2019

dinhxuanvu commented Nov 14, 2019

awgreene commented Nov 14, 2019

dinhxuanvu commented Nov 14, 2019

njhale commented Nov 14, 2019

dinhxuanvu commented Nov 14, 2019

dinhxuanvu commented Nov 14, 2019

openshift-ci-robot commented Nov 15, 2019

dinhxuanvu commented Nov 20, 2019

openshift-ci-robot commented Nov 21, 2019

njhale commented Nov 21, 2019

openshift-ci-robot commented Nov 21, 2019

dinhxuanvu commented Nov 22, 2019

dinhxuanvu commented Nov 22, 2019

dinhxuanvu commented Nov 26, 2019

dinhxuanvu commented Dec 3, 2019

jpeeler commented Dec 3, 2019

dinhxuanvu commented Dec 3, 2019

openshift-ci-robot commented Dec 3, 2019

jpeeler commented Dec 3, 2019

jpeeler commented Dec 3, 2019

openshift-ci-robot commented Dec 3, 2019

openshift-ci-robot commented Dec 3, 2019

njhale left a comment

Choose a reason for hiding this comment

njhale Dec 3, 2019

Choose a reason for hiding this comment

njhale Dec 3, 2019

Choose a reason for hiding this comment

njhale Dec 3, 2019

Choose a reason for hiding this comment

njhale Dec 3, 2019

Choose a reason for hiding this comment

njhale Dec 3, 2019

Choose a reason for hiding this comment

njhale Dec 3, 2019

Choose a reason for hiding this comment

dinhxuanvu commented Dec 3, 2019

openshift-cherrypick-robot commented Dec 3, 2019

dinhxuanvu commented Dec 4, 2019

openshift-cherrypick-robot commented Dec 4, 2019