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

[DB migrations] Added downgrade support and AlembicUtil Tests #583

Merged
merged 8 commits into from Dec 10, 2020

Conversation

quaark
Copy link
Member

@quaark quaark commented Dec 9, 2020

When initializing the database:

  • On db initialization a backup of the DB is made before the upgrade to the latest schema revision, with its previous alembic revision as its name (Any new backup overrides the old backup if it is the same revision)
  • If the current DB revision doesn't exist in the known revision history, this means the DB is too new for the code, and we will try to fall back to the backup of the latest know revision.
  • If no such backup exists - raise a RuntimeError and fail initializing the DB.

In addition - completely unit-tested the AlembicUtil

Copy link
Contributor

@Hedingber Hedingber left a comment

Choose a reason for hiding this comment

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

Think of this scenario:
MLRun is in version X -> we're installing version X+1, migration succeeds (so backup created at this point) -> user uses this X+1 (so data is added) -> we're installing version X+2, migration failed -> we're installing X+1 again -> it uses the backup, but we lost a lot of the data.
I think you should backup the DB before the migration (before the action that might fail and you'll want to revert)

mlrun/api/utils/alembic.py Outdated Show resolved Hide resolved
@quaark quaark changed the title Added DB downgrade support Added DB downgrade support and AlembicUtil Tests Dec 10, 2020
Copy link
Contributor

@Hedingber Hedingber left a comment

Choose a reason for hiding this comment

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

Looks very good.
Test coverage - 💯 ❤️
one comment inside, also please update the PR description

mlrun/api/utils/alembic.py Outdated Show resolved Hide resolved
@Hedingber Hedingber changed the title Added DB downgrade support and AlembicUtil Tests [DB migrations] Added downgrade support and AlembicUtil Tests Dec 10, 2020
@Hedingber Hedingber merged commit 86cf26c into mlrun:development Dec 10, 2020
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.

None yet

2 participants