Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feat/migrations] split plugins migrations #443

Merged
merged 8 commits into from
Aug 21, 2015
Merged

Conversation

thibaultcha
Copy link
Member

Proper architecture and CLI update for plugin-specific migrations. This
is the first step in separating the plugins out of the core repo as
planed for 0.5.0. It is related to #93.

⚠️ This is a breaking change between 0.4 and 0.5.

Changes

This is implemented by using the same table as before
(schema_migrations) to keep track of the executed migrations, except
that each plugin and the core itself all have their own row.

For simplifications, migrations (plugins or core) now live in a single
file. The database folder disappeared for core and the migration lives
in the DAO. Plugins have migrations in a migration folder, named after
the type of the database (cassandra.lua currently).

For now, only the keyauth plugin has its own migrations.

CLI

The kong migrations up|down commands slightly changed. It takes a -t
parameter to specify which migrations to run (core or a plugin name) and
is running all migrations by default. kong migrations list lists all
executed migration for the core and all plugins.

Summary
  • New tests (integration)
  • Old unit tests were removed
  • database was removed
  • kong migrations slightly changed

@thibaultcha thibaultcha added dao pr/wip A work in progress PR opened to receive feedback labels Aug 1, 2015
@thibaultcha thibaultcha force-pushed the plugins-migrations branch 6 times, most recently from 9074f84 to 0a6d09b Compare August 6, 2015 22:11
@thibaultcha
Copy link
Member Author

Because this is a breaking change to the underlying datastore, now is a good time to:

  • Apply any heavy change on the schema if necessary
  • Rename plugins to be dasherized ("request-transformer")

Of course users will need to migrate to this new setup, should we provide a script to do that?

@thibaultcha thibaultcha added NEEDS REVIEW and removed pr/wip A work in progress PR opened to receive feedback labels Aug 8, 2015
@thibaultcha
Copy link
Member Author

Ping @thefosk @ahmadnassri previous comment is of major importance.

@subnetmarco
Copy link
Member

If this is a breaking change we should also include all the other major changes that are in the pipeline. But I would merge this into master after 0.4.2 has been released.

@ahmadnassri
Copy link
Contributor

  • release a patch version with smaller changes first (get those out of the way)
  • 👍 standardizing plugin naming
  • we could avoid migration problems by major version bump + standalone script to "migrate" rather than "upgrade"?
  • otherwise it will have to be a rolling migration with existing migration setup

@ahmadnassri
Copy link
Contributor

another option is an export / import script, that can export data into JSON, then use the same JSON to re-import with HTTP admin API (rather than cql)

@thibaultcha
Copy link
Member Author

standalone script to "migrate" rather than "upgrade"?

Don't get that part

@ahmadnassri
Copy link
Contributor

current setup is labeled "migration" but it's more of an "upgrade" of schema changes on the same system / db

where "migrate" usually indicates moving from one system to another, or one db cluster to another ... synonymous with "moving" data

@ahmadnassri
Copy link
Contributor

so a "migration script" treats old version vs. new version as separate systems and "moves" data between them using a full reconfiguration & setup (new IDs) so it would be agnostic to schema changes

@ahmadnassri
Copy link
Contributor

which would also be useful to have anyways regardless

@subnetmarco
Copy link
Member

To make it easier we should definitely write a script to migrate old data into the new format. It can also be a bash/python/lua script that moves data from an old cassandra to the new cassandra with the new schema.

@thibaultcha 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 NEEDS REVIEW labels Aug 13, 2015
@thibaultcha
Copy link
Member Author

Should I start writing a script for the migration? For now, the script can modify the content of the schema_migrations table (the only thing that changes for now).

If we decide to change the plugins names, the script can also handle it.

If so, what should I write it in? I could write it in Lua or Python (not sure if all instances have Lua installed @thefosk?). In Python users could just pip install cassandra-driver, point to their Kong config file (YAML) and that would be it.

@subnetmarco
Copy link
Member

Python maybe it's more portable?

Would the script also be able to change the plugin configuration values? For example the ratelimiting went from value.limit=20&value.period=second to value.second=20. Now I keep retro compatibility in the code, but it would be cleaner to migrate the old configuration to the new one.

@thibaultcha
Copy link
Member Author

Here is a Python script to migrate to 0.5.0 with a cluster already migrated to 0.4.2: https://gist.github.com/thibaultCha/87bbfe2831e035274232

  • It can either be ran before upgrading to Kong 0.5.0.
  • If Kong was already upgraded to 0.5.0, it can be run after to purge the database of old values.

One of its limitations is that it does not support different ports for each node (like Kong does). All hosts must listen on the same port. Currently it only supports port 9042 since the port property has been removed from the configuration file in 0.4.2. I'll introduce an argument for the port.


We can add more steps if we decide to rename the plugins and to change some other values (like the ratelimiting values).

+1 to do both of these things ^ ? Feedback on the script? Should this script live in this repo or should it just be a gist? @thefosk @ahmadnassri

@ahmadnassri
Copy link
Contributor

could we not compile the script into a binary? (so its not language dependent)

@ahmadnassri
Copy link
Contributor

Should this script live in this repo or should it just be a gist?

safe bet that we might have need in the future for similar, if not exact scenario (v2.0, v15.0, etc ..) so might as well be a repo (kong-migrations) with script named after the source-target combination they serve?

@ahmadnassri
Copy link
Contributor

One of its limitations is that it does not support different ports for each node (like Kong does). All hosts must listen on the same port.

but why would that matter? the script only needs to run on one specific node, and the changes apply across the board ... no?

@thibaultcha
Copy link
Member Author

could we not compile the script into a binary? (so its not language dependent)

I'm really not following here?

safe bet that we might have need in the future for similar, if not exact scenario (v2.0, v15.0, etc ..) so might as well be a repo (kong-migrations) with script named after the source-target combination they serve?

I feel like this might be overkill for now though. Let it just be a script that can be easily changed (I wrote the functions with that in mind).

but why would that matter? the script only needs to run on one specific node, and the changes apply across the board ... no?

I wanted it to just be a drop-in. You could run it by just giving your Kong configuration. But the configuration might be specifying different ports for each node and the script would just not work. Basically one would have to edit the config file, or the script should take contact_points and a port as arguments

@thibaultcha
Copy link
Member Author

We can add more steps if we decide to rename the plugins and to change some other values (like the ratelimiting values).

This too ^ Should it do those things too?

@ahmadnassri
Copy link
Contributor

could we not compile the script into a binary? (so its not language dependent)

wrap the script in an executable binary, so users wont have to pip install anything.

@ahmadnassri
Copy link
Contributor

as for naming convention: kong-plugin-xyz

makes it easy to search and list in github... matches our kong-dist-xyz too

any conflict with luarocks in doing that?

@thibaultcha thibaultcha mentioned this pull request Aug 17, 2015
@thibaultcha
Copy link
Member Author

See #480 for plugins renaming.

@subnetmarco
Copy link
Member

I believe the script should live in this repository.

Proper architecture and CLI update for plugin-specific migrations. This
is the first step in separating the plugins out of the core repo as
planed for 0.5.0. It is related to #93.

This is implemented by using the same table as before
(`schema_migrations`) to keep track of the executed migrations, except
that each plugin and the core itself all have their own row.

For simplifications, migrations (plugins or core) now live in a single
file. The `database` folder disappeared for core and the migration lives
in the DAO. Plugins have migrations in a `migration` folder, named after
the type of the database (`cassandra.lua` currently).

For now, only the keyauth plugin has its own migrations.

The `kong migrations up|down` commands slightly changed. It takes a `-t`
parameter to specify which migrations to run (core or a plugin name) and
is running all migrations by default. `kong migrations list` lists all
executed migration for the core and all plugins.

- Needs tests (integration)
- Old unit tests were removed
- `database` was removed
- `kong migrations` slightly changed
@thibaultcha
Copy link
Member Author

Here are the instructions to run the script but I don't know where to store the actual script. In this repo under a new scripts/ folder, but it is not supposed to be part of Kong, it is only run once by users, and after a short period of time should be removed from the repo. I also feel like it should be easily downloadable, and in a git repo it is not so easy.

We could put it in a gist, under Mashape? In another repo like @ahmadnassri suggested is still kind of a pain for users.

@thibaultcha thibaultcha removed the dao label Aug 21, 2015
thibaultcha added a commit that referenced this pull request Aug 21, 2015
[feat/migrations] split plugins migrations
@thibaultcha thibaultcha merged commit 6cc0d18 into master Aug 21, 2015
@thibaultcha thibaultcha deleted the plugins-migrations branch August 21, 2015 01:41
ctranxuan pushed a commit to streamdataio/kong that referenced this pull request Aug 25, 2015
[feat/migrations] split plugins migrations

Former-commit-id: c60894e8f384d6181e5aac99c62ec0fbfd632592
@thibaultcha thibaultcha mentioned this pull request Aug 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants