Bug 1880501: Adding overwrite-latest flag to index add #448

gallettilance · 2020-09-18T16:11:14Z

Description of the change:

Allow index maintainers to overwrite the latest bundle in a package on opm index add by adding an --overwrite-latest option.

Caveats

valid only when using replaces mode
each overwrite pulls all images for the package in order to rebuild the graph

Benefits

image pulls are parallelized
supports overwrites of the update graph

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 · 2020-09-18T17:16:48Z

Can we have a description/summary on what we are trying to accomplish with this? Perhaps mentioning the use case/issue here would be helpful.

openshift-ci-robot · 2020-09-18T17:18:02Z

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

Bug 1880501: Adding overwrite-latest flag to index add

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/registry/populator.go

pkg/registry/testdata/overwrite/prometheus.0.14.0/manifests/servicemonitor.crd.yaml

pkg/registry/populator_test.go

openshift-ci-robot · 2020-09-18T23:06:17Z

@gallettilance: This pull request references Bugzilla bug 1880501, which is valid.

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 POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1880501: Adding overwrite-latest flag to index add

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 · 2020-09-18T23:07:36Z

/retest

pkg/lib/registry/registry.go

pkg/registry/populator_test.go

pkg/registry/populator.go

gallettilance · 2020-09-22T21:23:09Z

/test unit

pkg/registry/bundle.go

ecordell · 2020-09-22T22:06:01Z

pkg/registry/imageinput.go

@@ -16,9 +16,9 @@ type ImageInput struct {
 	metadataDir      string
 	to               image.Reference
 	from             string
-	annotationsFile  *AnnotationsFile
+	AnnotationsFile  *AnnotationsFile


same: why did these become public?

same as #448 (comment)

ecordell · 2020-09-22T22:27:59Z

pkg/registry/populator.go

@@ -119,28 +167,38 @@ func (i *DirectoryPopulator) loadManifests(imagesToAdd []*ImageInput, mode Mode)
 		// same package linearly.
 		var err error
 		var validImagesToAdd []*ImageInput
+
+		for pkg := range i.overwriteDirMap {
+			err := i.loader.RemovePackage(pkg)


I think we want the remove / add to happen in a single transaction.

It would also be nice to not need to thread through the "overwriteImages" separately everywhere. is there a way we can we push the overwrite option down to loadManifestReplaces (or call loadManifestsReplaceOverwrite if overwrite is set, etc), so that we only need to worry about overwriting when we try to insert and it fails because it already exists?

we can but that requires exposing the image registry package to the registry package which today can't easily be done because of cyclic imports...

I think you could avoid importing the image/registry package with a ReplacePackage(pkg string, bundle ...Bundle) method on the loader -- still getting a single transaction at the DB level.

However, I do think that at this point, we should prioritize getting something working and merged over long-term support; partly b/c the declarative index config will supersede this work in the near future.

ecordell · 2020-09-22T22:28:06Z

pkg/registry/populator.go

+		graphLoader:     graphLoader,
+		querier:         querier,
+		imageDirMap:     imageDirMap,
+		overwriteDirMap: overwriteDirMap,


do we need to distinguish which ones are "overwrites" up front?

yeah I think it makes sense to do it upfront so we only pull the images when we need to. I think more of the registry package needs to be made public in order to make that possible but it shouldn't be too bad

it reduces the amount of unit tests we can do though...

cmd/opm/index/add.go

pkg/lib/registry/registry.go

maneeshmehra · 2020-09-23T14:07:43Z

@gallettilance had already marked the bugzilla item linked to this PR as critical. Asking all reviewers to review this with highest priority as it blocks our operator verification pipelines. TIA.

openshift-ci-robot · 2020-09-23T15:15:20Z

@gallettilance: This pull request references Bugzilla bug 1880501, which is valid.

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 POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1880501: Adding overwrite-latest flag to index add

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 · 2020-09-23T17:26:34Z

/approve

openshift-ci-robot · 2020-09-23T17:26:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gallettilance, njhale

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

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

gallettilance · 2020-09-23T19:00:44Z

/restest

gallettilance · 2020-09-23T19:22:17Z

/retest

This adds the ability to overwrite the latest bundle during index add in order to avoid having to remove all the bundles from the index and re-add them before being able to add the updated bundle.

njhale · 2020-09-23T22:44:22Z

All pending comments addressed offline.

/lgtm

openshift-bot · 2020-09-23T23:03:24Z

/retest

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

openshift-bot · 2020-09-23T23:16:24Z

/retest

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

openshift-bot · 2020-09-23T23:29:23Z

/retest

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

openshift-ci-robot · 2020-09-24T00:30:36Z

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

operator-framework/operator-registry#448

Bugzilla bug 1880501 has been moved to the MODIFIED state.

In response to this:

Bug 1880501: Adding overwrite-latest flag to index add

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 awgreene and dinhxuanvu September 18, 2020 16:11

gallettilance requested review from kevinrizza, shawn-hurley, ecordell and njhale September 18, 2020 16:13

gallettilance changed the title ~~Adding overwrite-latest flag to index add~~ WIP: Adding overwrite-latest flag to index add Sep 18, 2020

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

gallettilance force-pushed the force-bundle branch 2 times, most recently from c76b413 to 91b4d82 Compare September 18, 2020 16:47

gallettilance changed the title ~~WIP: Adding overwrite-latest flag to index add~~ Bug 1880501: Adding overwrite-latest flag to index add Sep 18, 2020

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

openshift-ci-robot added bugzilla/severity-urgent bugzilla/valid-bug labels Sep 18, 2020

njhale reviewed Sep 18, 2020

View reviewed changes

pkg/registry/populator.go Outdated Show resolved Hide resolved

gallettilance force-pushed the force-bundle branch from 91b4d82 to 46f8bbc Compare September 18, 2020 17:38

njhale reviewed Sep 18, 2020

View reviewed changes

pkg/registry/populator.go Outdated Show resolved Hide resolved

gallettilance force-pushed the force-bundle branch 2 times, most recently from 301a859 to f283a55 Compare September 18, 2020 20:59

njhale reviewed Sep 18, 2020

View reviewed changes

pkg/registry/populator.go Outdated Show resolved Hide resolved

pkg/registry/testdata/overwrite/prometheus.0.14.0/manifests/servicemonitor.crd.yaml Outdated Show resolved Hide resolved

pkg/registry/populator_test.go Show resolved Hide resolved

gallettilance force-pushed the force-bundle branch from f283a55 to c815e8a Compare September 18, 2020 21:23

gallettilance force-pushed the force-bundle branch 3 times, most recently from f789745 to 78f4fda Compare September 20, 2020 22:14

njhale reviewed Sep 21, 2020

View reviewed changes

pkg/lib/registry/registry.go Outdated Show resolved Hide resolved

pkg/lib/registry/registry.go Outdated Show resolved Hide resolved

pkg/registry/populator_test.go Show resolved Hide resolved

pkg/registry/populator.go Show resolved Hide resolved

gallettilance force-pushed the force-bundle branch from 78f4fda to fe93629 Compare September 21, 2020 14:05

gallettilance force-pushed the force-bundle branch from db367fb to af77afb Compare September 22, 2020 14:15

openshift-ci-robot added the needs-rebase label Sep 22, 2020

gallettilance force-pushed the force-bundle branch 2 times, most recently from 32cc1e4 to a4b1169 Compare September 22, 2020 14:38

openshift-ci-robot removed the needs-rebase label Sep 22, 2020

gallettilance force-pushed the force-bundle branch 4 times, most recently from 7346a0f to f3c5dbb Compare September 22, 2020 21:14

ecordell reviewed Sep 22, 2020

View reviewed changes

njhale reviewed Sep 22, 2020

View reviewed changes

cmd/opm/index/add.go Outdated Show resolved Hide resolved

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

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

gallettilance force-pushed the force-bundle branch from 1ea2d95 to a84e891 Compare September 23, 2020 17:31

(fix) index add --overwrite-latest

16b91b8

This adds the ability to overwrite the latest bundle during index add in order to avoid having to remove all the bundles from the index and re-add them before being able to add the updated bundle.

gallettilance force-pushed the force-bundle branch from a84e891 to 16b91b8 Compare September 23, 2020 22:41

openshift-ci-robot assigned njhale Sep 23, 2020

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

openshift-merge-robot merged commit f634a5a into operator-framework:master Sep 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1880501: Adding overwrite-latest flag to index add #448

Bug 1880501: Adding overwrite-latest flag to index add #448

gallettilance commented Sep 18, 2020 •

edited

Loading

dinhxuanvu commented Sep 18, 2020

openshift-ci-robot commented Sep 18, 2020

openshift-ci-robot commented Sep 18, 2020

njhale commented Sep 18, 2020

gallettilance commented Sep 22, 2020

ecordell Sep 22, 2020

gallettilance Sep 23, 2020

ecordell Sep 22, 2020

gallettilance Sep 23, 2020

njhale Sep 23, 2020

ecordell Sep 22, 2020

gallettilance Sep 23, 2020

gallettilance Sep 23, 2020

maneeshmehra commented Sep 23, 2020

openshift-ci-robot commented Sep 23, 2020

njhale commented Sep 23, 2020

openshift-ci-robot commented Sep 23, 2020

gallettilance commented Sep 23, 2020

gallettilance commented Sep 23, 2020

njhale commented Sep 23, 2020

openshift-bot commented Sep 23, 2020

openshift-bot commented Sep 23, 2020

openshift-bot commented Sep 23, 2020

openshift-ci-robot commented Sep 24, 2020

Bug 1880501: Adding overwrite-latest flag to index add #448

Bug 1880501: Adding overwrite-latest flag to index add #448

Conversation

gallettilance commented Sep 18, 2020 • edited Loading

dinhxuanvu commented Sep 18, 2020

openshift-ci-robot commented Sep 18, 2020

openshift-ci-robot commented Sep 18, 2020

njhale commented Sep 18, 2020

gallettilance commented Sep 22, 2020

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

maneeshmehra commented Sep 23, 2020

openshift-ci-robot commented Sep 23, 2020

njhale commented Sep 23, 2020

openshift-ci-robot commented Sep 23, 2020

gallettilance commented Sep 23, 2020

gallettilance commented Sep 23, 2020

njhale commented Sep 23, 2020

openshift-bot commented Sep 23, 2020

openshift-bot commented Sep 23, 2020

openshift-bot commented Sep 23, 2020

openshift-ci-robot commented Sep 24, 2020

gallettilance commented Sep 18, 2020 •

edited

Loading