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

Clarify 'zpool remove' restrictions #7893

Merged
merged 1 commit into from
Sep 18, 2018
Merged

Conversation

behlendorf
Copy link
Contributor

Motivation and Context

Make the documentation clear about the restrictions of device removal. #7880

Description

Update zpool(8) to clarify what type of vdevs may be safely removed and that the existence of any top-level raidz device which is part of the primary pool will prevent device removal.

How Has This Been Tested?

Proofread.

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.
  • Change has been approved by a ZFS on Linux member.

@DeHackEd
Copy link
Contributor

While I don't condone non-redundant pools, disks in a pure RAID-0 configuration can also be removed.

@behlendorf
Copy link
Contributor Author

Good point, refreshed and slightly reworded.

     zpool remove [-np] pool device...
             Removes the specified device from the pool.  This command supports
             removing hot spare, cache, log, and both mirrored and non-redundant
             primary top-level vdevs, this includes dedup and special vdevs.  When
             the primary pool storage includes a top-level raidz vdev only hot
             spare, cache, and log devices can be removed.

@dweeezil
Copy link
Contributor

As long as this is being clarified, I think it might be a good idea to refer to the device_removal feature by name:

$ diff -u zpool.8 zpool.8.mod
--- zpool.8	2018-09-13 14:15:02.443896936 -0500
+++ zpool.8.mod	2018-09-13 14:21:56.936155588 -0500
@@ -1940,8 +1940,10 @@
 the background.
 The removal progress can be monitored with
 .Nm zpool Cm status.
-This feature must be enabled to be used, see
-.Xr zpool-features 5
+The
+.Sy device_removal
+feature flag must be enabled to remove a top-level vdev, see
+.Xr zpool-features 5 .
 .Pp
 A mirrored top-level device (log or data) can be removed by specifying the top-level mirror for the
 same.

Even some more additional text might be prudent to make it clear that removing a top-level vdev has compatibility ramifications as opposed to removing a log or cache device.

@Lady-Galadriel
Copy link

LGTM, and @dweeezil's comment about restrictions is probably valid as well.

@behlendorf
Copy link
Contributor Author

Refreshed to include @dweeezil's additions.

man/man8/zpool.8 Outdated
This command currently only supports removing hot spares, cache, log
devices and mirrored top-level vdevs (mirror of leaf devices); but not raidz.
This command supports removing hot spare, cache, log, and both mirrored and
non-redundant primary top-level vdevs, this includes dedup and special vdevs.
Copy link
Member

Choose a reason for hiding this comment

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

I think it would flow better with "this includes" -> "including"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, that flows a little better.

man/man8/zpool.8 Outdated
This command supports removing hot spare, cache, log, and both mirrored and
non-redundant primary top-level vdevs, this includes dedup and special vdevs.
When the primary pool storage includes a top-level raidz vdev only hot spare,
cache, and log devices can be removed.
Copy link
Member

Choose a reason for hiding this comment

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

I get this, but IMO it would be more easily understood to say When the primary pool storage includes a raidz vdev, other top-level vdevs may not be removed.

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 considered a wording very similar, but decided against it in favor of this clunkier more precise wording. My concern was that it implied spare, cache, and log devices, which look like top-level vdevs in the zpool status output, could not be removed. But I'd love a better way to phase this.

Update zpool(8) to clarify what type of vdevs may be safely
removed and that the existence of any top-level raidz device
which is part of the primary pool will prevent device removal.

Signed-off-by: Tim Chase <tim@chase2k.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) Reviewed labels Sep 17, 2018
@behlendorf behlendorf merged commit 2ced3cf into openzfs:master Sep 18, 2018
@behlendorf behlendorf deleted the issue-7880 branch April 19, 2021 19:28
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.

None yet

5 participants