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

Allow zfs to send replication streams with missing snapshots #11710

Merged
merged 1 commit into from
Apr 11, 2021
Merged

Allow zfs to send replication streams with missing snapshots #11710

merged 1 commit into from
Apr 11, 2021

Conversation

pabloyoyoista
Copy link
Contributor

@pabloyoyoista pabloyoyoista commented Mar 8, 2021

Motivation and Context

A similar change was proposed previously in #5285 by @loli10K. In the pull request, it was discussed that the integration of OpenZFS 6111 (6635624) reduced the utility of sending replication streams. If there are any datasets in the hierarchy which are not snapshoted, a replication send of any parent will fail due to the missing snapshots. The rationale for that decision is to avoid administrators to think that they've sent all the data when in reality that is not the case. According to #5285, the right way to do this would be to add an extra flag (I chose -f|--force --skip-missing|-s) which modifies that behavior for administrators which request it.

Also, this is my first contribution to the OpenZFS project, so I apologize for any possible newbie mistakes. I have not added documentation on purpose, because I would like to be sure that the ideas here make sense before putting extra effort into it.

Description

This patch adds an extra option (--skip-missing|-s) to zfs send. The skip flag can only be used in conjunction with -R. In the regular replication behavior, if zfs encounters a dataset without the corresponding snapshot and that was created before the parent snapshot, then an error will occur and the send will be aborted. This MR modifies that behavior, so that if skip flag is also provided, then the dataset and its children will be ignored. Instead of an error, a warning text will be written to stderr and the send will continue.

How Has This Been Tested?

One small test has been added to the testsuite, which I could extend if required and some local testing worked without issues. I have not done any further tests in a real system, but I could also possibly do if needed. I have placed the test under functional/cli_root/zfs_send because it is just a small feature... But I hesitated whether it would fit best under functional/rsend with a bit extra testing. Any suggestions?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@bghira
Copy link

bghira commented Mar 9, 2021

one comment is that the -f flag name is very vague and overused. it might become the norm to use -f without remembering why. can you make it more specific to skipping missing snapshot?

@pabloyoyoista
Copy link
Contributor Author

Yes, that is totally true. Maybe -s|--skip-missing? -S|--saved is already used to send partially received datasets, but I'd guess the use cases are different enough not to get confusing due to the capitalization. Any other suggestions otherwise?

@pabloyoyoista
Copy link
Contributor Author

Replaced to --skip-missing|-s now

@pabloyoyoista pabloyoyoista changed the title Draft: Allow zfs to send replication streams with missing snapshots Allow zfs to send replication streams with missing snapshots Mar 14, 2021
@pabloyoyoista pabloyoyoista marked this pull request as ready for review March 14, 2021 17:19
@pabloyoyoista
Copy link
Contributor Author

As there were not any other comments, I have finished and pushed the documentation. This should be ready for review.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Mar 16, 2021
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this! I don't have any objection to adding a command line option to allow this.

@behlendorf behlendorf requested review from pcd1193182 and ahrens March 16, 2021 22:27
A tentative implementation and discussion was done in #5285.
According to it a send --skip-missing|-s flag has been added.
In a replication stream, when there are snapshots missing in
the hierarchy, if -s is provided print a warning and ignore
dataset (and its children) instead of throwing an error

Signed-off-by: Pablo Correa Gómez <[email protected]>
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Apr 3, 2021
@behlendorf behlendorf merged commit 099fa7e into openzfs:master Apr 11, 2021
behlendorf added a commit that referenced this pull request Apr 12, 2021
Commit 099fa7e intentionally modified the libzfs ABI.  However, it
failed to include an update for the libzfs.abi file.  This commit
resolves the `make checkabi` warning due to that omission.

Signed-off-by: Brian Behlendorf <[email protected]>
Closes #11710
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Apr 14, 2021
A tentative implementation and discussion was done in openzfs#5285.
According to it a send --skip-missing|-s flag has been added.
In a replication stream, when there are snapshots missing in
the hierarchy, if -s is provided print a warning and ignore
dataset (and its children) instead of throwing an error

Reviewed-by: Paul Dagnelie <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Pablo Correa Gómez <[email protected]>
Closes openzfs#11710
behlendorf added a commit to behlendorf/zfs that referenced this pull request Apr 14, 2021
Commit 099fa7e intentionally modified the libzfs ABI.  However, it
failed to include an update for the libzfs.abi file.  This commit
resolves the `make checkabi` warning due to that omission.

Signed-off-by: Brian Behlendorf <[email protected]>
Closes openzfs#11710
@ghost ghost mentioned this pull request Apr 15, 2021
13 tasks
mcmilk pushed a commit to mcmilk/zfs that referenced this pull request Apr 15, 2021
Commit 099fa7e intentionally modified the libzfs ABI.  However, it
failed to include an update for the libzfs.abi file.  This commit
resolves the `make checkabi` warning due to that omission.

Signed-off-by: Brian Behlendorf <[email protected]>
Closes openzfs#11710
behlendorf pushed a commit that referenced this pull request Apr 16, 2021
Before #11710 the flags in zfs-send(8) were sorted.
Restore order and bump the date.

Reviewed-by: George Melikov <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes #11905
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Apr 21, 2021
Before openzfs#11710 the flags in zfs-send(8) were sorted.
Restore order and bump the date.

Reviewed-by: George Melikov <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11905
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
A tentative implementation and discussion was done in openzfs#5285.
According to it a send --skip-missing|-s flag has been added.
In a replication stream, when there are snapshots missing in
the hierarchy, if -s is provided print a warning and ignore
dataset (and its children) instead of throwing an error

Reviewed-by: Paul Dagnelie <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Pablo Correa Gómez <[email protected]>
Closes openzfs#11710
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
Commit 099fa7e intentionally modified the libzfs ABI.  However, it
failed to include an update for the libzfs.abi file.  This commit
resolves the `make checkabi` warning due to that omission.

Signed-off-by: Brian Behlendorf <[email protected]>
Closes openzfs#11710
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
Before openzfs#11710 the flags in zfs-send(8) were sorted.
Restore order and bump the date.

Reviewed-by: George Melikov <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11905
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants