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

Apply database migration on backup restore #3737

Merged
merged 1 commit into from
Aug 27, 2018

Conversation

emkll
Copy link
Contributor

@emkll emkll commented Aug 24, 2018

Status

Ready for review

Description of Changes

Fixes #3732 .

Apply migrations as part of backup restore script.

Testing

  1. Install SecureDrop version < 0.9.0
  2. ./securedrop-admin backup
  3. Upgrade SecureDrop to 0.9.0-rc1
  4. Grab filename from backup tarball that was written to install_files/ansible-base/
  5. ./securedrop-admin restore <filename>
  6. Go to the source interface and submit a document
  7. Validate that the document can be retrieved on the journalist interface
  8. Inspect the database to ensure the migrations have been successful, and that the following tables have UUIDs (sqlite3 /var/lib/securedrop/db.sqlite):
    a. replies
    b. submissions
    c. sources
    d. journalists

Deployment

This will be used when restoring from backup. Code will be updated via SecureDrop Updater GUI.

Checklist

If you made changes to securedrop-admin:

  • Linting and tests (make -C admin test) pass in the admin development container

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR

@emkll emkll added this to the 0.9 milestone Aug 24, 2018
@emkll emkll requested review from conorsch and msheiny as code owners August 24, 2018 20:07
@zenmonkeykstop
Copy link
Contributor

Do the test steps need a securedrop-admin restore <file> as well?

Backups restored to a newer version of SecureDrop may introduce breakage
if a migration has occurred. dpkg-reconfigure will call the postinst
which will in turn apply all necessary alembic migrations to the
database.
@emkll emkll force-pushed the 3732-restore-db-migrations branch from b6a15d4 to 0f2ce05 Compare August 24, 2018 20:25
@emkll
Copy link
Contributor Author

emkll commented Aug 24, 2018

Woops, updated the test plan with restore instructions. Thanks @zenmonkeykstop !

@codecov-io
Copy link

codecov-io commented Aug 24, 2018

Codecov Report

Merging #3737 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #3737   +/-   ##
========================================
  Coverage    84.76%   84.76%           
========================================
  Files           44       44           
  Lines         2757     2757           
  Branches       297      297           
========================================
  Hits          2337     2337           
  Misses         353      353           
  Partials        67       67

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 f922012...0f2ce05. Read the comment docs.

@@ -45,6 +45,8 @@ def main():
# If the process exits with a non-zero return code, raises an exception.
subprocess.check_call(['service', 'apache2', 'restart'])
subprocess.check_call(['service', 'tor', 'reload'])
# Apply database migrations (if backed-up version < version to restore)
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically this comment is wrong. It will always attempt to migrate to head which will throw an exception if we restore a v3 version DB to a Debian package with v2 migration code (meaning one migration is missing) alembic will explode and yell at us that it can't do the thing.

@redshiftzero redshiftzero self-assigned this Aug 27, 2018
@redshiftzero redshiftzero self-requested a review August 27, 2018 18:18
Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

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

followed test plan, all is as expected after restore ✅

@redshiftzero redshiftzero merged commit 0c6e184 into develop Aug 27, 2018
@redshiftzero redshiftzero deleted the 3732-restore-db-migrations branch August 27, 2018 23:07
@emkll emkll mentioned this pull request Aug 30, 2018
22 tasks
@zenmonkeykstop zenmonkeykstop mentioned this pull request Oct 16, 2018
24 tasks
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.

5 participants