Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

ecordell
Copy link
Member

@ecordell ecordell commented Mar 28, 2020

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:

  1. 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.
  2. 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:

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

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 28, 2020
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 28, 2020
@ecordell
Copy link
Member Author

/retest

required apis with channel entries, rather than bundles.

this trades an additional join at query time for a much simpler load
interface
@ecordell 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 openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 30, 2020
@ecordell
Copy link
Member Author

ecordell commented Mar 30, 2020

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

@exdx exdx self-assigned this Mar 30, 2020
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
Copy link
Member

@njhale njhale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
pkg/containertools/labelreader_test.go Outdated Show resolved Hide resolved
pkg/registry/bundle.go Outdated Show resolved Hide resolved
pkg/registry/interface.go Show resolved Hide resolved
pkg/sqlite/migrations/007_replaces_skips.go Outdated Show resolved Hide resolved
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.
Copy link
Member

@njhale njhale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@ecordell
Copy link
Member Author

/retest

3 similar comments
@ecordell
Copy link
Member Author

/retest

@ecordell
Copy link
Member Author

/retest

@ecordell
Copy link
Member Author

ecordell commented Apr 1, 2020

/retest

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@exdx
Copy link
Member

exdx commented Apr 1, 2020

/retest

@exdx
Copy link
Member

exdx commented Apr 1, 2020

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 openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 1, 2020
@openshift-ci-robot
Copy link

[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:

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

@ecordell
Copy link
Member Author

ecordell commented Apr 1, 2020

/retest

@openshift-bot
Copy link
Contributor

/retest

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

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants