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

Fix encrypted dataset restore and make backup cron setting optional #68

Merged
merged 5 commits into from
Jan 24, 2023

Conversation

rdelcorro
Copy link
Contributor

  • As stated in the docs, the cron config should be optional. When not providing a cron config, it was crashing on backup as its dereferenced in the add_job call.

  • The tool was correctly using the raw method to upload the send binary but on receive / restore, it was first creating a datastore and then forcing a rewrite with the -F flag. This poses a problem on encrypted datasets as you can't restore with -F. See zfs receive -F cannot be used to destroy an encrypted filesystem openzfs/zfs#6793 for more information. This change removes the -F but also does not create the dataset, which should be fine as receive would do that anyways. Tested on encrypted datasets and now its working

Copy link
Owner

@ddebeau ddebeau left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

I added the -F option to fix an issue where the restore would fail if data had been written to the file system since the most recent snapshot. #33

I'll have to look into your issue in more detail. It may be worth adding some tests that replicate #33 and the encrypted restore issue.

zfs_uploader/__main__.py Show resolved Hide resolved
@rdelcorro
Copy link
Contributor Author

rdelcorro commented Jan 18, 2023

I can try to add the test for the encrypted case, but this is basically the workflow:

zfs create -o encryption=on -o keylocation=prompt -o keyformat=passphrase ztest/sourceenc
touch /ztest/sourceenc/file

In the config, set this up:
[ztest/sourceenc]
max_snapshots = 7
max_incremental_backups_per_full = 6
max_backups = 7

zfsup backup
zfsup restore ztest/sourceenc --destination ztest/destenc (this datastore does not exist)

result:

root@rover:/etc/zfs_uploader# zfsup restore ztest/sourceenc --destination ztest/destenc
time=2023-01-18T04:25:48.286 level=INFO file_path=config.cfg msg="Loading configuration file."
time=2023-01-18T04:25:48.772 level=INFO filesystem=ztest/destenc snapshot_name=20230118_042500 s3_key=ztest/sourceenc/20230118_042500.full msg="Restoring snapshot."
Traceback (most recent call last):
  File "/usr/local/sbin/zfsup", line 8, in <module>
    sys.exit(cli())
  File "/etc/zfs_uploader/env/lib/python3.10/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/etc/zfs_uploader/env/lib/python3.10/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "/etc/zfs_uploader/env/lib/python3.10/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/etc/zfs_uploader/env/lib/python3.10/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/etc/zfs_uploader/env/lib/python3.10/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/etc/zfs_uploader/env/lib/python3.10/site-packages/click/decorators.py", line 26, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/etc/zfs_uploader/env/lib/python3.10/site-packages/zfs_uploader/__main__.py", line 135, in restore
    job.restore(backup_time, destination)
  File "/etc/zfs_uploader/env/lib/python3.10/site-packages/zfs_uploader/job.py", line 265, in restore
    self._restore_snapshot(backup, filesystem)
  File "/etc/zfs_uploader/env/lib/python3.10/site-packages/zfs_uploader/job.py", line 413, in _restore_snapshot
    raise ZFSError(stderr)
zfs_uploader.zfs.ZFSError: cannot receive new filesystem stream: zfs receive -F cannot be used to destroy an encrypted filesystem or overwrite an unencrypted one with an encrypted one

Should we consider adding a boolean flag to the restore operation to pass in the -F like (--force)? Restoring on top of a dataset that has changes would indeed cause data loss

@ddebeau
Copy link
Owner

ddebeau commented Jan 22, 2023

@rdelcorro Could you merge in master? The zfs receive -F issues should be resolved now.

@rdelcorro
Copy link
Contributor Author

@ddebeau code is now merged. Thanks!

Copy link
Owner

@ddebeau ddebeau left a comment

Choose a reason for hiding this comment

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

Just a couple quick fixes and the PR should be ready to merge.

zfs_uploader/__main__.py Outdated Show resolved Hide resolved
zfs_uploader/__main__.py Outdated Show resolved Hide resolved
@rdelcorro
Copy link
Contributor Author

Corrected the comments. Feel free to merge when possible. I do not seem to have the ability to do so. Do you know why tests are failing?

@ddebeau
Copy link
Owner

ddebeau commented Jan 24, 2023

I don't think your PR has access to the secrets required to run the tests. I'll run the tests before I make a release. Thank you for your help!

@ddebeau ddebeau merged commit bc3d15b into ddebeau:master Jan 24, 2023
@ddebeau
Copy link
Owner

ddebeau commented Jan 25, 2023

@rdelcorro
Copy link
Contributor Author

Thanks, I will test it out

@rdelcorro rdelcorro deleted the rdelcorroFixEncryptedDataset branch January 2, 2024 01:23
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