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

implement bookmark copying & ZCP bookmark support #9571

Closed
wants to merge 2 commits into from

Conversation

problame
Copy link
Contributor

@problame problame commented Nov 11, 2019

This feature allows copying existing bookmarks using

  zfs bookmark fs#target fs#newbookmark

There are some niche use cases for such functionality,
e.g. when using bookmarks as markers for replication progress.

Copying redaction bookmarks produces a normal bookmark that
cannot be used for redacted send (we are not duplicating
the redaction object).

ZCP support for bookmarking (both creation and copying) is implemented in a separate commit in this PR.

Overview:

  • Terminology:
    • source = existing snapshot or bookmark
    • new/bmark = new bookmark
  • Implement bookmark copying in dsl_bookmark.c
    • create new bookmark node
    • copy source's zbn_phys to new's zbn_phys
    • zero-out redaction object id in copy
  • Extend existing bookmark ioctl nvlist schema to accept
    bookmarks as sources
    • => dsl_bookmark_create_nvl_validate is authoritative
  • use dsl_dataset_is_before check for both snapshot
    and bookmark sources
  • Adjust CLI
    • refactor shortname expansion logic in zfs_do_bookmark
  • Update man pages
    • warn about redaction bookmark handling
  • Add test cases
    • CLI
    • ZCP (very basic)
    • pyyzfs libzfs_core bindings

How Has This Been Tested?

  • The tests added as part of this PR
  • Manual testing on a file-backed pool, unencrypted dataset

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)
  • Documentation (a change to man pages or other documentation)

Checklist:

Copy link
Contributor

@Ornias1993 Ornias1993 left a comment

Choose a reason for hiding this comment

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

I'm not to much into bookmarks/snapshots to review your TODO's and such.
But I do have a few notes about terminology.

I do notice about 20% of this PR seems to be renaming snapshot to target, I don't think thats really needed and might needlessly confuse users.

cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved
man/man8/zfs.8 Outdated Show resolved Hide resolved
@ahrens ahrens added Component: Send/Recv "zfs send/recv" feature Type: Feature Feature request or new feature Status: Code Review Needed Ready for review and testing Status: Work in Progress Not yet ready for general review and removed Status: Code Review Needed Ready for review and testing labels Nov 11, 2019
cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved
cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved
module/zfs/dsl_bookmark.c Outdated Show resolved Hide resolved
module/zfs/dsl_bookmark.c Outdated Show resolved Hide resolved
module/zfs/dsl_bookmark.c Show resolved Hide resolved
module/zfs/dsl_bookmark.c Outdated Show resolved Hide resolved
module/zfs/dsl_bookmark.c Outdated Show resolved Hide resolved
module/zfs/dsl_bookmark.c Outdated Show resolved Hide resolved
module/zfs/dsl_bookmark.c Outdated Show resolved Hide resolved
@problame
Copy link
Contributor Author

problame commented Nov 24, 2019

I addressed most of the review comments. Open issues:

  • mechanism to copy redaction bookmarks
  • naming: target vs src vs origin
  • channel program support (is this necessary?)
    • update man page

@problame problame changed the title implement bookmark cloning implement bookmark cloning & ZCP bookmark support Nov 25, 2019
@problame
Copy link
Contributor Author

problame commented Jan 7, 2020

We talked about this PR on the OpenZFS montly call. From the notes:

  • redaction object might be too large to copy it in syncing context, would have to be done in the background
  • => don’t implement redaction bookmark cloning at this point
  • => fail on: zfs bookmark #redactionbook #newbook
  • => create thin bookmark: zfs bookmark --thin #redactionbook #newbook
    will not contain the redaction object itself

cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved
module/zfs/zfs_ioctl.c Outdated Show resolved Hide resolved
module/zfs/dsl_bookmark.c Outdated Show resolved Hide resolved
module/zfs/dsl_bookmark.c Outdated Show resolved Hide resolved
@problame
Copy link
Contributor Author

problame commented Jan 14, 2020

Changes in the force-push:

  • prettify commit history prior to addressing review round
  • Merge dsl_bookmark_clone machinery into dsl_bookmark_create
  • Implement my proposal to always use full paths in the nvlists, both from ioctl and zcp
  • Make zfs bookmark ds#redactionbookmark ds#newbookmark create a 'normal' bookmark that doesn't reference the redaction bookmark, put explicit notice in man page
    • @pcd1193182 is ok with that
    • if full copies of redaction bookmarks are desired at some point in the future, a CLI flag such as zfs bookmark --redaction ds#redactionbookmark ds#newbookmark could be used

Planned changes after next review round:

  • address review feedback
  • prettify commit history, add Signed-Off-By in commit messages
  • Do I need to rebase onto master myself or will the person merging this PR do that?
    • In any case, the merge should not need to squash the commits any further

@problame problame marked this pull request as ready for review January 14, 2020 23:17
module/zfs/dsl_bookmark.c Outdated Show resolved Hide resolved
module/zfs/dsl_bookmark.c Outdated Show resolved Hide resolved
module/zfs/dsl_bookmark.c Outdated Show resolved Hide resolved
module/zfs/dsl_bookmark.c Outdated Show resolved Hide resolved
module/zfs/dsl_bookmark.c Show resolved Hide resolved
module/zfs/dsl_bookmark.c Outdated Show resolved Hide resolved
@problame
Copy link
Contributor Author

problame commented Jan 16, 2020

  • consistently use 'copying' instead of 'cloning'
  • test case for bookmark copying with source on different fs that is origin of newbookmark's fs
    • added a new zfs_error_t to distinguish that case
  • test / review empty nvlist
    • covered by pyzfs test suite
  • understand why the zcp code is not in coverage https://codecov.io/gh/zfsonlinux/zfs/compare/1bb5f5e2b47336ddc8d22833801eadbee8ce21e6...95bb9e74782f83cea33cb0e7ffa2b39c79eaa388/diff
  • fix test failures in pylint
    • extended pyzfs bindings + test coverage
  • add validation to *create_redacted prior to it calling dsl_bookmark_create_check_impl
    • was causing test failures in debug mode (yay, asserts)
  • after the above: another review round

@problame problame changed the title implement bookmark cloning & ZCP bookmark support implement bookmark copying & ZCP bookmark support Jan 16, 2020
@problame problame force-pushed the clonebookmarks branch 2 times, most recently from 932e69e to 8be1578 Compare January 17, 2020 15:22
@problame
Copy link
Contributor Author

The ubuntu 16.04 TEST result link is a redirection loop.

The CentOS 7 failure:


Tests with results other than PASS that are unexpected:
    SKIP cli_root/zpool_create/zpool_create_008_pos (expected PASS)

doesn't really seem to be a failure (I did not mark it as SKIP anywhere)

The coverage test fails in ztest, but I cannot reproduce the crashes on my machine.

@ahrens @pcd1193182 I think this PR is ripe for another round of code review. See the checkboxes in prior comments for what changed since the last one. Thanks!

@behlendorf
Copy link
Contributor

The ubuntu 16.04 TEST result link is a redirection loop.

Sorry about that, it was a CI related infrastructure failure was has now been sorted out for the next run.

The CentOS 7 failure:

Right, unrelated and recently fixed in master by e5030fb.

The coverage test fails in ztest, but I cannot reproduce the crashes on my machine.

The ztest failures look to be unrelated, but the reported memory leak should be looked in to.

@problame
Copy link
Contributor Author

@problame it looks like several commits in this stack could each be moved to their own PRs. This would help me get them merged more quickly. Specifically, I'm thinking of:

f72eddc dsl_bookmark_create_check: fix NULL pointer deref if dbca_errors == NULL
8c6a30f zfs_namecheck: entity_namecheck: update doc comment to include space as allowed
c1eb601 scripts/zfs-test.sh: example for -t

Also #9867 has been merged.

Opened the respective PRs, one for each commit.
I have rebased this branch onto latest master and resolved conflicts with the zcp inherit support, but won't force-push until those one-commit PRs are merged and this branch is rebased again.

@behlendorf behlendorf added Status: Code Review Needed Ready for review and testing and removed Status: Work in Progress Not yet ready for general review labels Jan 24, 2020
@behlendorf
Copy link
Contributor

@problame thanks for splitting out those small changes. They've now been merged.

man/man8/zfs-bookmark.8 Outdated Show resolved Hide resolved
man/man8/zfs-bookmark.8 Outdated Show resolved Hide resolved
man/man8/zfs-program.8 Outdated Show resolved Hide resolved
man/man8/zfs.8 Outdated Show resolved Hide resolved
module/zfs/zcp_synctask.c Outdated Show resolved Hide resolved
module/zfs/dsl_bookmark.c Outdated Show resolved Hide resolved
module/zfs/dsl_bookmark.c Outdated Show resolved Hide resolved
module/zfs/dsl_bookmark.c Outdated Show resolved Hide resolved
module/zfs/dsl_bookmark.c Outdated Show resolved Hide resolved
module/zfs/dsl_bookmark.c Outdated Show resolved Hide resolved
@problame
Copy link
Contributor Author

problame commented Jan 28, 2020

addressed @ahrens 's last review comments:

  • fix bookmark creation of snapshots of the pool dataset
    • add corresponding regression test
  • extend pyzfs unittests for bookmark creation with missing sources
    to explicitly check that no bookmarks were created if a source is missing
  • more concise doc comments
  • use correct assert macro
  • address spacecheck todos in zcp code
  • fix typos

@problame problame requested a review from ahrens January 29, 2020 15:24
@problame
Copy link
Contributor Author

Something about the codecov upload / processing seems to be broken again (cc @behlendorf )

Apart from that, all pending review remarks have been addressed.

module/zcommon/zfs_namecheck.c Outdated Show resolved Hide resolved
module/zfs/dsl_bookmark.c Outdated Show resolved Hide resolved
@behlendorf behlendorf added this to New OpenZFS 2.0 Features (0.8->2.0) in OpenZFS 2.0 Feb 10, 2020
This feature allows copying existing bookmarks using
```
  zfs bookmark fs#target fs#newbookmark
```
There are some niche use cases for such functionality,
e.g. when using bookmarks as markers for replication progress.

Copying redaction bookmarks produces a normal bookmark that
cannot be used for redacted send (we are not duplicating
the redaction object).

ZCP support for bookmarking (both creation and copying) will be
implemented in a separate patch based on this work.

Overview:

- Terminology:
    - source = existing snapshot or bookmark
    - new/bmark = new bookmark
- Implement bookmark copying in `dsl_bookmark.c`
  - create new bookmark node
  - copy source's `zbn_phys` to new's `zbn_phys`
  - zero-out redaction object id in copy
- Extend existing bookmark ioctl nvlist schema to accept
  bookmarks as sources
  - => `dsl_bookmark_create_nvl_validate` is authoritative
- use `dsl_dataset_is_before` check for both snapshot
  and bookmark sources
- Adjust CLI
  - refactor shortname expansion logic in `zfs_do_bookmark`
- Update man pages
  - warn about redaction bookmark handling
- Add test cases
  - CLI
  - pyyzfs libzfs_core bindings

Signed-off-by: Christian Schwarz <me@cschwarz.com>
supports bookmark creation and cloning

Signed-off-by: Christian Schwarz <me@cschwarz.com>
@problame
Copy link
Contributor Author

problame commented Feb 11, 2020

Addressed @pcd1193182 's remarks:

  • EQUIV
  • remove redundant SET_ERROR(error) /* dsl_bookmark_lookup_impl doesn't */
  • drop "future work" comments in test suite
  • rebased without conflicts onto latest master

@codecov
Copy link

codecov bot commented Feb 11, 2020

Codecov Report

Merging #9571 into master will decrease coverage by <1%.
The diff coverage is 87%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #9571    +/-   ##
========================================
- Coverage      79%      79%   -<1%     
========================================
  Files         385      385            
  Lines      122282   122372    +90     
========================================
- Hits        96838    96791    -47     
- Misses      25444    25581   +137
Flag Coverage Δ
#kernel 79% <90%> (ø) ⬇️
#user 67% <21%> (ø) ⬇️

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 dceeca5...8967bcf. Read the comment docs.

@problame
Copy link
Contributor Author

The CI failure seems like a flaky test.

ahrens
ahrens approved these changes Feb 11, 2020
@@ -22,11 +22,15 @@


# https://stackoverflow.com/a/1695250
Copy link
Member

Choose a reason for hiding this comment

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

I realize that this is "just" contrib code, but it would be preferable for the comment to explain the code. We could consider also linking to an external expanded explanation if needed, while weighing the risk of this link becoming stale. I also realize you aren't adding this link, but if you have time, fixing this would be appreciated.

We could explain why we can't use the built-in python enums (presumably because we want to support pre-2.4 python installs); explain approximately what this code does (creates a new type?); and that this is the canonical way of doing so on old python. It may then not be necessary to include the external link.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just cargo-culted generalized the existing enum type constructor/generator to do what I needed it to do.
I'd guess the intention behind the link was to point a future developer to practical examples for enum type construction.
FWIW: The stackoverflow page is archived on archive.org ¯_(ツ)_/¯

I figure future work should replace _constants.py with code-generation anyways, so I guess this is not a blocker for merging this feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

This potentially looks like a good candidate to cleanup in another PR if someone is interested. Python 2 officially went EOL in January 2020 so bumping the minimum supported Python version to 3.4 makes sense. I tend to agree this doesn't need to block this feature.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Feb 11, 2020
@@ -22,11 +22,15 @@


# https://stackoverflow.com/a/1695250
Copy link
Contributor

Choose a reason for hiding this comment

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

This potentially looks like a good candidate to cleanup in another PR if someone is interested. Python 2 officially went EOL in January 2020 so bumping the minimum supported Python version to 3.4 makes sense. I tend to agree this doesn't need to block this feature.

behlendorf pushed a commit that referenced this pull request Feb 11, 2020
Add support for bookmark creation and cloning.

Reviewed-by: Matt Ahrens <matt@delphix.com>
Reviewed-by: Paul Dagnelie <pcd@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Christian Schwarz <me@cschwarz.com>
Closes #9571
@behlendorf
Copy link
Contributor

Merged as:

948f0c4 zcp: add zfs.sync.bookmark
a73f361 Implement bookmark copying

jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
This feature allows copying existing bookmarks using

    zfs bookmark fs#target fs#newbookmark

There are some niche use cases for such functionality,
e.g. when using bookmarks as markers for replication progress.

Copying redaction bookmarks produces a normal bookmark that
cannot be used for redacted send (we are not duplicating
the redaction object).

ZCP support for bookmarking (both creation and copying) will be
implemented in a separate patch based on this work.

Overview:

- Terminology:
    - source = existing snapshot or bookmark
    - new/bmark = new bookmark
- Implement bookmark copying in `dsl_bookmark.c`
  - create new bookmark node
  - copy source's `zbn_phys` to new's `zbn_phys`
  - zero-out redaction object id in copy
- Extend existing bookmark ioctl nvlist schema to accept
  bookmarks as sources
  - => `dsl_bookmark_create_nvl_validate` is authoritative
- use `dsl_dataset_is_before` check for both snapshot
  and bookmark sources
- Adjust CLI
  - refactor shortname expansion logic in `zfs_do_bookmark`
- Update man pages
  - warn about redaction bookmark handling
- Add test cases
  - CLI
  - pyyzfs libzfs_core bindings

Reviewed-by: Matt Ahrens <matt@delphix.com>
Reviewed-by: Paul Dagnelie <pcd@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Christian Schwarz <me@cschwarz.com>
Closes openzfs#9571
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
Add support for bookmark creation and cloning.

Reviewed-by: Matt Ahrens <matt@delphix.com>
Reviewed-by: Paul Dagnelie <pcd@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Christian Schwarz <me@cschwarz.com>
Closes openzfs#9571
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Send/Recv "zfs send/recv" feature Status: Accepted Ready to integrate (reviewed, tested) Type: Feature Feature request or new feature
Projects
No open projects
OpenZFS 2.0
  
New OpenZFS 2.0 Features (0.8->2.0)
Development

Successfully merging this pull request may close these issues.

None yet

5 participants