Database migration initialization by kevinrizza · Pull Request #90 · operator-framework/operator-registry

kevinrizza · 2019-09-27T13:51:06Z

This commit introduces a day 0 set of building blocks to implement a
database migration framework into the sqlite database. Adding the
following changes:

Introduce the golang-migrate project as a dependency and vendor it
Introduce a db_migrations folder with an initial no-op migration to
bootstrap the initial database version for newly created databases
Update the initialization of the database when creating the initial
schema to hard set the database migration version on db creation
before loading any data

This commit does not introduce a method of actually running future
migrations against existing databases -- that is left for future
implementation based on this groundwork when a set of commands that
updates the database in place exists and requires the migration.

The intention of this commit is also to ensure that future initialized
registry databases have an initial version that can be used for
migrations in future releases.

ecordell · 2019-09-27T15:40:45Z

+
+const (
+	// Hardcoded path where the db_migrations live
+	defaultMigrationsPath = "./pkg/sqlite/db_migrations"


doesn't this mean that migrations only work if you have the operator-registry repo check out locally?

Yep, that is true. Regardless you at the very least need the migrations folder. For the initializer, this is currently only run today from inside a container image that has the relevant files copied into it. We could also add a flag to the initializer that takes the migration folder as input, but in either case those files will need to be pulled down.

We could also, in theory, save the files themselves as source code so they could be compiled as part of the binary and then written to a directory before being read. That would make this more portable if we found that significantly valuable.

A quick google led me to this: https://github.com/gobuffalo/packr - if this looks reasonable to add to this PR quickly (say <1hr?) can you go ahead and do that? I think the UX is much better if we don't have to ship migration files around.

If it looks like it would take longer than that, we don't have to bother with it now, and we can revisit the idea in the future.

@ecordell I don't really like this tool because it makes a bunch of assumptions about the underlying filesystem of your files and also requires that you wrap go build inside their packr command. I just messed around with https://github.com/go-bindata/go-bindata and it seems like it's a little bit better for our specific use case -- it just generates a .go file with compressed binary versions of the files. We still need to use a binary tool to generate them, but we should be able to just add something to the makefile to download and run it when we need to generate the files.

I'm going to update this PR in a little bit with a second commit with an implementation that includes that.

Awesome, thanks for finding that!

ecordell · 2019-10-03T13:31:06Z

 	}

-	sqlLoader, err := sqlite.NewSQLLiteLoader(dbName)
+	sqlLoader, err := sqlite.NewSQLLiteLoader(dbName, "")


We use some different patterns in other places that I think could clean this up.

Either a builder with good defaults

// uses the compiled migrations sqlite.NewSQLLiteLoader(dbName) // for tests sqlite.NewSQLLiteLoader(dbName).WithMigrator(*testMigrator)

or the variadic option pattern

sqlite.NewSQLLiteLoader(options.WithDBName(dbName), options.WithMigrator(default))

Yep, this is a good point. I went with the trivial implementation instead of using a builder because the constructor currently has actual parsing logic that requires the migrator to exist since there is currently no distinction between calling NewSQLLiteLoader and actually initializing the database -- I was attempting to retain that step as an atomic function call instead of splitting it into several steps.

But using the option pattern is a really good idea, I'll try to make this more digestible for the default case that way.

ecordell · 2019-10-03T13:31:43Z

 		t.Run(tt.description, func(t *testing.T) {
 			db := fmt.Sprintf("%d.db", rand.Int())
-			store, err := NewSQLLiteLoader(db)
+			store, err := NewSQLLiteLoader(db, "./db_migrations")


We should probably be testing the compiled migrations, right?

We do further down in the file:

migrator, err := NewSQLLiteMigrator(store.db, "")

But actually testing the values of the default migrations would involve either keeping the latest version in a configuration file or actually parsing the version. At that point, we would be maintaining test code that provides no functionality except to execute tests: IMO that is unnecessary overhead and technical debt.

That's why I implemented all the value checking using a set of test migrations that can be statically defined and their values referenced directly in the tests, and the actual check of the real data is just "does this throw an error when parsed".

Do you feel like there is significant value to be gained from ensuring something I'm missing?

Ah, just realized this is for load_test.go, not the new test file I added. Yeah in this case you are right this should be updated to just use the correct default initialization.

kevinrizza · 2019-10-07T16:22:34Z

@ecordell Updated. ptal

ecordell

Made a couple of bookkeeping comments but this looks good!

If you're tired of looking at migrations, I think we can go ahead and merge this once the remaining comments are addressed.

...but, there's still something that's slightly bugging me. The current approach uses a framework that assumes the inputs are files, so we use go-bindata to get around that.

But mostly, migrations are just a bit of sql that could very easily fit in a constant in our binary. Additionally, some migrations may require non-sql code to run as well (doing backfills, etc) and I don't see a great way to do that with this.

For the most part, all we need is:

A table that tracks the current version
Some abstraction (in go) around a migration that allows you to go up or down (and most of those are a single sql.Execute)
An ordered list of those abstractions to walk through

I went back to the migrate lib you pulled in and found this as well: golang-migrate/migrate#15

Which led me to this package:

https://github.com/fnproject/fn/tree/master/api/datastore/sql/migratex

It seems simple enough to pull in or to write something similar ourselves.

What do you think about this?

ecordell · 2019-10-12T17:05:33Z

 	go build $(MOD_FLAGS) $(extra_flags) -o $@ ./cmd/$(shell basename $@)

-build: clean $(CMDS)
+build: clean generate-migration-bundle $(CMDS)


I don't think we want this as part of the build command so that CI will catch when we don't correctly commit the generated migration bundle to git.

ecordell · 2019-10-12T17:07:43Z

 clean:
 	@rm -rf ./bin
-
+	@rm -f ./pkg/sqlite/migrations.go


like the rest of our codegen, we probably want to go ahead and commit these artifacts.

ecordell · 2019-10-12T17:08:41Z

 	github.com/grpc-ecosystem/grpc-health-probe v0.2.1-0.20181220223928-2bf0a5b182db
 	github.com/imdario/mergo v0.3.7 // indirect
 	github.com/mattn/go-sqlite3 v1.10.0
+	github.com/operator-framework/operator-lifecycle-manager v3.11.0+incompatible // indirect


Where did this come from? We recently did some work to remove the dependency on OLM.

(also this is picking up the 3.11 version which is like 0.7.0 I think)

ecordell · 2019-10-12T17:09:05Z

@@ -0,0 +1,23 @@
+package sqlite
+
+type DbOptions struct {


ecordell · 2019-10-14T15:27:03Z

Discussed offline:

We'll stick with the current implementation and revisit in the future if/when we need migrations that require more than SQL.

Just need those dependencies fixed up and we can merge @kevinrizza !

ecordell · 2019-10-15T19:48:06Z

 var _ registry.Load = &SQLLoader{}

-func NewSQLLiteLoader(outFilename string) (*SQLLoader, error) {
+func NewSQLLiteLoader(outFilename, migrationsPath string) (*SQLLoader, error) {


Did I hallucinate a change to this API? I thought this became variadic with config options?

you did not, i rebased poorly :) latest should contain the right bits.

This commit introduces a day 0 set of building blocks to implement a database migration framework into the sqlite database. Adding the following changes: - Introduce the `golang-migrate` project as a dependency and vendor it - Introduce a `db_migrations` folder with an initial no-op migration to bootstrap the initial database version for newly created databases - Update the initialization of the database when creating the initial schema to hard set the database migration version on db creation before loading any data This commit does not introduce a method of actually running future migrations against existing databases -- that is left for future implementation based on this groundwork when a set of commands that updates the database in place exists and requires the migration. The intention of this commit is also to ensure that future initialized registry databases have an initial version that can be used for migrations in future releases.

Use the go-bindata tool to generate a golang migrations file to compile the content of the db_migrations folder into the bundle. This ensures that the compiled binaries are portable and do not require around the migrations themselves along with the binary. Also updated the makefile to pull the go-bindata file and generate the `migration.go` file as part of `make build`

ecordell · 2019-10-15T20:08:01Z

/lgtm

openshift-ci-robot · 2019-10-15T20:08:05Z

[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

Details

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

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 27, 2019

openshift-ci-robot requested review from dinhxuanvu and ecordell September 27, 2019 13:51

ecordell reviewed Sep 27, 2019

View reviewed changes

kevinrizza force-pushed the db-migration-scaffold branch 2 times, most recently from 3545430 to bc4ce2a Compare September 30, 2019 17:29

ecordell reviewed Oct 3, 2019

View reviewed changes

kevinrizza force-pushed the db-migration-scaffold branch from bc4ce2a to 3b57071 Compare October 7, 2019 16:22

ecordell reviewed Oct 12, 2019

View reviewed changes

kevinrizza force-pushed the db-migration-scaffold branch from 3b57071 to 4621b4f Compare October 15, 2019 12:25

ecordell mentioned this pull request Oct 15, 2019

Image digests for referencing bundles #99

Closed

5 tasks

ecordell reviewed Oct 15, 2019

View reviewed changes

kevinrizza added 2 commits October 15, 2019 15:55

kevinrizza force-pushed the db-migration-scaffold branch from 4621b4f to fcf1ff1 Compare October 15, 2019 20:01

openshift-ci-robot assigned ecordell Oct 15, 2019

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

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 15, 2019

openshift-merge-robot merged commit b58d8a6 into operator-framework:master Oct 15, 2019

Conversation

kevinrizza commented Sep 27, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinrizza Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinrizza commented Oct 7, 2019

Uh oh!

ecordell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ecordell commented Oct 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ecordell commented Oct 15, 2019

Uh oh!

openshift-ci-robot commented Oct 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kevinrizza Oct 3, 2019 •

edited

Loading