fix(loader): Image Loader doesn't create the same channel entries as the other loaders. #236

ecordell · 2020-03-28T19:31:25Z

This began as an investigation into #205 - odd behavior when adding a new bundle via index add

The issue there was the image loader was not properly handling the case (as described in #205) where a new bundle being added both replaces something in an existing channel, and starts a new channel as well.

I removed the assumption that a replaces exists in the current channel, which was enough to prevent the bug in #205. I wrote a test in image_test.go to verify the behavior with the new graph representation.

That test made me realize that the image loader was not building graphs the same way as the other loaders - because the image loader doesn't synthesize nodes outside the current channel, the graph comes out missing some nodes. In particular, we would fail to answer correctly "does a replacement for X exist in channel A" even if there is an operator in channel A with a replaces field that says X.

The fact that the behaviors diverge at all is somewhat concerning, so I looked at what it would take to have the image loader and the other loaders share the same graph building implementation. There were two blockers:

When the "old" loaders build the graph, they expect to be able to query the CSV for its replaces field, which is not always present anymore when using bundle images.
Provided and Required APIs are associated with a channel entry, not a bundle (this was done originally to remove a join from the most common query path). The simplest thing to do to keep the loaders the same was to update the graph by deleting the existing graph for a package and recalculating it - but deleting the graph would lose the provided and required api data, because they were tied to the graph nodes and not the bundle.

I started by addressing those two things:

The first commit adds a migration that associates provided and required apis with the bundle directly
The second commit adds a migration that extracts replaces / skips from the bundle when adding it.
The third commit adds a test to verify the correct graph output for the opm: Adding bundle to registry with new channel fails and deletes previous bundles #205 case, and refactors the image loader to re-use the existing load paths.

Follow up work:

There should be some indication that the migration may be incomplete if bundlepaths are used. I think this is somewhat okay at the moment because there are not a lot of bundle images being used. But existing indexes would need to rm the package and re-add all of the bundle images so that the replaces/skips fields are unpacked correctly. We should have some standard way to handle this going forward and surface options to the user for whether they want bundle pulls to occur for migrations.

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

ecordell · 2020-03-29T18:18:03Z

/retest

required apis with channel entries, rather than bundles. this trades an additional join at query time for a much simpler load interface

ecordell · 2020-03-30T21:51:23Z

Tested with some real bundles:

$ opm index add -c docker -b quay.io/shawn_hurley/testoperator:v1.0.0 --tag quay.io/ecordell/test-index:0
INFO[0000] building the index                            bundles="[quay.io/shawn_hurley/testoperator:v1.0.0]"
INFO[0000] running docker pull                           img="quay.io/shawn_hurley/testoperator:v1.0.0"
INFO[0000] running docker save                           img="quay.io/shawn_hurley/testoperator:v1.0.0"
INFO[0000] loading Bundle quay.io/shawn_hurley/testoperator:v1.0.0  img="quay.io/shawn_hurley/testoperator:v1.0.0"
INFO[0000] found annotations file searching for csv      dir=bundle_tmp215707306 file=bundle_tmp215707306/metadata load=annotations
INFO[0000] found csv, loading bundle                     dir=bundle_tmp215707306 file=bundle_tmp215707306/manifests load=bundle
INFO[0000] loading bundle file                           dir=bundle_tmp215707306/manifests file=testoperator.v1.0.0.clusterserviceversion.yaml load=bundle name=testoperator.v1.0.0
INFO[0000] Generating dockerfile                         bundles="[quay.io/shawn_hurley/testoperator:v1.0.0]"
INFO[0000] writing dockerfile: index.Dockerfile857028972  bundles="[quay.io/shawn_hurley/testoperator:v1.0.0]"
INFO[0000] running docker build                          bundles="[quay.io/shawn_hurley/testoperator:v1.0.0]"
INFO[0000] [docker build -f index.Dockerfile857028972 -t quay.io/ecordell/test-index:0 .]  bundles="[quay.io/shawn_hurley/testoperator:v1.0.0]"

Channel Entry Table:

entry_id	channel_name	package_name	operatorbundle_name	replaces	depth
1	BETA	test-operator	testoperator.v1.0.0		0

$ docker push quay.io/ecordell/test-index:0
$ opm index add -c docker -f quay.io/ecordell/test-index:0 -b quay.io/shawn_hurley/testoperator:v1.0.1 --tag quay.io/ecordell/test-index:1
INFO[0000] building the index                            bundles="[quay.io/shawn_hurley/testoperator:v1.0.1]"
INFO[0000] Pulling previous image quay.io/ecordell/test-index:0 to get metadata  bundles="[quay.io/shawn_hurley/testoperator:v1.0.1]"
INFO[0000] running docker pull                           bundles="[quay.io/shawn_hurley/testoperator:v1.0.1]"
INFO[0000] Getting label data from previous image        bundles="[quay.io/shawn_hurley/testoperator:v1.0.1]"
INFO[0000] running docker inspect                        bundles="[quay.io/shawn_hurley/testoperator:v1.0.1]"
INFO[0000] running docker pull                           bundles="[quay.io/shawn_hurley/testoperator:v1.0.1]"
INFO[0001] running docker save                           bundles="[quay.io/shawn_hurley/testoperator:v1.0.1]"
INFO[0043] running docker pull                           img="quay.io/shawn_hurley/testoperator:v1.0.1"
INFO[0044] running docker save                           img="quay.io/shawn_hurley/testoperator:v1.0.1"
INFO[0044] loading Bundle quay.io/shawn_hurley/testoperator:v1.0.1  img="quay.io/shawn_hurley/testoperator:v1.0.1"
INFO[0044] found annotations file searching for csv      dir=bundle_tmp494931583 file=bundle_tmp494931583/metadata load=annotations
INFO[0044] found csv, loading bundle                     dir=bundle_tmp494931583 file=bundle_tmp494931583/manifests load=bundle
INFO[0044] loading bundle file                           dir=bundle_tmp494931583/manifests file=testoperator.v1.0.1.clusterserviceversion.yaml load=bundle name=testoperator.v1.0.1
INFO[0044] Generating dockerfile                         bundles="[quay.io/shawn_hurley/testoperator:v1.0.1]"
INFO[0044] writing dockerfile: index.Dockerfile925925129  bundles="[quay.io/shawn_hurley/testoperator:v1.0.1]"
INFO[0044] running docker build                          bundles="[quay.io/shawn_hurley/testoperator:v1.0.1]"
INFO[0044] [docker build -f index.Dockerfile925925129 -t quay.io/ecordell/test-index:1 .]  bundles="[quay.io/shawn_hurley/testoperator:v1.0.1]"

Channel Entry table:

entry_id	channel_name	package_name	operatorbundle_name	replaces	depth
1	BETA	test-operator	testoperator.v1.0.1	2	0
2	BETA	test-operator	testoperator.v1.0.0		1
3	STABLE	test-operator	testoperator.v1.0.1	4	0
4	STABLE	test-operator	testoperator.v1.0.0		1

pkg/sqlite/directory_test.go

pkg/sqlite/graphloader_test.go

pkg/sqlite/load.go

pkg/sqlite/migrations/006_associate_apis_with_bundle.go

njhale

Looks great!

My primary concern is the TODO for the migration of bundle image refs; i.e. where do we get replaces/skips since the CSV has been removed for non-channel-head bundles?

bundles/prometheus.0.15.0/metadata/annotations.yaml

pkg/containertools/labelreader_test.go

pkg/registry/bundle.go

pkg/registry/interface.go

pkg/sqlite/migrations/007_replaces_skips.go

pkg/sqlite/migrations/007_replaces_skips_test.go

This adds an additional test to the imageloader test suite that shows the issue (it fails without the other changes in this commit). The update logic was different from the "new" logic for adding bundles. To simplify reasoning about these two paths, they're now the same - updating will recalculate the graph from the new entrypoints and replace whatever was there before.

requires migrations

njhale

Looks great!

ecordell · 2020-03-31T22:30:40Z

/retest

ecordell · 2020-03-31T23:03:47Z

/retest

ecordell · 2020-03-31T23:49:19Z

/retest

ecordell · 2020-04-01T00:22:46Z

/retest

exdx · 2020-04-01T14:54:20Z

pkg/sqlite/migrations/006_associate_apis_with_bundle.go

+		return err
+	},
+	Down: func(ctx context.Context, tx *sql.Tx) error {
+		// TODO


what happens if the migration fails? should we do anything with this TODO?

exdx · 2020-04-01T14:56:17Z

/retest

exdx · 2020-04-01T14:57:39Z

Spent a good deal of time going over the PR with @ecordell - thanks for all the insight. Pulling out the skips and replaces (via the new schema updates) and rebuilding the image loader graph for each new bundle will also help with the bundle image commutativity work that we've been looking at @gallettilance @kevinrizza.

/lgtm

openshift-ci-robot · 2020-04-01T14:57:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ecordell, exdx, 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~~ [ecordell,njhale]

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

ecordell · 2020-04-01T17:51:36Z

/retest

openshift-bot · 2020-04-01T18:36:03Z

/retest

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

openshift-bot · 2020-04-01T20:59:01Z

/retest

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

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

openshift-ci-robot requested review from awgreene and njhale March 28, 2020 19:31

openshift-ci-robot added the approved label Mar 28, 2020

ecordell force-pushed the fix-add-err branch from 31ae1f4 to ff6ec08 Compare March 29, 2020 19:13

ecordell force-pushed the fix-add-err branch from ff6ec08 to 399b586 Compare March 29, 2020 19:14

ecordell changed the title ~~[wip] fix(loader): Image Loader doesn't create the same channel entries as the other loaders.~~ fix(loader): Image Loader doesn't create the same channel entries as the other loaders. Mar 30, 2020

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

exdx self-assigned this Mar 30, 2020

njhale reviewed Mar 31, 2020

View reviewed changes

pkg/sqlite/directory_test.go Outdated Show resolved Hide resolved

njhale reviewed Mar 31, 2020

View reviewed changes

pkg/sqlite/graphloader_test.go Outdated Show resolved Hide resolved

pkg/sqlite/load.go Outdated Show resolved Hide resolved

pkg/sqlite/load.go Outdated Show resolved Hide resolved

pkg/sqlite/migrations/006_associate_apis_with_bundle.go Show resolved Hide resolved

njhale reviewed Mar 31, 2020

View reviewed changes

ecordell added 2 commits March 31, 2020 17:23

feat(bundle): extract replaces/skips data on load

6494b99

ecordell force-pushed the fix-add-err branch from 651fb3b to 92b414d Compare March 31, 2020 21:35

test(migrations): add tests for api provider and

57e25b3

requires migrations

ecordell force-pushed the fix-add-err branch from 92b414d to 57e25b3 Compare March 31, 2020 21:42

njhale approved these changes Mar 31, 2020

View reviewed changes

exdx reviewed Apr 1, 2020

View reviewed changes

openshift-ci-robot added the lgtm label Apr 1, 2020

cdjohnson mentioned this pull request Apr 1, 2020

opm: Adding bundle to registry with new channel fails and deletes previous bundles #205

Closed

openshift-merge-robot merged commit dd110eb into operator-framework:master Apr 1, 2020

This was referenced Apr 6, 2020

opm 1.7.0: All channels deleted except for latest with opm registry add #258

Closed

opm 1.8.0: Adding a new channel incorrectly deletes csv and bundle json from other channels #261

Closed

dinhxuanvu mentioned this pull request Apr 27, 2020

Bug 1809320: fix(registry): change db load to get providedAPIs via bundle content #235

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(loader): Image Loader doesn't create the same channel entries as the other loaders. #236

fix(loader): Image Loader doesn't create the same channel entries as the other loaders. #236

ecordell commented Mar 28, 2020 •

edited

Loading

ecordell commented Mar 29, 2020

ecordell commented Mar 30, 2020 •

edited

Loading

njhale left a comment

njhale left a comment

ecordell commented Mar 31, 2020

ecordell commented Mar 31, 2020

ecordell commented Mar 31, 2020

ecordell commented Apr 1, 2020

exdx Apr 1, 2020

exdx commented Apr 1, 2020

exdx commented Apr 1, 2020

openshift-ci-robot commented Apr 1, 2020

ecordell commented Apr 1, 2020

openshift-bot commented Apr 1, 2020

openshift-bot commented Apr 1, 2020

fix(loader): Image Loader doesn't create the same channel entries as the other loaders. #236

fix(loader): Image Loader doesn't create the same channel entries as the other loaders. #236

Conversation

ecordell commented Mar 28, 2020 • edited Loading

ecordell commented Mar 29, 2020

ecordell commented Mar 30, 2020 • edited Loading

njhale left a comment

Choose a reason for hiding this comment

njhale left a comment

Choose a reason for hiding this comment

ecordell commented Mar 31, 2020

ecordell commented Mar 31, 2020

ecordell commented Mar 31, 2020

ecordell commented Apr 1, 2020

exdx Apr 1, 2020

Choose a reason for hiding this comment

exdx commented Apr 1, 2020

exdx commented Apr 1, 2020

openshift-ci-robot commented Apr 1, 2020

ecordell commented Apr 1, 2020

openshift-bot commented Apr 1, 2020

openshift-bot commented Apr 1, 2020

ecordell commented Mar 28, 2020 •

edited

Loading

ecordell commented Mar 30, 2020 •

edited

Loading