Apply migrations when tink-server boots #296

gianarb · 2020-09-17T13:29:39Z

Description

It will be nice to have the tink-server capable of applying migrations
if necessary. The idea to self-apply migrations will may look unusual
but it is an easy way to distribute software capable of migrating
database schema without having to run any external procedures.

Why is this needed

Fixes: #295

How Has This Been Tested?

$ docker run -d -it -e  POSTGRES_PASSWORD=tinkerbell -e POSTGRES_USER=tinkerbell -p 5432:5432 postgres
# PGPASSWORD=tinkerbell PGUSER=tinkerbell PGSSLMODE=disable PACKET_VERSION=v1 PACKET_ENV=ga ROLLBAR_TOKEN=ignored go run cmd/tink-server/main.go
{"level":"info","ts":1600772721.3009,"caller":"tink-server/main.go:30","msg":"starting version devel","service":"github.com/tinkerbell/tink"}
migrations  1
records  0
{"level":"info","ts":1600772721.309925,"caller":"tink-server/main.go:54","msg":"Your database schema is not up to date. Please apply migrations running tink-server with env var ONLY_MIGRATION set.","service":"github.com/tinkerbell/tink"}
{"level":"info","ts":1600772721.310011,"caller":"metrics/metrics.go:58","msg":"initializing label values","service":"github.com/tinkerbell/tink"}

As suggested you should run tink-server with ONLY_MIGRATION=true

$ ONLY_MIGRATION=true PGPASSWORD=tinkerbell PGUSER=tinkerbell PGSSLMODE=disable PACKET_VERSION=v1 PACKET_ENV=ga ROLLBAR_TOKEN=ignored go run cmd/tink-server/main.go
{"level":"info","ts":1600772853.779405,"caller":"tink-server/main.go:30","msg":"starting version devel","service":"github.com/tinkerbell/tink"}
{"level":"info","ts":1600772853.779805,"caller":"tink-server/main.go:40","msg":"Applying migrations. This process will end when migrations will take place.","service":"github.com/tinkerbell/tink"}
{"level":"info","ts":1600772853.819854,"caller":"tink-server/main.go:45","msg":"Running migrations if necessary","service":"github.com/tinkerbell/tink","num_applied_migrations":1}

How are existing users impacted? What migration steps/scripts do we need?

They are not impacted by this change.

Checklist:

I have:

updated the documentation and/or roadmap (if required)
added unit or e2e tests
provided instructions on how to upgrade

codecov · 2020-09-17T13:33:38Z

Codecov Report

Merging #296 into master will increase coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #296      +/-   ##
==========================================
+ Coverage   22.90%   22.91%   +0.01%     
==========================================
  Files          15       15              
  Lines        1275     1274       -1     
==========================================
  Hits          292      292              
+ Misses        964      963       -1     
  Partials       19       19

Impacted Files	Coverage Δ
grpc-server/grpc_server.go	`0.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b014b9...1381963. Read the comment docs.

gianarb · 2020-09-17T13:34:34Z

This is just a PoC to see what other people think about it.

Ideally, I would like to add something in the tink-cli or somewhere that can help us to generate a migration file.go and maybe that can also update the actual slice of migrations.
The original CLI tool supports a status command, I would like to expose the same capability but via HTTP endpoint:

(original status command)

$ sql-migrate status
+---------------+-----------------------------------------+
|   MIGRATION   |                 APPLIED                 |
+---------------+-----------------------------------------+
| 1_initial.sql | 2014-09-13 08:19:06.788354925 +0000 UTC |
| 2_record.sql  | no                                      |
+---------------+-----------------------------------------+

Rollback is still an open question 👍

db/db.go

grpc-server/grpc_server.go

mmlb · 2020-09-18T17:48:56Z

db/migration/202009171251-init-database.go

+        , metadata JSONB
+        , data JSONB
+);`},
+		Down: []string{"DROP DATABASE tinkerbell"},


I um worry about this. I think every thing I've ever read about Down migrations is always "no, don't". Of course I can't find anything on this atm, but I'll keep looking :D.

In general I think its just always easier to think about failing-forward instead of reverting. iirc its less foot-gunny.

Right now there is no logic that applies rollback :) :) That's one of the topic I would like to discuss. We can say: "you rollback is one migration forward"

So lets drop this Down ;)

mmlb · 2020-09-18T17:53:25Z

db/migration/202009171251-init-database.go

+func Get202009171251() *migrate.Migration {
+	return &migrate.Migration{
+		Id: "123",


I think for minimizing context it would be best to have the Id match the name?

It would be great to have a sed or similar check that Id matches Get.*.

mmlb · 2020-09-18T17:55:51Z

cmd/tink-server/main.go

+	tinkDB := db.Connect(logger)
+	numAppliedMigrations, err := tinkDB.Migrate()
+	if err != nil {
+		panic(err)
+	}
+	log.With("num_applied_migrations", numAppliedMigrations).Info("Running migrations if necessary")


We've found out the hard way that its best to allow for migrations to run separately from normal service startup. This way once we have multiple tinks running an upgrade won't have +1 tinks trying to migrate the db.

It is a good point that I didn't think about. Ideally, good migrations should be applicable more than once without side effect I presume but still... I see your point.

This PR has a very specific title :P so if we don't want to do that, I will close this in favor of something else. I think there are a good amount of people that will stay benefit from having a managed migration process and this will avoid for them the extra work to apply migrations by themself when they update.

I presume for the other people we can have a flag=--disable-migration? Or do you think an external workflow will make everyone happy enough?

I am just doing an exercise here. But, we can embed the same migration function we do here:

numAppliedMigrations, err := tinkDB.Migrate() if err != nil { panic(err) }

As part of the tink-cli itself, so who wants to manage migration externally can disable migration in tink-server and they can apply it when they like via tink-cli. Not sure if it has sense or if it is just useless complexity.

Well if you start up 2 tink-servers at the same time they may both try to do some costly migration and take up db time and only one will succeed, the other will fail and restart and then continue. It would stay correct, but waste time/db resources.

Having --migrate-only would be better imo, as it can be a separate step in the deployment pipeline and would exit when done.

Do you think that flag should stay in tink-server? I like that! I will check if the library supports something like a dry run that checks for migrations but does not apply them. In this way I can run it at start and notify the user that there are migration not applied yet

Done @mmlb and I have updated the PR description with the new workflow

yeah the dry run would be pretty nice imo.

cmd/tink-server/main.go

mmlb · 2020-09-22T15:14:54Z

db/db.go

+	return migrate.Exec(t.instance, "postgres", migration.GetMigrations(), migrate.Up)
+}
+
+func (t *TinkDB) CheckAvailableMigrations() (int, error) {


Available seems like the wrong word to me... Necessary? Required?

mmlb · 2020-09-22T15:14:56Z

db/db.go

+	fmt.Println("migrations ", len(migrations))
+	fmt.Println("records ", len(records))


mmlb · 2020-09-22T15:17:06Z

db/migration/202009171251-init-database.go

+		Up: []string{`
+SELECT 'CREATE DATABASE tinkerbell' WHERE NOT EXISTS (SELECT FROM pg_database WHERE datname = 'tinkerbell');
+
+SET ROLE tinkerbell;


this should probably happen before the SELECT 'CREATE...' if possible.

It will be nice to have the tink-server capable of applying migrations if necessary. The idea to self-apply migrations will may look unusual but it is an easy way to distribute software capable of migrating database schema without having to run any external procedures. ```console $ docker run -d -it -e POSTGRES_PASSWORD=tinkerbell -e POSTGRES_USER=tinkerbell -p 5432:5432 postgres $ PGPASSWORD=tinkerbell PGUSER=tinkerbell PGSSLMODE=disable PACKET_VERSION=v1 PACKET_ENV=ga ROLLBAR_TOKEN=ignored go run cmd/tink-server/main.go {"level":"info","ts":1600349232.375571,"caller":"tink-server/main.go:31","msg":"starting version devel","service":"github.com/tinkerbell/tink"} {"level":"info","ts":1600349232.419096,"caller":"tink-server/main.go:42","msg":"Running migrations if necessary","service":"github.com/tinkerbell/tink","num_applied_migrations":1} ``` Second time you run it: ``` $ PGPASSWORD=tinkerbell PGUSER=tinkerbell PGSSLMODE=disable PACKET_VERSION=v1 PACKET_ENV=ga ROLLBAR_TOKEN=ignored go run cmd/tink-server/main.go [12/78]{"level":"info","ts":1600349264.877263,"caller":"tink-server/main.go:31","msg":"starting version devel","service":"github.com/tinkerbell/tink"} {"level":"info","ts":1600349264.8883042,"caller":"tink-server/main.go:42","msg":"Running migrations if necessary","service":"github.com/tinkerbell/tink","num_applied_migrations":0} ``` Signed-off-by: Gianluca Arbezzano <[email protected]>

Reverts #296 I have to rever this PR because we merged it too early. I don't have a good story yet for what concerns "how to apply migration" in sandbox or with docker compose.

gianarb requested a review from mmlb September 17, 2020 13:29

mmlb suggested changes Sep 17, 2020

View reviewed changes

db/db.go Show resolved Hide resolved

grpc-server/grpc_server.go Show resolved Hide resolved

gianarb requested a review from mmlb September 18, 2020 11:44

mmlb suggested changes Sep 18, 2020

View reviewed changes

gianarb requested a review from mmlb September 22, 2020 14:50

mmlb suggested changes Sep 22, 2020

View reviewed changes

gianarb requested a review from mmlb September 28, 2020 07:13

mmlb approved these changes Sep 28, 2020

View reviewed changes

mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Sep 28, 2020

mergify bot added 2 commits September 28, 2020 15:32

Merge branch 'master' into feature/migrate

61ad898

Merge branch 'master' into feature/migrate

1381963

mergify bot merged commit 11bd870 into tinkerbell:master Sep 28, 2020

gianarb mentioned this pull request Sep 28, 2020

Revert "Apply migrations when tink-server boots" #310

Merged

mmlb removed the ready-to-merge Signal to Mergify to merge the PR. label Jan 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply migrations when tink-server boots #296

Apply migrations when tink-server boots #296

gianarb commented Sep 17, 2020 •

edited

Loading

codecov bot commented Sep 17, 2020 •

edited

Loading

gianarb commented Sep 17, 2020 •

edited

Loading

mmlb Sep 18, 2020

gianarb Sep 21, 2020

mmlb Sep 21, 2020

gianarb Sep 22, 2020

mmlb Sep 18, 2020

gianarb Sep 21, 2020

mmlb Sep 21, 2020

mmlb Sep 18, 2020

gianarb Sep 21, 2020 •

edited

Loading

mmlb Sep 21, 2020

gianarb Sep 22, 2020

gianarb Sep 22, 2020

mmlb Sep 22, 2020

mmlb Sep 22, 2020

mmlb Sep 22, 2020

mmlb Sep 22, 2020

gianarb Sep 28, 2020

		fmt.Println("migrations ", len(migrations))
		fmt.Println("records ", len(records))

Apply migrations when tink-server boots #296

Apply migrations when tink-server boots #296

Conversation

gianarb commented Sep 17, 2020 • edited Loading

Description

Why is this needed

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

codecov bot commented Sep 17, 2020 • edited Loading

Codecov Report

gianarb commented Sep 17, 2020 • edited Loading

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

gianarb Sep 21, 2020 • edited Loading

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

Choose a reason for hiding this comment

gianarb commented Sep 17, 2020 •

edited

Loading

codecov bot commented Sep 17, 2020 •

edited

Loading

gianarb commented Sep 17, 2020 •

edited

Loading

gianarb Sep 21, 2020 •

edited

Loading