Add support for storing and retrieving related images #101

ecordell · 2019-10-22T23:08:10Z

Description of the change:

This is based on #100, which should merge first.

This adds two new Query methods:

type Query interface { 
  ListImages(ctx context.Context) ([]string, error)
  GetImagesForBundle(ctx context.Context, bundleName string) ([]string, error)
  // ...
}

The bundle abstraction now also understands the images in the containers and initContainers fields of a CSV, and the relatedImages field:

kind: ClusterServiceVersion 
spec:
  relatedImages:
  - name: default
    image: quay.io/coreos/etcd@sha256:12345 
  - name: etcd-2.1.5
    image: quay.io/coreos/etcd@sha256:12345 
  - name: etcd-3.1.1
    image: quay.io/coreos/etcd@sha256:12345

Motivation for the change:

See the proposal, this is primarily to support disconnected mirroring of a catalog.

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

Replaces #62

ecordell · 2019-10-24T13:22:13Z

cmd/opm/registry/serve.go

@@ -98,3 +103,23 @@ func runRegistryServeCmdFunc(cmd *cobra.Command, args []string) error {

 	return nil
 }
+
+func migrate(cmd *cobra.Command, db *sql.DB) error {


I duplicated this for now; I didn't want to refactor the commands and make this PR bigger than it already is.

ecordell · 2019-10-24T13:24:25Z

pkg/registry/csv.go

@@ -187,3 +191,74 @@ func (csv *ClusterServiceVersion) GetApiServiceDefinitions() (owned []*Definitio
 	required = definitions.Required
 	return
 }
+
+// GetRelatedImage returns the list of associated images for the operator


This is the where related and operator images are extracted

ecordell · 2019-10-24T13:26:35Z

pkg/sqlite/load.go

+		return err
+	}
+	// TODO: bulk insert
+	for img := range imgs {


This is where related/operator images are stored

ecordell · 2019-10-24T13:30:05Z

pkg/sqlite/migrations/000_init.go

+
+var initMigration = &Migration{
+	Id: InitMigrationKey,
+	Up: func(ctx context.Context, tx *sql.Tx) error {


This migration brings an empty DB up to where the schema from before we had migrations.

If we do this we lose the ability to run migrations on registry databases that were built before this migrator was added. I think that's fine, though.

Thanks for catching this - I had intended to go back and make another migrator method that migrates from the current version up to the latest version. Added and updated.

ecordell · 2019-10-24T13:30:17Z

pkg/sqlite/migrations/001_related_images.go

+		if err != nil {
+			return err
+		}
+		for _, bundle := range bundles {


This backfills the related_image table for existing stored bundles

ecordell · 2019-10-24T14:33:43Z

pkg/sqlite/migrator.go

-	if err != nil {
-		return err
-	}
+	for _, migration := range migrations {


The bulk of the migrator is here

ecordell · 2019-10-24T14:34:09Z

pkg/sqlite/migrations/migrations.go

+
+type MigrationSet map[int]*Migration
+
+var migrations MigrationSet = make(map[int]*Migration)


this map is built up in init methods in the migration files

anik120

Looks good @ecordell.

Just had a question about a down migration, and also, do you think including a doc that documents how to use the migration (file format, generate bin data etc) with this PR is a good idea?

anik120 · 2019-10-24T15:27:26Z

pkg/sqlite/migrations/000_init.go

+	},
+	Down: func(ctx context.Context, tx *sql.Tx) error {
+		sql := `
+			DROP TABLE operatorbundle;


Won't trying to drop tables without switching off foreign key constraints fail?

Great note - this will succeed because it will also drop all of the tables with the foreign key refs.

I'll update to enable foreign keys on these tests so that we catch future issues like this.

kevinrizza

Looks great, just had very small comments. Otherwise it should be good to merge.

kevinrizza · 2019-10-24T15:41:25Z

pkg/sqlite/migrations/000_init.go

+
+var initMigration = &Migration{
+	Id: InitMigrationKey,
+	Up: func(ctx context.Context, tx *sql.Tx) error {


If we do this we lose the ability to run migrations on registry databases that were built before this migrator was added. I think that's fine, though.

pkg/registry/csv.go

ecordell · 2019-10-24T15:56:22Z

@anik120 I enabled foreign keys for the tests and added a doc on adding migrations.

the migrations package we were using didn't allow for backfills

this uses the `relatedImages` field from the CSV and also extracts operator images from the deployment specs of CSVs.

to the latest

kevinrizza · 2019-10-24T21:16:16Z

/lgtm

openshift-ci-robot · 2019-10-24T21:16:20Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

ecordell · 2019-10-24T21:19:54Z

/retest

openshift-ci-robot requested review from alecmerdler and dinhxuanvu October 22, 2019 23:08

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 22, 2019

ecordell mentioned this pull request Oct 22, 2019

[WIP] feat(load,query): when loading bundles, extract a list of images #62

Closed

1 task

ecordell force-pushed the relatedimages branch 2 times, most recently from a9c2b74 to 91e63e7 Compare October 22, 2019 23:21

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 23, 2019

ecordell force-pushed the relatedimages branch 3 times, most recently from f0d89c4 to 77b1ce2 Compare October 24, 2019 13:16

ecordell commented Oct 24, 2019

View reviewed changes

anik120 reviewed Oct 24, 2019

View reviewed changes

ecordell force-pushed the relatedimages branch from 77b1ce2 to d172330 Compare October 24, 2019 15:43

kevinrizza reviewed Oct 24, 2019

View reviewed changes

ecordell force-pushed the relatedimages branch from d172330 to 808bb02 Compare October 24, 2019 15:55

ecordell added 2 commits October 24, 2019 11:57

feat(migrations): switch to go-based migrator

274bf6b

the migrations package we were using didn't allow for backfills

feat(images): add support for storing and querying related images

8415c18

this uses the `relatedImages` field from the CSV and also extracts operator images from the deployment specs of CSVs.

ecordell force-pushed the relatedimages branch from 808bb02 to 8415c18 Compare October 24, 2019 15:57

feat(migrate): migrate from current version up

addcd23

to the latest

ecordell force-pushed the relatedimages branch from 1edebcc to addcd23 Compare October 24, 2019 19:44

openshift-ci-robot assigned kevinrizza Oct 24, 2019

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

openshift-merge-robot merged commit 161f66a into operator-framework:master Oct 24, 2019

ecordell deleted the relatedimages branch October 24, 2019 21:55

This was referenced Oct 25, 2019

Image digests for referencing bundles #103

Merged

Migrate database up before serving using opm #100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for storing and retrieving related images #101

Add support for storing and retrieving related images #101

ecordell commented Oct 22, 2019

ecordell Oct 24, 2019

ecordell Oct 24, 2019

ecordell Oct 24, 2019

ecordell Oct 24, 2019

kevinrizza Oct 24, 2019

ecordell Oct 24, 2019

ecordell Oct 24, 2019

ecordell Oct 24, 2019

ecordell Oct 24, 2019 •

edited

Loading

anik120 left a comment

anik120 Oct 24, 2019

ecordell Oct 24, 2019

kevinrizza left a comment

kevinrizza Oct 24, 2019

ecordell commented Oct 24, 2019

kevinrizza commented Oct 24, 2019

openshift-ci-robot commented Oct 24, 2019

ecordell commented Oct 24, 2019


		type MigrationSet map[int]*Migration

		var migrations MigrationSet = make(map[int]*Migration)

Add support for storing and retrieving related images #101

Add support for storing and retrieving related images #101

Conversation

ecordell commented Oct 22, 2019

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

ecordell Oct 24, 2019 • edited Loading

Choose a reason for hiding this comment

anik120 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinrizza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecordell commented Oct 24, 2019

kevinrizza commented Oct 24, 2019

openshift-ci-robot commented Oct 24, 2019

ecordell commented Oct 24, 2019

ecordell Oct 24, 2019 •

edited

Loading