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

Auto migration using flyway #179

Draft
wants to merge 1 commit into
base: 5.0
Choose a base branch
from
Draft

Conversation

rishabhpoddar
Copy link
Contributor

@rishabhpoddar rishabhpoddar commented Jan 3, 2024

Summary of change

Currently users have to manually run SQL scripts for migration.
A solution would be to use Flyway for automatic database migration at Runtime. This will make it easier to migrate and track migration changes.

Technical solution

Flyway migrations will be included in the postgresql-plugin:

Directory structure:

|- /db/
|- /migration/
|- /access_token_signing_key_dynamic_false
|- /access_token_signing_key_dynamic_true
|- /common

The reason why we have 3 directory for migration is because some migrations are conditionally applied or skipped. Therefore, when I am building the location where Flyway searches for migration scripts, I can add or remove them from location. Flyway also offers other solutions to solve such situations. One of them would be custom placeholders, but more documentation will be required to understand if placeholders are a feasible solution for this case.

The default naming convention for migration scripts is as follows: V{version}__{description}.sql . The {version} is used to order the scripts, while the description is there to give some context on the migration. Java migrations are also accepted.

Packaging:
The new directory structure will also have to be present inside the postgresql-plugin.jar. By default it was not. So I had to update the build.gradle jar task.

Implementation details:

When will it be run?

  • Flyway migration will be run at Runtime, only once, after the user starts core application.

How to run migration for users that have manually run migration?

  • Users who were utilizing the Core before the introduction of the Flyway migration feature have already been manually running migration. But Flyway does not have details about this, therefore we will need to set a starting point for database migration. For this we will use Flyway baseline feature. To calculate what will be the baseline we have who was the last migration run. Some migrations contain multiple SQL queries, but we can assume if one of them was successfully run, the entire migration was successful.

Will this feature work by default?

  • This feature could be disabled by default for the sake of consistency for existing users. But they will be notified about this new feature, so if they decide to do this, they will only have to change the configuration to enable it.

What is the expected downtime during migration?

  • When a new core version is released, the user will have to stop the Core that was running with an older version and start the Core with a newer version

Will there be any code changes required in Core?

  • Yes, because there are migrations that will be skipped if a configuration if Core is present. Core will have to pass that value to plugin.

Related issues / prs:

Test Plan

TODO

Documentation changes

TODO

Checklist for important updates

  • Changelog has been updated
  • pluginInterfaceSupported.json file has been updated (if needed)
  • Changes to the version if needed
    • In build.gradle
  • Had installed and ran the pre-commit hook
  • If there are new dependencies that have been added in build.gradle, please make sure to add them in implementationDependencies.json.
  • Issue this PR against the latest non released version branch.
    • To know which one it is, run find the latest released tag (git tag) in the format vX.Y.Z, and then find the latest branch (git branch --all) whose X.Y is greater than the latest released tag.
    • If no such branch exists, then create one from the latest released branch.
  • How will our SaaS use this? Cause right now, when moving one user from one core version to another, we first spin up the new core connected to their existing db, and then change the dns. So now, if the new core does migration on startup automatically, then it may break the existing core before the dns propagation is done

* Initial configuration for Flyway

* Use flyway migration at runtime and also calculate baseline dynamically

* Remove Flyway plugin configuration

* Replace dummy migration with migrations from CHANGELOG

* Replace dummy baseline script

* Removed all migration written in sql and replace them with Java migration

* Removed all migration written in sql and replace them with Java migration 2.0

* Made migration work without compilation errors + added initial testing

* cleanup the build.gradle of previous config

* moved init database as a migration script

* breakdown migration into smaller subunit for V3 and started the preparation for V1

* V1 will create only necessary tables and columns without indexes

* updated migrations

* some missing indexes for V3

* clean up

---------

Co-authored-by: Sabin Antohe <[email protected]>
private static Map<String, String> getPlaceholders(Start start) {
Map<String, String> ph = new HashMap<>();
ph.put("process_id", start.getProcessId());
ph.put("access_token_signing_key_dynamic", String.valueOf( Config.getConfig(start).getAccessTokenSigningKeyDynamic()));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will have to be bought in from the core. As this config is a core config and not a db plugin config

.locations(LOCATION)
.placeholders(getPlaceholders(start))
.load();
flyway.migrate();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • if an index is added for a migration that takes several seconds or mins to run, will that be blocking or will it happen in the badkground?
  • When we issue a create index command (which can potentially take several mins), does psql return immediately (and does it in the background), or it blocks?

{
"jar": "https://repo1.maven.org/maven2/org/flywaydb/flyway-core/7.15.0/flyway-core-7.15.0.jar",
"name": "Flyway Core 7.15.0",
"src": "https://repo1.maven.org/maven2/org/flywaydb/flyway-core/7.15.0/flyway-core-7.15.0-sources.jar"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add all dependencies of this too

import org.flywaydb.core.api.migration.Context;

import java.util.Map;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to confirm all these migrations are correctly specified. How can we confirm this?

* License for the specific language governing permissions and limitations
* under the License.
*/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename all migration files to be VX__A_B_C.java

Where VX is V1, V2 and so on.. and A_B_C is the core version that introduced the new changes

public static void startMigration(Start start) throws SQLException, StorageQueryException {
String baseline = BaselineMigrationQueries.getBaselineMigrationVersion(start);
if (Integer.parseInt(baseline) >= BaselineMigrationQueries.LAST_MIGRATION) {
return;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we are going to be running v6, then this will have to run. This can be done by getting the baseline from flyway and checking for that as well. Right now LAST_MIGRATION points to V5 cause V6 is a compulsory one to run in case there is no baseline in the db already.

We can solve this by checking if there is a baseline in the db from flyway's side, and if there is, we should also check if that is >= V6 (not BaselineMigrationQueries.LAST_MIGRATION), and if it is, then we should return.

private static Map<String, String> getPlaceholders(Start start) {
Map<String, String> ph = new HashMap<>();
ph.put("process_id", start.getProcessId());
ph.put("access_token_signing_key_dynamic", String.valueOf( Config.getConfig(start).getAccessTokenSigningKeyDynamic()));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will be removed in the future cause it would be saved inside the start class. Or some other way...

}

if (!doesTableExists(start, Config.getConfig(start).getKeyValueTable())) {
{
getInstance(start).addState(CREATING_NEW_TABLE, null);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can move this process state into v1 migration script and just do it once, instead of doing it again and again here

public void migrate(Context context) throws Exception {
Map<String, String> ph = context.getConfiguration().getPlaceholders();
Start start = MigrationContextManager.getContext(ph.get("process_id"));
GeneralQueries.createTablesIfNotExists(start);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unlike before, there is no index being created in this step. Is that okay? Need to confirm that all necessary indexes are being created in the later on migration steps.

* License for the specific language governing permissions and limitations
* under the License.
*/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants