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

add zfs create dryrun #8974

Merged
merged 1 commit into from
Jul 16, 2019
Merged

add zfs create dryrun #8974

merged 1 commit into from
Jul 16, 2019

Conversation

mgerdts
Copy link
Contributor

@mgerdts mgerdts commented Jul 1, 2019

Adds the ability to sanity check zfs create arguments and to see the
value of any additional properties that will local to the dataset. For
example, automation that may need to adjust quota on a parent filesystem
before creating a volume may call zfs create -nP -V <size> <volume> to
obtain the value of refreservation. This adds the following options to
zfs create:

  • -n dry-run (no-op)
  • -v verbose
  • -P parseable (implies verbose)

Motivation and Context

This PR is being raised to gain feedback from the zfsonlinux community on the proposed user-visible changes to zfs create. The first changeset is from PR 8973. At this point, I'm looking for confirmation that the the new CLI options are acceptable. I am aware of a couple format string nits I need to fix in the code.

In SmartOS, we need a way to determine the value that refreservation will take before the volume is created.

Two paths have been explored:

  1. Write a standalone utility that calls zvol_volsize_to_reservation, which is a private interface in libzfs. Other variants of this approach involve creating node.js bindings for the same function. If this path is chosen, it would a private utility in SmartOS.
  2. Extend zfs create to have a dry-run mode that is as consistent as possible with other zfs commands' dry-run modes.

This PR implements the second approach.

Description

Other commands that implement a dry-run feature implement verbose and parseable output with -v and -p. With zfs create, -p is used to create parent dataset(s), so -P is used. The following is added to zfs create in zfs(8):

       -n  Do a dry-run ("No-op") creation.  No datasets will be created.  This is
           useful in conjunction with the -v or -P flags to validate properties
           that are passed via -o options and those implied by other options.  The
           actual dataset creation can still fail due to insufficient privileges
           or available capacity.

       -P  Print machine-parseable verbose information about the created dataset.

       -v  Print verbose information about the created dataset.

The following examples illustrate the behavior:

Example 1: -n without -v or -P

# zfs create -nV 400t zones/notcreated
# echo $?
0
# zfs list zones/notcreated
cannot open 'zones/notcreated': dataset does not exist

Example 2: -n with -v

# zfs create -nv -V 1g zones/foo
would create zones/foo
	volsize=1073741824
	refreservation=1109393408

Example 3: -n with -P

In this case, the white space between tokens is a single tab.

# zfs create -Pv -V 1g zones/foo
create	zones/foo
property	volsize	1073741824
property	refreservation	1109393408

Example 4: -P without -n

While there's not a great use case for this, it does work:

# zfs list -Ho name,type -r zones/foo
cannot open 'zones/foo': dataset does not exist
# zfs create -pP -V 100m zones/foo/vol1
create_ancestors        zones/foo/vol1
create  zones/foo/vol1
property        volsize 104857600
property        refreservation  110362624
# zfs list -Ho name,type -r zones/foo
zones/foo       filesystem
zones/foo/vol1  volume

How Has This Been Tested?

Full test suite runs with this change have been performed on SmartOS. scripts/zfs-tests.sh -v -d /data -T zfs_create reports no failures on CentOS 7.

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:

@behlendorf behlendorf added the Status: Design Review Needed Architecture or design is under discussion label Jul 1, 2019
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.

Opting for the second solution here makes a lot of sense to me. I was a little surprised this wasn't already supported, so I'm happy to see this!

cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved
cmd/zfs/zfs_main.c Show resolved Hide resolved
lib/libzfs/libzfs_dataset.c Outdated Show resolved Hide resolved
man/man8/zfs.8 Show resolved Hide resolved
tests/runfiles/linux.run Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 2, 2019

Codecov Report

Merging #8974 into master will decrease coverage by 0.08%.
The diff coverage is 87.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8974      +/-   ##
==========================================
- Coverage   78.62%   78.53%   -0.09%     
==========================================
  Files         401      401              
  Lines      120158   120186      +28     
==========================================
- Hits        94476    94391      -85     
- Misses      25682    25795     +113
Flag Coverage Δ
#kernel 79.45% <ø> (+0.04%) ⬆️
#user 65.95% <87.17%> (-0.3%) ⬇️

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 e5db313...5cacbad. Read the comment docs.

@behlendorf
Copy link
Contributor

@mgerdts would you mind rebasing this on master now that #8973 has been merged.

@mgerdts mgerdts force-pushed the OS-7867 branch 2 times, most recently from b2441cf to b318f3b Compare July 8, 2019 12:53
@ghost
Copy link

ghost commented Jul 9, 2019

This will be a handy feature. A question though -
Example 1: Is there a rationale/precedent for returning 0 when it would have failed?

@mgerdts
Copy link
Contributor Author

mgerdts commented Jul 10, 2019

This will be a handy feature. A question though -
Example 1: Is there a rationale/precedent for returning 0 when it would have failed?

Excellent question. This relates to @behlendorf's comment:

Similar to the zpool create dry-run command I think it would be a good idea to include the following additional sentence. "The actual dataset creation can still fail due to insufficient privileges or available capacity."

I made those changes to the man page but did not update PR with the same text. The PR now matches the man page update.

man/man8/zfs.8 Show resolved Hide resolved
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Design Review Needed Architecture or design is under discussion labels Jul 15, 2019
@behlendorf
Copy link
Contributor

Thanks everyone for the feedback. @mgerdts would you just rebasing this on the latest master, squashing the commits, and adding your signed-off-by. Then this looks ready to integrate.

Adds the ability to sanity check zfs create arguments and to see the
value of any additional properties that will local to the dataset.  For
example, automation that may need to adjust quota on a parent filesystem
before creating a volume may call `zfs create -nP -V <size> <volume>` to
obtain the value of refreservation.  This adds the following options to
zfs create:
- -n dry-run (no-op)
- -v verbose
- -P parseable (implies verbose)

Signed-off-by: Mike Gerdts <mike.gerdts@joyent.com>
@behlendorf behlendorf merged commit d45d7f0 into openzfs:master Jul 16, 2019
TulsiJain pushed a commit to TulsiJain/zfs that referenced this pull request Jul 20, 2019
Adds the ability to sanity check zfs create arguments and to see the
value of any additional properties that will local to the dataset.  For
example, automation that may need to adjust quota on a parent filesystem
before creating a volume may call `zfs create -nP -V <size> <volume>` to
obtain the value of refreservation.  This adds the following options to
zfs create:

- -n dry-run (no-op)
- -v verbose
- -P parseable (implies verbose)

Reviewed-by: Ryan Moeller <ryan@ixsystems.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Matt Ahrens <matt@delphix.com>
Reviewed-by: Jerry Jelinek <jerry.jelinek@joyent.com>
Signed-off-by: Mike Gerdts <mike.gerdts@joyent.com>
Closes openzfs#8974
TulsiJain pushed a commit to TulsiJain/zfs that referenced this pull request Jul 20, 2019
Adds the ability to sanity check zfs create arguments and to see the
value of any additional properties that will local to the dataset.  For
example, automation that may need to adjust quota on a parent filesystem
before creating a volume may call `zfs create -nP -V <size> <volume>` to
obtain the value of refreservation.  This adds the following options to
zfs create:

- -n dry-run (no-op)
- -v verbose
- -P parseable (implies verbose)

Reviewed-by: Ryan Moeller <ryan@ixsystems.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Matt Ahrens <matt@delphix.com>
Reviewed-by: Jerry Jelinek <jerry.jelinek@joyent.com>
Signed-off-by: Mike Gerdts <mike.gerdts@joyent.com>
Closes openzfs#8974
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.

3 participants