Adding OPM cli with registry add and rm commands #89

gallettilance · 2019-09-26T20:22:54Z

Description of the change:

Adding a new cli to operator-registry called opm. It can add bundles to a database, delete packages from that db and serve its contents at a specified host and port.

Having built the binaries, a user can run the old binaries

./bin/initializer ...
./bin/appregistry-server ...
./bin/registry-server ...
./bin/configmap-server ...

or can use the new cli that supports (and will support) our new operator bundle format. Here is an example flow:

Build the database (or add to an existing one) using

./bin/opm registry add -d=bundles.db -b="quay.io/username/imagename,quay.io/username/otherimagename"

Remove operators from an existing db using

./bin/opm registry rm -d=bundles.db -o="operator-package-name"

Serve the db using (can optionally specify a host and port)

./bin/opm registry serve -t=path/to/file.log -d=bundles.db

Motivation for the change:

This is a first step toward creating new commands to interface with operator-registry as part of its corresponding epic.

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

kevinrizza

Just had a comment about the location of the code, otherwise this is pretty much what I was imagining.

kevinrizza · 2019-09-26T20:30:58Z

pkg/cli/appregistryserver.go

@@ -0,0 +1,127 @@
+package cli


I think this code should still live in the ./cmd folder. Why are we moving it to the ./pkg folder?

And this comment goes for all the different sub commands

So for example have a folder cmd/commands where the code that is now in pkg/cli lives? I don't see why that wouldn't work. We would just have to change the Makefile a bit I think. I don't have a good argument why it should live in pkg :)

I'm actually suggesting that you can just leave 95% of this code exactly where it is. You can create your operator-registry command as a file in the root of the existing ./cmd/ folder called main.go that just imports those sub packages. Then just refactor the existing main() functions in each command slightly so that they do exactly what you have done to update them. Right now this PR has a ton of noise that makes it hard to read and I'm just suggesting that we can make this more readable without creating any extraneous files in the ./pkg/ folder.

kevinrizza · 2019-09-27T13:49:40Z

looks good to me

jpeeler · 2019-09-27T15:13:03Z

Makefile

@@ -1,5 +1,5 @@
 MOD_FLAGS := $(shell (go version | grep -q -E "1\.(11|12)") && echo -mod=vendor)
-CMDS  := $(addprefix bin/, $(shell go list $(MOD_FLAGS) ./cmd/... | xargs -I{} basename {}))
+CMDS  := $(addprefix bin/, $(shell go list $(MOD_FLAGS) ./cmd/... | xargs -I{} basename {} | grep -Fv "cli"))


Nit: I think it'd be better to at least use "cmd/cli" here, but it's probably fine.

Oops, I didn't think about the xargs here. A simpler addition would be just to add -w to grep so there's some whole word matching.

ecordell · 2019-09-27T15:44:57Z

cmd/operator-registry/main.go

+
+func main() {
+	rootCmd := &cobra.Command{
+		Use:   "operator-registry",


let's start the bikeshed:

opreg?

I liked what @njhale said at scrum: opm

krab 🦀

I liked what @njhale said at scrum: opm

What does opm stand for?

operator matters :)

I think it was operator package manager

What about opreg for the server stuff (configmap, appregistry, registry-server today), opm for bundle / index stuff (initializer today)?

It makes sense to me but didn't we want to combine all these into a single binary?

ecordell · 2019-10-03T14:08:46Z

@gallettilance This is looking good!

I think we just need:

the result of the bikeshedding discussion recorded (opm serve, opm build, etc)
an update to the readme to acknowledge these new commands
a scaffold of a man page / doc that we can update as we add functionality to the root command

ecordell · 2019-10-31T22:05:59Z

pkg/sqlite/image.go

+	return utilerrors.NewAggregate(errs)
+}
+
+// LoadBundleFunc walks the directory. When it sees a `.clusterserviceversion.yaml` file, it


nit: description is wrong

ecordell · 2019-10-31T22:07:58Z

pkg/sqlite/image.go

+	}
+
+	// Finally let's delete all the old bundles
+	if err = i.store.ClearNonDefaultBundles(packageManifest.PackageName); err != nil {


this should be behind a flag at least - it limits our options for transitioning (i.e. if I add an image bundle to an existing database, I lose all of my operator history...)

Are you imagining that we want to support a workflow like that? imo if you are using opm it means you are just getting the new stuff that version of opm provides. otherwise it will start getting really hard about reasoning about migrations, for example.

if they want an old catalog, they are still able to use initialize

Is there anything in the csv that isn't in the db (as far as function is concerned)?

ecordell · 2019-11-01T02:19:46Z

cmd/opm/registry/add.go

+
+	rootCmd.Flags().Bool("debug", false, "enable debug logging")
+	rootCmd.Flags().StringP("database", "d", "bundles.db", "relative path to database file")
+	rootCmd.Flags().StringSliceP("bundle-images", "b", []string{}, "comma separated list of links to bundle image")


non-blocking: did we think about making these positional?

opm add -d bundles.db quay.io/my/manifests:1 quay.io/my/manifests:2

That looks cool

ecordell · 2019-11-01T02:23:02Z

pkg/registry/interface.go

 	AddPackageChannels(manifest PackageManifest) error
+	RmPackageName(packageName string) error
+	ClearNonDefaultBundles(packageName string) error


non-blocking: I think these should probably be separate interfaces. we can refactor later.

ecordell · 2019-11-01T02:40:52Z

pkg/sqlite/load.go

@@ -135,8 +135,11 @@ func (s *SQLLoader) AddPackageChannels(manifest registry.PackageManifest) error
 	defer addReplaces.Close()

 	if _, err := addPackage.Exec(manifest.PackageName); err != nil {


instead of this, can we just always "update" package channels, so that it creates if it's not there? (rather than trying to write data and falling back to update on failure?)

non-blocking, we can follow up.

In the old format, it was not allowed to have packagename collisions and that's exactly the case in the new format where we are trying to update the db to contain a new version of some operator. I tried to take advantage of that such that when there are no previous versions of an operator in the db it defaults to the old format behavior

ecordell · 2019-11-01T14:49:24Z

pkg/sqlite/migrations/004_cascade_delete_test.go

+	// migrate up to, but not including, this migration
+	db, migrator, cleanup := CreateTestDbAt(t, migrations.CascadeDeleteMigrationKey-1)
+	defer cleanup()
+


you should validate that the table doesn't have the cascades here, before migrating up

ecordell · 2019-11-01T14:50:25Z

pkg/sqlite/migrations/004_cascade_delete_test.go

+func TestCascadeDeleteDown(t *testing.T) {
+	db, migrator, cleanup := CreateTestDbAt(t, migrations.CascadeDeleteMigrationKey)
+	defer cleanup()
+


again - you should validate the correct state here

ecordell · 2019-11-01T14:54:21Z

pkg/sqlite/load.go

-		return err
+		err = s.updatePackageChannels(tx, manifest)
+		if err != nil {
+			return err


should add to the errlist and return the errorlist, right?

ecordell · 2019-11-01T14:54:27Z

pkg/sqlite/load.go

+		if err != nil {
+			return err
+		}
+		return tx.Commit()


ecordell · 2019-11-01T14:55:17Z

pkg/sqlite/load.go

+	if err != nil {
+		return err
+	}
+	if !chanHeadRows.Next() {


you need to call chanHeadRows.Close() after you scan

ecordell · 2019-11-01T14:57:05Z

pkg/sqlite/load.go

+		bundles[bundleToUpdate.String] = struct{}{}
+	}
+
+	if len(bundles) > 0 {


this is fine, but I think we can do this loop with one sql query

ecordell · 2019-11-01T14:57:27Z

pkg/sqlite/load.go

+		}
+		csvNames = append(csvNames, csvName)
+	}
+


close rows

ecordell · 2019-11-01T14:58:02Z

pkg/sqlite/load.go

+		return fmt.Errorf("no default channel found for package %s", packageName)
+	}
+	var defaultChannel sql.NullString
+	if err := defaultChannelRows.Scan(&defaultChannel); err != nil {


close defaultChannelRows

ecordell · 2019-11-01T14:58:18Z

pkg/sqlite/load.go

+		return err
+	}
+	bundles := make(map[string]struct{}, 0)
+	for chanBundleRows.Next() {


Adds opm registry command to add bundle content from images to the database. For non-default bundles, once the database has been updated, the bundle and csv field are deleted from all bundles that are not the tip of the default channel of the package being updated. Adds opm registry command to rm a package from the database (removing all owned APIs).

ecordell · 2019-11-01T16:28:23Z

/lgtm

openshift-ci-robot · 2019-11-01T16:28:29Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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-11-01T18:27:36Z

/refresh

ecordell · 2019-11-01T18:27:54Z

/test e2e-aws

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 Sep 26, 2019

openshift-ci-robot requested review from alecmerdler and ecordell September 26, 2019 20:23

openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 26, 2019

gallettilance changed the title ~~[WIP] Adding top level CLI that combines all others~~ Adding top level CLI that combines all others Sep 26, 2019

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 Sep 26, 2019

kevinrizza requested changes Sep 26, 2019

View reviewed changes

gallettilance force-pushed the operator-registry-cli branch from eb27824 to a451f63 Compare September 27, 2019 13:47

jpeeler reviewed Sep 27, 2019

View reviewed changes

ecordell reviewed Sep 27, 2019

View reviewed changes

gallettilance force-pushed the operator-registry-cli branch from a451f63 to 0654051 Compare September 27, 2019 15:51

kevinrizza mentioned this pull request Oct 3, 2019

Initial proposal for mutable operator registry #95

Closed

2 tasks

gallettilance force-pushed the operator-registry-cli branch from 0654051 to d83485b Compare October 10, 2019 17:51

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 10, 2019

gallettilance changed the title ~~Adding top level CLI that combines all others~~ [WIP] Adding top level OPM cli with registry commands Oct 10, 2019

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 Oct 10, 2019

gallettilance force-pushed the operator-registry-cli branch 3 times, most recently from ce583bf to 992c745 Compare October 28, 2019 13:21

anik120 mentioned this pull request Oct 28, 2019

Image digests for referencing bundles #103

Merged

5 tasks

kevinrizza mentioned this pull request Oct 28, 2019

Add index build commands #106

Merged

5 tasks

gallettilance force-pushed the operator-registry-cli branch from 992c745 to a04dc1b Compare October 29, 2019 16:35

gallettilance changed the title ~~[WIP] Adding top level OPM cli with registry commands~~ [WIP] Adding OPM cli with registry add and rm commands Oct 31, 2019

gallettilance force-pushed the operator-registry-cli branch from 82202a2 to 4ffe4bd Compare October 31, 2019 15:10

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2019

gallettilance force-pushed the operator-registry-cli branch from 4ffe4bd to de2dd43 Compare October 31, 2019 15:31

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2019

ecordell reviewed Oct 31, 2019

View reviewed changes

gallettilance force-pushed the operator-registry-cli branch 2 times, most recently from ee5339c to 5375767 Compare November 1, 2019 01:03

ecordell mentioned this pull request Nov 1, 2019

feat(api): return version and skiprange in bundle #113

Merged

5 tasks