zpool: Dryrun fails to list some devices #11167

AttilaFueloep · 2020-11-06T07:10:56Z

Motivation and Context

zpool create -n fails to list cache and spare vdevs and zpool add -n fails to list spare devices.

Description

Add them to the list.

Closes #11122

How Has This Been Tested?

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 OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

AttilaFueloep · 2020-11-06T07:12:04Z

Regarding the tests, I'm not sure if I should add one and where to put it.

AttilaFueloep · 2020-11-06T12:32:28Z

On Debian 10 one ztest run dies with SIGABRT. Unfortunately there is no ztest.gdboutput, so I can't tell if this is related or not. The FreeBSD test error seems unrelated (compression).

ghost

Don't worry, the test failures are unrelated.

behlendorf

Thanks! It's good to see this finally fixed. Regarding where to put a test case for this I'd suggest creating a new tests/functional/cli_root/zpool_create/zpool_create_dryrun.ksh test case. We already have a similar tests/functional/cli_root/zfs_create/zfs_create_dryrun.ksh for datasets and this would complement it nicely.

AttilaFueloep · 2020-11-07T08:58:10Z

All right, I will add test cases. The zpool create dryrun test case doesn't need root, so I guess it's better placed below cli_user, right? The zpool add test case on the other hand needs to create a pool, so I will put it below cli_root.

behlendorf · 2020-11-07T19:15:53Z

The drurun test case might need root to check for labels on the block devices or perhaps perform other privileged checks. If needed, I think it'd be fine to put it under cli_root with the other similar tests.

AttilaFueloep · 2020-11-09T16:24:29Z

Putting them under cli_root has also the added benefit to keep them all in one place, so I will go that route.

Unfortunately there are still some issues left.

  pool: foo
 state: ONLINE
config:

	NAME           STATE     READ WRITE CKSUM
	foo            ONLINE       0     0     0
	  mirror-0     ONLINE       0     0     0
	    /tmp/dev1  ONLINE       0     0     0
	    /tmp/dev2  ONLINE       0     0     0
	special	
	  mirror-1     ONLINE       0     0     0
	    /tmp/dev3  ONLINE       0     0     0
	    /tmp/dev4  ONLINE       0     0     0

$ zpool split -n foo bar
would create 'bar' with the following layout:

	bar
	  /tmp/dev2
	  /tmp/dev4

But that should obviously read

would create 'bar' with the following layout:

	bar
	  /tmp/dev2
	special
	  /tmp/dev4

So I've to look into this further. I've also seen

would create 'bar' with the following layout:

	bar
	  /tmp/dev2
	  hole
	  /tmp/dev4

I've also seen hole vdevs with zpool add -n Not sure how I got there though and I can't currently reproduce it. IIRC those hole vdevs are some kind of placeholders and I wonder how we should handle them.

behlendorf · 2020-11-09T17:18:19Z

Good finds. Regarding the hole vdev's they're the result of removing a log vdev, they're left as a placeholder as you said in order to preserve the top-level vdev id order. They should never be output by the zpool commands when printing the config.

AttilaFueloep · 2020-11-09T18:03:19Z

Hah, the order matters then! zpool split adds log vdevs as holes into the config, so:

$ sudo zpool create foo mirror /tmp/dev1 /tmp/dev2 log mirror /tmp/dev3 /tmp/dev4 special mirror /tmp/dev5 /tmp/dev6
$zpool split -n foo bar
would create 'bar' with the following layout:

	bar
	  /tmp/dev2
	  hole
	  /tmp/dev6
$sudo zpool destroy foo
$ sudo zpool create foo mirror /tmp/dev1 /tmp/dev2 special mirror /tmp/dev5 /tmp/dev6 log mirror /tmp/dev3 /tmp/dev4
$ zpool split -n foo bar
would create 'bar' with the following layout:

	bar
	  /tmp/dev2
	  /tmp/dev6

That's easy to fix, the missing special seems a bit trickier but I think I've an idea.

AttilaFueloep · 2020-11-11T15:13:19Z

I wonder if we should set the allocations bias on the vdevs splited off regardless of the dryrun setting? Split does the right thing without them though. Any thoughts?

Otherwise I think I'm done with the code fixes. Next is adding test cases but it may take a while since I need to get acquainted to the ZTS.

behlendorf · 2020-11-11T17:46:33Z

That code fixes look good, let me know if you have any questions about the ZTS. The usual things to double check when adding a new test are that it's included in the Makefile.am and common.run file.

As for setting the allocation bias regardless of dryrun I think we probably want to limit ourselves the fixing the output for this PR.

AttilaFueloep · 2020-11-11T18:16:19Z

That code fixes look good, let me know if you have any questions about the ZTS. The usual things to double check when adding a new test are that it's included in the Makefile.am and common.run file.

Thanks, I'll let you know should I have questions.

As for setting the allocation bias regardless of dryrun I think we probably want to limit ourselves the fixing the output for this PR.

Yes, this makes sense.

behlendorf · 2020-12-01T23:21:05Z

@AttilaFueloep any headway? We could probably live without the additional test cases (clearly we didn't have them before), but the zpool_add_003_pos ZTS is related and will need to be sorted out.

AttilaFueloep · 2020-12-02T01:49:59Z

Sorry for the delay, I'm currently trying to sort out why I'm seeing Warning: Test 'zpool_add_dryrun_output.ksh' removed from TestGroup '.../cli_root/zpool_add' because it failed verification. while trying to debug the new tests with scripts/zfs-tests.sh -t .... I've already a fix for the failing test case, going to push now.

ghost · 2020-12-02T14:18:55Z

Please rebase before the next push, too. It should resolve the FreeBSD head failure.

behlendorf · 2020-12-02T16:53:09Z

Warning: Test 'zpool_add_dryrun_output.ksh' removed from TestGroup '.../cli_root/zpool_add

No problem! The most likely reasons I've seen for this are usually either a typo of the test name in the runfile, or incorrect permission bits of the new test case (should be 755), or forgetting to include the new test case in the Makefile.am so it fails to get included by make dist. Perhaps it's one of these things?

AttilaFueloep · 2020-12-02T17:15:55Z

@freqlabs Sure, will do.

@behlendorf Thanks Brian, indeed the problem was missing execute permissions on this file. Could have sworn I did a chmod -x but obviously I missed it.

AttilaFueloep · 2020-12-02T17:17:32Z

Still polishing the tests a bit, going to push later today.

`zpool create -n` fails to list cache and spare vdevs. `zpool add -n` fails to list spare devices. `zpool split -n` fails to list `special` and `dedup` labels. `zpool add -n` and `zpool split -n` shouldn't list hole devices. Signed-off-by: Attila Fülöp <[email protected]> Closes openzfs#11122

AttilaFueloep · 2020-12-03T07:44:39Z

Added tests, squashed and rebased.

tests/zfs-tests/tests/functional/cli_root/zpool_create/cleanup.ksh

behlendorf · 2020-12-03T18:13:46Z

tests/zfs-tests/tests/functional/cli_root/zpool_create/zpool_create_dryrun_output.ksh

+# 1. Create a storage pool
+#
+
+typeset dev_size=4M


4M is below the minimum vdev size. Since this is just a dry-run nothing actually fails, but I'd still suggest bumping this to $SPA_MINDEVSIZE. Since you're using truncate to create the file vdevs it won't actually increase the space used by the test.

The intention was indeed to save space, but you're right since this is truncate it won't waste any space. I somehow missed that fact. Will change back to $SPA_MINDEVSIZE.

Signed-off-by: Attila Fülöp <[email protected]>

codecov · 2020-12-04T02:44:53Z

Codecov Report

Merging #11167 (8f158ae) into master (0aacde2) will decrease coverage by 0.38%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master   #11167      +/-   ##
==========================================
- Coverage   79.87%   79.49%   -0.39%     
==========================================
  Files         400      400              
  Lines      127524   127524              
==========================================
- Hits       101863   101369     -494     
- Misses      25661    26155     +494

Flag	Coverage Δ
kernel	`80.60% <75.00%> (-0.07%)`	⬇️
user	`63.46% <25.00%> (-1.79%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
module/zfs/vdev_queue.c	`94.70% <75.00%> (-0.53%)`	⬇️
module/os/linux/spl/spl-zlib.c	`55.35% <0.00%> (-28.58%)`	⬇️
module/zcommon/zfs_fletcher.c	`68.09% <0.00%> (-10.20%)`	⬇️
cmd/ztest/ztest.c	`70.86% <0.00%> (-7.75%)`	⬇️
module/lua/lmem.c	`83.33% <0.00%> (-4.17%)`	⬇️
module/zcommon/zfs_fletcher_superscalar.c	`97.05% <0.00%> (-2.95%)`	⬇️
module/icp/algs/modes/gcm.c	`78.14% <0.00%> (-2.82%)`	⬇️
module/zfs/dsl_synctask.c	`92.30% <0.00%> (-2.57%)`	⬇️
lib/libzpool/util.c	`93.67% <0.00%> (-2.54%)`	⬇️
module/os/linux/zfs/vdev_disk.c	`78.21% <0.00%> (-2.15%)`	⬇️
... and 61 more

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 0aacde2...e2129e8. Read the comment docs.

Follow up fix for 0cb40fa. Remove unused variables and add missed cleanup. Signed-off-by: Attila Fülöp <[email protected]>

Follow up fix for 0cb40fa. Remove unused variables, don't source unused libs and add missed cleanup. Signed-off-by: Attila Fülöp <[email protected]>

Follow up fix for 0cb40fa. Remove unused variables, don't source unused libs and add missed cleanup. Reviewed-by: Ryan Moeller <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Attila Fülöp <[email protected]> Closes #11311

`zpool create -n` fails to list cache and spare vdevs. `zpool add -n` fails to list spare devices. `zpool split -n` fails to list `special` and `dedup` labels. `zpool add -n` and `zpool split -n` shouldn't list hole devices. Reviewed-by: Ryan Moeller <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Attila Fülöp <[email protected]> Closes openzfs#11122 Closes openzfs#11167

Follow up fix for 0cb40fa. Remove unused variables, don't source unused libs and add missed cleanup. Reviewed-by: Ryan Moeller <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Attila Fülöp <[email protected]> Closes openzfs#11311

AttilaFueloep · 2021-01-06T23:43:49Z

@behlendorf Not sure if this and #11311 should be included in a 2.0 / 0.8 point release as well?

behlendorf · 2021-01-07T01:01:36Z

@AttilaFueloep yup, I think they'd make sense for the next 2.0 point release. Would you mind opening a new PR against the zfs-2.0.2-staging branch with both of these changes.

AttilaFueloep · 2021-01-07T02:14:07Z

Sure, will do.

`zpool create -n` fails to list cache and spare vdevs. `zpool add -n` fails to list spare devices. `zpool split -n` fails to list `special` and `dedup` labels. `zpool add -n` and `zpool split -n` shouldn't list hole devices. Reviewed-by: Ryan Moeller <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Attila Fülöp <[email protected]> Closes openzfs#11122 Closes openzfs#11167

Follow up fix for 0cb40fa. Remove unused variables, don't source unused libs and add missed cleanup. Reviewed-by: Ryan Moeller <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Attila Fülöp <[email protected]> Closes openzfs#11311

`zpool create -n` fails to list cache and spare vdevs. `zpool add -n` fails to list spare devices. `zpool split -n` fails to list `special` and `dedup` labels. `zpool add -n` and `zpool split -n` shouldn't list hole devices. Reviewed-by: Ryan Moeller <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Attila Fülöp <[email protected]> Closes #11122 Closes #11167

Follow up fix for 0cb40fa. Remove unused variables, don't source unused libs and add missed cleanup. Reviewed-by: Ryan Moeller <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Attila Fülöp <[email protected]> Closes #11311

`zpool create -n` fails to list cache and spare vdevs. `zpool add -n` fails to list spare devices. `zpool split -n` fails to list `special` and `dedup` labels. `zpool add -n` and `zpool split -n` shouldn't list hole devices. Reviewed-by: Ryan Moeller <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Attila Fülöp <[email protected]> Closes openzfs#11122 Closes openzfs#11167

Follow up fix for 0cb40fa. Remove unused variables, don't source unused libs and add missed cleanup. Reviewed-by: Ryan Moeller <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Attila Fülöp <[email protected]> Closes openzfs#11311

`zpool create -n` fails to list cache and spare vdevs. `zpool add -n` fails to list spare devices. `zpool split -n` fails to list `special` and `dedup` labels. `zpool add -n` and `zpool split -n` shouldn't list hole devices. Reviewed-by: Ryan Moeller <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Attila Fülöp <[email protected]> Closes openzfs#11122 Closes openzfs#11167

Follow up fix for 0cb40fa. Remove unused variables, don't source unused libs and add missed cleanup. Reviewed-by: Ryan Moeller <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Attila Fülöp <[email protected]> Closes openzfs#11311

ghost approved these changes Nov 6, 2020

View reviewed changes

ghost added Component: Userspace user space functionality Status: Code Review Needed Ready for review and testing labels Nov 6, 2020

behlendorf approved these changes Nov 6, 2020

View reviewed changes

AttilaFueloep force-pushed the zfs-fix-zpool-create-dryrun branch from 56c1fc9 to 04fe8b0 Compare December 3, 2020 07:44

ghost reviewed Dec 3, 2020

View reviewed changes

tests/zfs-tests/tests/functional/cli_root/zpool_create/cleanup.ksh Outdated Show resolved Hide resolved

behlendorf reviewed Dec 3, 2020

View reviewed changes

address review feedback, put test in the right group

e2129e8

Signed-off-by: Attila Fülöp <[email protected]>

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Dec 4, 2020

behlendorf merged commit 0cb40fa into openzfs:master Dec 4, 2020

AttilaFueloep added a commit to AttilaFueloep/zfs that referenced this pull request Dec 5, 2020

zpool: Two small fixes for openzfs#11167

d08eed3

Follow up fix for 0cb40fa. Remove unused variables and add missed cleanup. Signed-off-by: Attila Fülöp <[email protected]>

AttilaFueloep mentioned this pull request Dec 8, 2020

zpool: Three small fixes for #11167 #11311

Merged

12 tasks

AttilaFueloep mentioned this pull request Jan 8, 2021

zpool: Backport dryrun fixes (#11167 and #11311) #11442

Merged

13 tasks

AttilaFueloep mentioned this pull request Mar 21, 2021

Adding spare dry-run does not show the result #10889

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zpool: Dryrun fails to list some devices #11167

zpool: Dryrun fails to list some devices #11167

AttilaFueloep commented Nov 6, 2020

AttilaFueloep commented Nov 6, 2020

AttilaFueloep commented Nov 6, 2020 •

edited

Loading

ghost left a comment

behlendorf left a comment

AttilaFueloep commented Nov 7, 2020

behlendorf commented Nov 7, 2020

AttilaFueloep commented Nov 9, 2020 •

edited

Loading

behlendorf commented Nov 9, 2020

AttilaFueloep commented Nov 9, 2020

AttilaFueloep commented Nov 11, 2020

behlendorf commented Nov 11, 2020

AttilaFueloep commented Nov 11, 2020

behlendorf commented Dec 1, 2020

AttilaFueloep commented Dec 2, 2020

ghost commented Dec 2, 2020 •

edited by ghost

Loading

behlendorf commented Dec 2, 2020

AttilaFueloep commented Dec 2, 2020

AttilaFueloep commented Dec 2, 2020

AttilaFueloep commented Dec 3, 2020

behlendorf Dec 3, 2020

AttilaFueloep Dec 3, 2020

codecov bot commented Dec 4, 2020 •

edited

Loading

AttilaFueloep commented Jan 6, 2021

behlendorf commented Jan 7, 2021

AttilaFueloep commented Jan 7, 2021

zpool: Dryrun fails to list some devices #11167

zpool: Dryrun fails to list some devices #11167

Conversation

AttilaFueloep commented Nov 6, 2020

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

AttilaFueloep commented Nov 6, 2020

AttilaFueloep commented Nov 6, 2020 • edited Loading

ghost left a comment

Choose a reason for hiding this comment

behlendorf left a comment

Choose a reason for hiding this comment

AttilaFueloep commented Nov 7, 2020

behlendorf commented Nov 7, 2020

AttilaFueloep commented Nov 9, 2020 • edited Loading

behlendorf commented Nov 9, 2020

AttilaFueloep commented Nov 9, 2020

AttilaFueloep commented Nov 11, 2020

behlendorf commented Nov 11, 2020

AttilaFueloep commented Nov 11, 2020

behlendorf commented Dec 1, 2020

AttilaFueloep commented Dec 2, 2020

ghost commented Dec 2, 2020 • edited by ghost Loading

behlendorf commented Dec 2, 2020

AttilaFueloep commented Dec 2, 2020

AttilaFueloep commented Dec 2, 2020

AttilaFueloep commented Dec 3, 2020

behlendorf Dec 3, 2020

Choose a reason for hiding this comment

AttilaFueloep Dec 3, 2020

Choose a reason for hiding this comment

codecov bot commented Dec 4, 2020 • edited Loading

Codecov Report

AttilaFueloep commented Jan 6, 2021

behlendorf commented Jan 7, 2021

AttilaFueloep commented Jan 7, 2021

AttilaFueloep commented Nov 6, 2020 •

edited

Loading

AttilaFueloep commented Nov 9, 2020 •

edited

Loading

ghost commented Dec 2, 2020 •

edited by ghost

Loading

codecov bot commented Dec 4, 2020 •

edited

Loading