feat(cli) disable automatic migrations #2421

subnetmarco · 2017-04-19T21:44:57Z

⚠️ This is a breaking change in how Kong is being started with an empty database, or with missing migrations.

There are two steps to improve the current migrations:

Update the current strategy so that migrations are always non-breaking.
Even if (1) is being implemented, still making sure that migrations are not magically executed when starting Kong since things can always go wrong.

This PR is only trying to fix the (2) second point, by disabling automatic database migrations on kong start or kong restart, unless it's the first time Kong runs and in that case Kong will execute the migrations to initialize the empty database/keyspace.

Migrations now need to be explicitly executed with kong migrations up. The old behavior can be still replicated by adding --run-migrations on kong (re)start.

If --run-migrations is missing, this PR also checks that Kong can effectively run with the existing migrations or not.

Full changelog

Disable automatic database migrations on kong start or kong restart.
Added the optional --run-migrations flag on kong start and kong restart.

Issues resolved

Fix #1692.

TODO

Update website and documentation.

subnetmarco · 2017-04-19T21:51:16Z

@akasetty

p0pr0ck5 · 2017-04-19T22:39:59Z

Thoughts on inverting this logic, and providing a flag to not run migrations automatically? That makes the change less drastic, and doesn't require simpler/new users to take an extra step to keep up to date, while still allowing user with complex deployment/data store environments an avenue to avoid problems.

subnetmarco · 2017-04-19T22:54:46Z

Thoughts on inverting this logic

I disagree - once there is a migration because of a major version update, the upgrade process needs to be explicit and not manual. Then once it's migrated, you will never have to worry about this again because Kong will start normally. We want that extra step.

p0pr0ck5

There is a consistently failing test, not sure how its related, perhaps it just needs adjustment.

brockmiller

This change LGTM -- thanks for adding this safety check 👍

subnetmarco · 2017-04-20T00:40:08Z

Tests now passing.

bungle · 2017-04-21T14:49:16Z

kong/cmd/start.lua

+    if args.run_migrations then
+      assert(dao:run_migrations())
+    end
+    assert(dao:are_migrations_uptodate())


The are_migrations_uptodate will log a message with warn level. Still it seems like here it is a fatal. I would probably move that log statement out of that are_migrations_uptodate and do the logging where it has a context (e.g. here).

If we want this to prevent Kong from starting, it should be logged with possibly > ngx.ERR?

Also do we want this to be a fatal thing? Or should we just start the Kong and wait for the results of what happened to fan.

It is indeed fatal, since the assert will fail and return the error. The error is that migrations are not up to date, the warn just tells which migration is missing (it's a more verbose information). I could log it as verbose or error.

Ok, thanks for explanation. I don't think anything needs to be changed for now.

Instead of throwing the error, I think it would be nice if it would retry for a specified timeout (default 60 seconds?). That would allow orchestration tools to run a single kong migrations up and then schedule Kong containers/nodes using kong start. The containers will then wait before actually starting until migrations are complete, instead of failing.

whups, didnt mean to approve this, just cancel my requested changes since it was fixed

mgiorgio · 2017-04-27T15:51:16Z

Something it'd be great to clarify, especially for new users, is when migrations are supposed to be executed. As far as I understand, Kong starts with an empty schema, creates it and then runs migrations. That sounds a little counterintuitive to me because when you think about "migrating" something, that is typically related to updating to the next major version, not installing.

I agree with @thefosk that running migrations must be explicit when upgrading but I don't think that should be the case when installing Kong from scratch. I think those are two different test cases that should be treated differently from a user perspective.

mgiorgio · 2017-04-28T21:46:55Z

I will share our case to see if it matches anyone else's case. The way we deploy stuff is by creating recipes of what has to be deployed. As a result, deciding whether migrations must be run or not based on a parameter doesn't really work because we need to create one recipe for running migrations, wait until migrations are finished, then create the permanent recipe (without the migrations).
Something you folks could implement to solve this issue is by choosing if migrations must be run by checking for the existence of an arbitrary file. That way, I could normally deploy Kong (even run many nodes!), then go to one of my nodes, create the magic run-migrations file, restart that node (or all nodes, it doesn't really matter), then Kong on start-up will check for that file. Because the file is present, Kong will run migrations and then will delete the file. Consequently, next time Kong is restarted, the file won't be there and Kong won't attempt to run migrations again.

Tieske · 2017-05-01T06:04:09Z

kong/dao/factory.lua

+        local log = require "kong.cmd.utils.log"
+        log.warn(string.format("database is missing migration: (%s) %s", 
+                                module, migration.name))
+        return false, "migrations are not up to date"


Here the error message should probably contain a better user instruction telling them to start kong with the --run-migrations option (on one node only)

Tieske · 2017-05-01T09:00:33Z

CHANGELOG.md

+  `kong restart`. Now they need to be **explicitly** executed **before** Kong is
+  started with `kong migrations up`. You can still use the old behavior by
+  adding the optional `--run-migrations` argument to `kong start` or 
+  `kong restart`, which will automatically runs the migrations before starting


runs ==> run

Tieske · 2017-05-01T09:08:29Z

kong/cmd/start.lua

+    if args.run_migrations then
+      assert(dao:run_migrations())
+    end
+    assert(dao:are_migrations_uptodate())


Instead of throwing the error, I think it would be nice if it would retry for a specified timeout (default 60 seconds?). That would allow orchestration tools to run a single kong migrations up and then schedule Kong containers/nodes using kong start. The containers will then wait before actually starting until migrations are complete, instead of failing.

subnetmarco · 2017-06-07T20:02:31Z

@mgiorgio I understand your point, but either we execute migrations by default, or we disable them by default, but we shouldn't create edge-cases so that "they are always disabled, but.."

The first time you start Kong, you would run it with --run-migrations (because the schema would be empty) and if you forget to do so the error message could be more explanatory if it detects that this is the first migration, ie:

Kong cannot find the appropriate database schema - if this is the first you are running Kong then start it with the --run-migrations flag

or, another way would be to fix the semantics of the command and call the flag --run-db-updates, which assumes Kong will trigger the migrations on the first installation (because there's nothing to update, since it's the first installation), but it will not automatically update the database schema if they have already been initialized before.

mgiorgio · 2017-06-07T20:21:11Z

@thefosk, thanks for your answer. While it's arguable whether mine is an edge-case or not, I see the value on implementing this solution. The concern I still have is that relying on manual intervention for installing and upgrading Kong, will prevent developers from fully automating their deployments.
Is there any plan for going towards a fully automated solution for deployments?
Btw, +1 to --run-db-updates over --run-migrations.

subnetmarco · 2017-06-07T22:10:43Z

I have updated the PR:

On the first execution of Kong, when the database/keyspace is empty, Kong will automatically execute the migrations (with or without --run-migrations).
If the database is not empty, and new migrations need to be executed, Kong will not execute them until --run-migrations is being specified.

subnetmarco · 2017-07-12T09:54:49Z

Regarding this item:

drop auto-migrations on an empty database, as two nodes starting simultaneously will still create conflicts

IMHO it will make a first installation of Kong harder on an empty schema. I agree that multiple nodes could still create conflicts, but since it's a first run those conflicts will basically have no side effect and no data will be involved. At the same time it creates an "exception" in how we are handling migrations, so there's that.

Thoughts?

Tieske · 2017-07-12T23:02:26Z

@marco No more thoughts, we had a lengthy debate, and came to this consensus: remove the magic and add a very explicit/instructive error message.

(even the error message got a lengthy debate)

I fixed the first 2 items on the list, I'll add the schema consistency check tomorrow, then it will be up for review and merge.

outdated

Co-authors who participated to this decision: - Marco Palladino <[email protected]> - Thijs Schreijer <[email protected]> Migrations now must be run via `kong migrations up`. This is done to start spreading good practises regarding migrations, especially for Cassandra clusters, and considering our current migrations can be harmful is run incorrectly. Context ------- When using Cassandra, migrations **must** be run from a single node, whether the keyspace is fresh (new install) or not (upgrade). This is because of the eventual nature of Cassandra itself, and the lack of "lock" or "mutex" mechanism without the use of consistency=ALL flag, which we want to avoid for now. For this reason and because we want to enforce a manual process for major upgrades, we wish to disable automatic migrations until further rewrite of said migrations. Current implementation ---------------------- When Kong starts, we don't run the migrations automatically. - If the migrations are not up-to-date with the version of Kong being started: we error with a helpful message instructing the user on how to run the migrations manually, *from a single node* - When using Cassandra, if the schema consensus is not yet reached, we error out with a helpful error message as well, instructing the user to wait or investigate the cluster (schema consensus should have been waited for during the migrations already, so something could have went wrong if consensus is still not reached) - A `--run-migrations` flag was left, to allow for running migrations from `kong start`. While this is **the pattern we want to get away from**, it is left over as a "backwards-compatible" behavior, but should be considered deprecated. Long term --------- In the future, postgres and Cassandra instances should behave differently: - PostgreSQL: we can run the _initial_ migrations automatically, and via many nodes at the same time, especially if we use some mutex mechanism - Cassandra: we should enforce _initial_ manual migrations, from a *single node*, and eventually consider a mutex mechanism via a consistency=ALL flag. In both cases, we wish _upgrade_ migrations to be safer and manual, to provide no-loss-of-data and no-downtime upgrade paths. This process requires manual steps, or semi-manual. Fix: #1692 From: #2421

subnetmarco force-pushed the feat/disable-auto-migrations branch 3 times, most recently from 74306b5 to 0d25e12 Compare April 19, 2017 21:49

subnetmarco requested a review from thibaultcha April 19, 2017 21:50

subnetmarco force-pushed the feat/disable-auto-migrations branch from 0d25e12 to dff20ea Compare April 19, 2017 22:33

p0pr0ck5 suggested changes Apr 19, 2017

View reviewed changes

subnetmarco force-pushed the feat/disable-auto-migrations branch 2 times, most recently from 5560a1c to 093eb1f Compare April 19, 2017 23:35

brockmiller reviewed Apr 20, 2017

View reviewed changes

subnetmarco force-pushed the feat/disable-auto-migrations branch from 093eb1f to 859ad7b Compare April 20, 2017 00:21

p0pr0ck5 added the pr/status/please review label Apr 20, 2017

bungle reviewed Apr 21, 2017

View reviewed changes

p0pr0ck5 previously approved these changes Apr 25, 2017

View reviewed changes

Tieske previously requested changes May 1, 2017

View reviewed changes

Tieske reviewed May 1, 2017

View reviewed changes

p0pr0ck5 added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels May 3, 2017

thibaultcha added pr/status/do not merge (to discuss) and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels May 30, 2017

subnetmarco added the pr/status/please review label Jun 7, 2017

Tieske added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/do not merge (to discuss) labels Jul 10, 2017

Tieske force-pushed the feat/disable-auto-migrations branch from 16f6e24 to ae2af81 Compare July 12, 2017 22:54

Tieske force-pushed the feat/disable-auto-migrations branch from 034bf3e to 1ba9177 Compare July 13, 2017 10:15

Tieske added pr/status/please review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Jul 13, 2017

Tieske assigned Tieske and unassigned shashiranjan84 Jul 13, 2017

Tieske force-pushed the feat/disable-auto-migrations branch 5 times, most recently from c1a1b43 to 64bccf1 Compare July 20, 2017 09:47

chore(deps) bump lua-cassandra to 1.2.3

9bd83c0

thibaultcha force-pushed the feat/disable-auto-migrations branch 3 times, most recently from ce9b10b to 29a6de4 Compare July 21, 2017 00:22

thibaultcha force-pushed the feat/disable-auto-migrations branch from 29a6de4 to b3e05b3 Compare July 21, 2017 20:10

thibaultcha force-pushed the feat/disable-auto-migrations branch from b3e05b3 to 0771335 Compare July 22, 2017 00:51

thibaultcha approved these changes Jul 22, 2017

View reviewed changes

thibaultcha added pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) and removed pr/status/please review labels Jul 22, 2017

thibaultcha merged commit bae8f8c into next Jul 22, 2017

thibaultcha deleted the feat/disable-auto-migrations branch July 22, 2017 03:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli) disable automatic migrations #2421

feat(cli) disable automatic migrations #2421

subnetmarco commented Apr 19, 2017 •

edited

Loading

subnetmarco commented Apr 19, 2017

p0pr0ck5 commented Apr 19, 2017

subnetmarco commented Apr 19, 2017

p0pr0ck5 left a comment

brockmiller left a comment

subnetmarco commented Apr 20, 2017

bungle Apr 21, 2017 •

edited

Loading

bungle Apr 21, 2017

subnetmarco Apr 21, 2017

bungle Apr 21, 2017

Tieske May 1, 2017

mgiorgio commented Apr 27, 2017

mgiorgio commented Apr 28, 2017

Tieske May 1, 2017

Tieske May 1, 2017

Tieske May 1, 2017

subnetmarco commented Jun 7, 2017 •

edited

Loading

mgiorgio commented Jun 7, 2017

subnetmarco commented Jun 7, 2017

subnetmarco commented Jul 12, 2017 •

edited

Loading

Tieske commented Jul 12, 2017

feat(cli) disable automatic migrations #2421

feat(cli) disable automatic migrations #2421

Conversation

subnetmarco commented Apr 19, 2017 • edited Loading

Full changelog

Issues resolved

TODO

subnetmarco commented Apr 19, 2017

p0pr0ck5 commented Apr 19, 2017

subnetmarco commented Apr 19, 2017

p0pr0ck5 left a comment

Choose a reason for hiding this comment

brockmiller left a comment

Choose a reason for hiding this comment

subnetmarco commented Apr 20, 2017

bungle Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

bungle Apr 21, 2017

Choose a reason for hiding this comment

subnetmarco Apr 21, 2017

Choose a reason for hiding this comment

bungle Apr 21, 2017

Choose a reason for hiding this comment

Tieske May 1, 2017

Choose a reason for hiding this comment

mgiorgio commented Apr 27, 2017

mgiorgio commented Apr 28, 2017

Tieske May 1, 2017

Choose a reason for hiding this comment

Tieske May 1, 2017

Choose a reason for hiding this comment

Tieske May 1, 2017

Choose a reason for hiding this comment

subnetmarco commented Jun 7, 2017 • edited Loading

mgiorgio commented Jun 7, 2017

subnetmarco commented Jun 7, 2017

subnetmarco commented Jul 12, 2017 • edited Loading

Tieske commented Jul 12, 2017

subnetmarco commented Apr 19, 2017 •

edited

Loading

bungle Apr 21, 2017 •

edited

Loading

subnetmarco commented Jun 7, 2017 •

edited

Loading

subnetmarco commented Jul 12, 2017 •

edited

Loading