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

Backup: fetch tarball with synchronize instead of fetch #4326

Merged
merged 1 commit into from
Apr 8, 2019

Conversation

rmol
Copy link
Contributor

@rmol rmol commented Apr 8, 2019

Status

Ready for review

Description of Changes

Ansible's fetch module causes MemoryError with large files when run with 'become':

https://docs.ansible.com/ansible/latest/modules/fetch_module.html#notes

It was easily replaced with the synchronize module:

https://docs.ansible.com/ansible/latest/modules/synchronize_module.html

Testing

You'll need working app and admin machines. I tested with hardware, but it should be possible to test with virtual machines.

On the app server, create a one-gigabyte file in the SecureDrop store:

$ sudo dd if=/dev/urandom of=/var/lib/securedrop/store/bigfile.bin bs=1MB count=1000

On the admin workstation, using the develop branch, back up the app server with securedrop-admin backup. It should fail with a MemoryError. If it does not, try creating a bigger file by adjusting the dd command above.

On the admin workstation, check out this branch:

$ git remote add rmol git@github.com:rmol/securedrop.git
$ git fetch --all
$ git checkout -b fix-backup-fetch rmol/fix-backup-fetch

Then run ./securedrop-admin backup. It should complete successfully.

Deployment

This should be a drop-in replacement for the fetch module usage. The backup tarball creation has not changed, only the way in which it's transferred to the admin workstation. Restoration should not be affected.

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

Ansible's fetch module causes MemoryError with large files when
run with 'become':

 https://docs.ansible.com/ansible/latest/modules/fetch_module.html#notes

It was easily replaced with the synchronize module:

 https://docs.ansible.com/ansible/latest/modules/synchronize_module.html
@redshiftzero redshiftzero self-requested a review April 8, 2019 20:35
@redshiftzero redshiftzero added this to the 0.12.2 milestone Apr 8, 2019
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.

tested this on an instance that was consistently hitting the MemoryError during backup, works as advertised - thanks @rmol!

@redshiftzero redshiftzero merged commit 9ed6b88 into freedomofpress:develop Apr 8, 2019
eloquence added a commit that referenced this pull request Apr 10, 2019
Time-sensitive due to Xenial updates in progress.

This will be resolved with #4326 in 0.12.1, though we may want to
keep some version of this documentation around in case people
still encounter issues during the playbook run.

Credit to @rmol for authoring the rsync command used here, which
has been used during real-world upgrades.
eloquence added a commit that referenced this pull request Apr 10, 2019
Time-sensitive due to Xenial updates in progress.

This will be resolved with #4326 in 0.12.1, though we may want to
keep some version of this documentation around in case people
still encounter issues during the playbook run.

Credit to @rmol for authoring the rsync command used here, which
has been used during real-world upgrades.
eloquence added a commit that referenced this pull request Apr 10, 2019
Time-sensitive due to Xenial updates in progress.

This will be resolved with #4326 in 0.12.1, though we may want to
keep some version of this documentation around in case people
still encounter issues during the playbook run.

Credit to @rmol for authoring the rsync command used here, which
has been used during real-world upgrades.
eloquence added a commit that referenced this pull request Apr 10, 2019
Time-sensitive due to Xenial updates in progress.

This will be resolved with #4326 in 0.12.1, though we may want to
keep some version of this documentation around in case people
still encounter issues during the playbook run.

Credit to @rmol for authoring the rsync command used here, which
has been used during real-world upgrades.

(cherry picked from commit 8571ee1)
@emkll emkll mentioned this pull request Apr 18, 2019
16 tasks
kushaldas pushed a commit that referenced this pull request Sep 25, 2019
Time-sensitive due to Xenial updates in progress.

This will be resolved with #4326 in 0.12.1, though we may want to
keep some version of this documentation around in case people
still encounter issues during the playbook run.

Credit to @rmol for authoring the rsync command used here, which
has been used during real-world upgrades.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants