Bug 1880127: Parallelize opm index export #429

harishsurf · 2020-08-31T18:24:11Z

Description of the change:
This PR modifies opm index export to ensure exporting is done in parallel with an intent to cut down the overall time taken. It is achieved by the following two changes:

Make --package flag of the opm index export optional and allow comma-separated values
Parallelize the ExportFromIndex API

With this change, the export is possible by just specifying the index-image

Motivation for the change:
Unpacking and exporting of community operators takes >2hrs as it is done synchronously

Signed-off-by: Harish [email protected]

harishsurf · 2020-09-01T16:03:24Z

/test e2e-aws

benluddy

A little test coverage would go a long way toward feeling confident in this kind of change. It looks like there would be a lot of refactoring involved to make this unit testable. Maybe adding a few solid end-to-end tests is a decent compromise?

pkg/lib/indexer/indexer.go

pkg/lib/indexer/indexer_test.go

pkg/lib/indexer/indexer.go

kevinrizza · 2020-09-08T20:47:37Z

pkg/lib/indexer/indexer.go

+				return err
+			}
+			if bundleVersion == "" {
+				// operator-registry does not care about the folder name


This comment feels odd, why not just describe what we're doing rather than why it is happening? It feels like a comment that would quickly go out of date if another module changed behavior. It also probably means that bundleVersion is the wrong variable to set here (either bundleVersion should actually be dirPrefix or something like that, or we should set the value of the variable further down)

I don't think this check is needed in the newly modified code (maybe I am wrong?)

ah, you are mostly right. the problem is the case where bundleVersion is not set -- technically version is an optional field on the CSV. in that case, you still need to generate something for the value of the folder.

Added the check to generate a random name when empty

harishsurf · 2020-09-09T23:23:23Z

/retest

mvalahtv · 2020-09-10T08:21:31Z

@harishsurf is it possible to test it?

harishsurf · 2020-09-10T13:06:03Z

@mvalahtv, I have added an e2e test that uses the community operator's catsrc image. If you had to test it locally, once you pull down this code, you could run opm index export --index quay.io/operatorhubio/catalog:latest to see it in action

kevinrizza · 2020-09-10T13:46:47Z

test/e2e/opm_test.go

@@ -44,6 +44,7 @@ var (
 	indexImage1 = "quay.io/olmtest/e2e-index:" + indexTag1
 	indexImage2 = "quay.io/olmtest/e2e-index:" + indexTag2
 	indexImage3 = "quay.io/olmtest/e2e-index:" + indexTag3
+	indexImage4 = "quay.io/operatorhubio/catalog:latest"


Can we use one of the test indexes that we already build rather than rely on an external dependency that we don't control?

changed it to use indexImage2

This change makes --package flag optional and allows multiple packages to be specified. It also parallelizes the export by leveraging go's concurrency Signed-off-by: Harish <[email protected]>

harishsurf · 2020-09-10T19:35:46Z

/test e2e-aws

kevinrizza · 2020-09-10T20:15:26Z

/approve

mvalahtv · 2020-09-15T07:19:24Z

@harishsurf it is working fine for me on local machine. I can export catalog in 2 mins. Great job !!!

njhale

Looking good. I think we want to attach a BZ so we can get this merged. It improves UX considerably from what we have today.

pkg/lib/indexer/indexer.go

openshift-ci-robot · 2020-09-17T18:06:55Z

@harishsurf: This pull request references Bugzilla bug 1880127, which is invalid:

expected the bug to target the "4.6.0" release, but it targets "4.6.z" 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 1880127: Parallelize opm index export

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.

harishsurf · 2020-09-18T13:41:01Z

/bugzilla refresh

openshift-ci-robot · 2020-09-18T13:41:09Z

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

dinhxuanvu

/lgtm

openshift-ci-robot · 2020-09-18T14:01:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu, harishsurf, kevinrizza

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~~ [dinhxuanvu,kevinrizza]

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 · 2020-09-18T15:44:28Z

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

operator-framework/operator-registry#429

Bugzilla bug 1880127 has been moved to the MODIFIED state.

In response to this:

Bug 1880127: Parallelize opm index export

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 do-not-merge/work-in-progress label Aug 31, 2020

openshift-ci-robot requested review from Bowenislandsong and hasbro17 August 31, 2020 18:24

harishsurf force-pushed the parallelize-export branch from 8ad9be1 to 2379eb6 Compare September 1, 2020 14:07

benluddy reviewed Sep 1, 2020

View reviewed changes

harishsurf force-pushed the parallelize-export branch 4 times, most recently from 6fcb6ef to 4942103 Compare September 8, 2020 18:56

harishsurf changed the title ~~[WIP] Parallelize opm index export~~ Parallelize opm index export Sep 8, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress label Sep 8, 2020

benluddy reviewed Sep 8, 2020

View reviewed changes

kevinrizza reviewed Sep 8, 2020

View reviewed changes

harishsurf force-pushed the parallelize-export branch from 4942103 to b8d1c06 Compare September 9, 2020 18:11

kevinrizza reviewed Sep 10, 2020

View reviewed changes

Parallelize opm index export

338b4fa

This change makes --package flag optional and allows multiple packages to be specified. It also parallelizes the export by leveraging go's concurrency Signed-off-by: Harish <[email protected]>

harishsurf force-pushed the parallelize-export branch from b8d1c06 to 338b4fa Compare September 10, 2020 18:49

openshift-ci-robot added the approved label Sep 10, 2020

njhale reviewed Sep 17, 2020

View reviewed changes

pkg/lib/indexer/indexer.go Show resolved Hide resolved

pkg/lib/indexer/indexer.go Show resolved Hide resolved

njhale changed the title ~~Parallelize opm index export~~ Bug 1880127: Parallelize opm index export Sep 17, 2020

openshift-ci-robot added bugzilla/severity-medium bugzilla/invalid-bug labels Sep 17, 2020

openshift-ci-robot added bugzilla/severity-high bugzilla/valid-bug and removed bugzilla/severity-medium bugzilla/invalid-bug labels Sep 18, 2020

dinhxuanvu approved these changes Sep 18, 2020

View reviewed changes

openshift-ci-robot assigned dinhxuanvu Sep 18, 2020

openshift-ci-robot added the lgtm label Sep 18, 2020

openshift-merge-robot merged commit e6088e1 into operator-framework:master Sep 18, 2020

ankitathomas mentioned this pull request Oct 1, 2020

Bug 1881527: Add e2e tests for export directory structure #468

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1880127: Parallelize opm index export #429

Bug 1880127: Parallelize opm index export #429

harishsurf commented Aug 31, 2020 •

edited

Loading

harishsurf commented Sep 1, 2020

benluddy left a comment

kevinrizza Sep 8, 2020

harishsurf Sep 9, 2020

kevinrizza Sep 9, 2020

harishsurf Sep 10, 2020

harishsurf commented Sep 9, 2020

mvalahtv commented Sep 10, 2020

harishsurf commented Sep 10, 2020

kevinrizza Sep 10, 2020

harishsurf Sep 10, 2020

harishsurf commented Sep 10, 2020

kevinrizza commented Sep 10, 2020

mvalahtv commented Sep 15, 2020

njhale left a comment

openshift-ci-robot commented Sep 17, 2020

harishsurf commented Sep 18, 2020

openshift-ci-robot commented Sep 18, 2020

dinhxuanvu left a comment

openshift-ci-robot commented Sep 18, 2020

openshift-ci-robot commented Sep 18, 2020

Bug 1880127: Parallelize opm index export #429

Bug 1880127: Parallelize opm index export #429

Conversation

harishsurf commented Aug 31, 2020 • edited Loading

harishsurf commented Sep 1, 2020

benluddy left a comment

Choose a reason for hiding this comment

kevinrizza Sep 8, 2020

Choose a reason for hiding this comment

harishsurf Sep 9, 2020

Choose a reason for hiding this comment

kevinrizza Sep 9, 2020

Choose a reason for hiding this comment

harishsurf Sep 10, 2020

Choose a reason for hiding this comment

harishsurf commented Sep 9, 2020

mvalahtv commented Sep 10, 2020

harishsurf commented Sep 10, 2020

kevinrizza Sep 10, 2020

Choose a reason for hiding this comment

harishsurf Sep 10, 2020

Choose a reason for hiding this comment

harishsurf commented Sep 10, 2020

kevinrizza commented Sep 10, 2020

mvalahtv commented Sep 15, 2020

njhale left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Sep 17, 2020

harishsurf commented Sep 18, 2020

openshift-ci-robot commented Sep 18, 2020

dinhxuanvu left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Sep 18, 2020

openshift-ci-robot commented Sep 18, 2020

harishsurf commented Aug 31, 2020 •

edited

Loading