Remove requirement for -d 1 for zfs list and zfs get with bookmarks #9589

InsanePrawn · 2019-11-14T21:17:54Z

as discussed in #9574:

df58307 removed the need to specify -d 1 when zfs list and zfs get are

called with -t snapshot on a datset. This commit extends the same
behaviour to -t bookmark.

This commit also introduces the 'snap' shorthand for snapshots from
zfs list to zfs get.

Signed-off-by: InsanePrawn insane.prawny@gmail.com

Motivation and Context

Basically UX.

Description

zfs list and zfs get can now be used with -t snapshot or -t bookmark to list snapshots/bookmarks of a dataset without the need to specify -d 1 anymore.
Also, you can now use zfs get -t snap instead of zfs get -t snapshot.

How Has This Been Tested?

Compiled, manually tested.

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:

My code follows the ZFS on Linux code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
All new and existing tests passed.
All commit messages are properly formatted and contain Signed-off-by.

df58307 removed the need to specify -d 1 when zfs list and zfs get are called with -t snapshot on a datset. This commit extends the same behaviour to -t bookmark. This commit also introduces the 'snap' shorthand for snapshots from zfs list to zfs get. Signed-off-by: InsanePrawn <insane.prawny@gmail.com>

codecov · 2019-11-15T05:51:32Z

Codecov Report

Merging #9589 into master will decrease coverage by 4.5%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master    #9589      +/-   ##
==========================================
- Coverage   79.23%   74.73%   -4.51%     
==========================================
  Files         419      389      -30     
  Lines      123696   120898    -2798     
==========================================
- Hits        98014    90352    -7662     
- Misses      25682    30546    +4864

Flag	Coverage Δ
#kernel	`79.78% <ø> (-0.01%)`	⬇️
#user	`61.12% <75%> (-5.87%)`	⬇️

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 64c77c4...5555f39. Read the comment docs.

Ornias1993 · 2019-11-15T10:50:38Z

I went over the failed test: doesn't seem to be related to this PR.
"zpool_reopen_001_pos" failed, in a weird spot after multiple hours during basic testpool creation:
one or more devices is currently unavailable

"zpool_reopen_002_pos" ran fine, in the right spot, doing the same thing. As do all other tests doing poolcreation.

Ornias1993

Very neat solution, love how clean the lack of break in case 2 leads to case 3 being used for both case 2 (Snapshot) and case 3 (snap). Very KISS

behlendorf

Thanks, can you also add a basic test for the snap alias and bookmark in cli_root/zfs_get/zfs_get_009_pos.ksh test case. The same as what as done in df58307.

tcaputi

LGTM. Definitely add the test cases @behlendorf asked for. Thanks for extending this.

Signed-off-by: InsanePrawn <insane.prawny@gmail.com>

InsanePrawn · 2019-11-16T18:54:20Z

I've taken a stab at extending the tests in the easiest ways that seemed remotely reasonable to me.

Does the extension of depth_fs_setup look sane to everybody?
I've run cli_root/zfs_get/zfs_get_009_pos.ksh which passed multiple times on my system. cli_user/zfs_list/setup.ksh also uses depth_fs_setup, and even though I ran the cli_user/zfs_list suite and it passed, it'd be nice if someone verified I didn't break any of those tests.
Runfile used:

#
# This file and its contents are supplied under the terms of the
# Common Development and Distribution License ("CDDL"), version 1.0.
# You may only use this file in accordance with the terms of version
# 1.0 of the CDDL.
#
# A full copy of the text of the CDDL should have accompanied this
# source.  A copy of the CDDL is also available via the Internet at
# http://www.illumos.org/license/CDDL.
#

[DEFAULT]
pre = setup
quiet = False
pre_user = root
user = root
timeout = 600
post_user = root
post = cleanup
outputdir = /var/tmp/test_results
tags = ['functional']

[tests/functional/cli_root/zfs_get]
tests = ['zfs_get_009_pos']
tags = ['functional', 'cli_root', 'zfs_get']

[tests/functional/cli_user/zfs_list]
tests = ['zfs_list_001_pos', 'zfs_list_002_pos', 'zfs_list_003_pos',
    'zfs_list_004_neg', 'zfs_list_007_pos', 'zfs_list_008_neg']
user =
tags = ['functional', 'cli_user', 'zfs_list']

behlendorf

Thank's for adding the test coverage, this looks right to me.

tcaputi

LGTM

df58307 removed the need to specify -d 1 when zfs list and zfs get are called with -t snapshot on a datset. This commit extends the same behaviour to -t bookmark. This commit also introduces the 'snap' shorthand for snapshots from zfs list to zfs get. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Tom Caputi <tcaputi@datto.com> Reviewed-by: Kjeld Schouten <kjeld@schouten-lebbing.nl> Signed-off-by: InsanePrawn <insane.prawny@gmail.com> Closes openzfs#9589

df58307 removed the need to specify -d 1 when zfs list and zfs get are called with -t snapshot on a datset. This commit extends the same behaviour to -t bookmark. This commit also introduces the 'snap' shorthand for snapshots from zfs list to zfs get. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Tom Caputi <tcaputi@datto.com> Reviewed-by: Kjeld Schouten <kjeld@schouten-lebbing.nl> Signed-off-by: InsanePrawn <insane.prawny@gmail.com> Closes #9589

Ornias1993 approved these changes Nov 15, 2019

View reviewed changes

behlendorf added the Status: Code Review Needed Ready for review and testing label Nov 15, 2019

behlendorf requested a review from tcaputi November 15, 2019 18:01

behlendorf reviewed Nov 15, 2019

View reviewed changes

tcaputi approved these changes Nov 15, 2019

View reviewed changes

Add tests for 'zfs get -t snap' and 'zfs list -t bookmark <dataset>'

5555f39

Signed-off-by: InsanePrawn <insane.prawny@gmail.com>

behlendorf approved these changes Nov 18, 2019

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Nov 18, 2019

behlendorf requested a review from tcaputi November 18, 2019 21:09

tcaputi approved these changes Nov 19, 2019

View reviewed changes

behlendorf merged commit 8221bcf into openzfs:master Nov 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove requirement for -d 1 for zfs list and zfs get with bookmarks #9589

Remove requirement for -d 1 for zfs list and zfs get with bookmarks #9589

InsanePrawn commented Nov 14, 2019

codecov bot commented Nov 15, 2019 •

edited

Ornias1993 commented Nov 15, 2019

Ornias1993 left a comment •

edited

behlendorf left a comment

tcaputi left a comment

InsanePrawn commented Nov 16, 2019 •

edited

behlendorf left a comment

tcaputi left a comment

Remove requirement for -d 1 for zfs list and zfs get with bookmarks #9589

Remove requirement for -d 1 for zfs list and zfs get with bookmarks #9589

Conversation

InsanePrawn commented Nov 14, 2019

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

codecov bot commented Nov 15, 2019 • edited

Codecov Report

Ornias1993 commented Nov 15, 2019

Ornias1993 left a comment • edited

Choose a reason for hiding this comment

behlendorf left a comment

Choose a reason for hiding this comment

tcaputi left a comment

Choose a reason for hiding this comment

InsanePrawn commented Nov 16, 2019 • edited

behlendorf left a comment

Choose a reason for hiding this comment

tcaputi left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 15, 2019 •

edited

Ornias1993 left a comment •

edited

InsanePrawn commented Nov 16, 2019 •

edited