Allow for '-o feature@<feature>=disable' on the command line. #3465

FransUrbo · 2015-06-02T16:57:52Z

Sometimes it is desired to specifically disable or enable a feature directly on the 'zpool create' command line.

If a disable have been specified, the default for all not mentioned features is 'enabled'.

If a enable have been specified, the default for all the not mentioned features is 'disabled'.

Signed-off-by: Turbo Fredriksson turbo@bayour.com
Closes #3460

FransUrbo · 2015-06-02T16:58:36Z

Note that this still contains debugging output and fails the cstyle. But input on it should be possible.

behlendorf

This change needs to be rebased on master, have the man pages updated, and a test case added to ensure it's behaving as designed. That will make it much easier to review and get a feel for the proposed command line interface.

FransUrbo · 2016-09-19T12:24:04Z

Apparently the test cases already exists:

tests/zfs-tests/tests/functional/cli_root/zpool_create/zpool_create_features_001_pos.ksh:

log_assert "'zpool create' creates pools with all features enabled"

log_must $ZPOOL create -f $TESTPOOL $DISKS
check_features
log_must $ZPOOL destroy -f $TESTPOOL

log_must $ZPOOL create -f -o feature@async_destroy=enabled $TESTPOOL $DISKS
check_features

log_pass "'zpool create' creates pools with all features enabled"

I'm going to setup a couple of test machines to try this, but do these tests run cleanly/successfully now?

behlendorf · 2016-09-19T17:03:45Z

All 4 of the zpool_create_features test cases do regularly pass, here's a link to a recent PR rebased on master. It looks like you'd still want to add a zpool_create_features_005_pos which verifies the more flexible syntax works.

FransUrbo · 2016-09-20T11:11:51Z

Ok, so if this works already, then maybe we don't need this PR?

behlendorf · 2016-09-20T18:33:29Z

The test case zpool_create_features_004_neg.ksh explicitly verifies that the only syntax that works is feature@async_destroy=enabled. Both feature@async_destroy=disabled and feature@async_destroy=active are expected to fail. I'm OK with dropping this but I thought you wanted to support the feature@async_destroy=disabled syntax.

FransUrbo · 2016-09-20T21:55:22Z

Ah, right. Is that something we want? I find it kind'a useful, but I create pools so seldom, I haven't used it (this functionality) in a long time..

behlendorf · 2016-09-20T22:47:26Z

It could prove handy particularly as we accumulate more features but it's definitely not crucial.

Unless you're excited to dive in to implement this I suggest we close it for now. We can always revisit implementation this when it's really needed or someone wants to work on it.

FransUrbo · 2016-09-20T22:53:16Z

No, that's ok. The code is there, I just need to create the test cases for it.

FransUrbo · 2016-09-21T12:25:06Z

Is there a way to, programmatically, discover what features are available? So that I don't have to hard code them in the test script(s)..

I found a couple of ways to do it, but they all require an existing pool to query (zpool get all | grep 'feature@' | while read line; do set -- $(echo "${line}"); echo "${2}"; done | sort | uniq), but I don't think I can expect an existing pool - that's what I'm [about to] create!

FransUrbo · 2016-09-21T13:30:22Z

I'm looking over the other script, and I think this will break zpool_create_features_001_pos.ksh. It assumes that when using -o feature@async_destroy=enabled, all features will still be in the enabled state (i.e., it's supposed to be a no-op). But with this PR, they will be disabled!

behlendorf · 2016-09-21T19:34:14Z

Is there a way to, programmatically, discover what features are available?

Aside from the method you described there isn't an easy way to do this. Although in this case I don't think it's necessary, it would be OK to just use a couple existing features to validate it's behaving as desired. It would be ideal to verify them all but even verifying a handful provides decent test coverage.

-o feature@async_destroy=enabled, all features will still be in the enabled state (i.e., it's supposed to be a no-op). But with this PR, they will be disabled!

I think this can be done without breaking any of the existing test cases. And that's critical because we can't break existing scripts which depend on the current behavior. This PR just needs to extend that logic.

Speaking for myself I would expect this patch to simply allow disabled to be a valid option. Specifying a value of enabled or disabled would simply act as an override to the default behavior. Which is by default all features are enabled unless -d in specified in which case all features are disabled.

Here are a few concrete examples:

# ONLY large_block enabled (this works today)
$ zpool create -d -o feature@large_blocks=enabled mypool

# All features EXCEPT large_blocks are enabled (this doesn't work today)
$ zpool create -o feature@large_blocks=disabled mypool

@FransUrbo did you have something different in mind? @DeHackEd would this be inline with your expectations?

DeHackEd · 2016-09-21T19:38:56Z

I tend to agree with @behlendorf, the best way to do this is to only disable all features if -d is specified. Maybe print a warning for using feature@something=enabled without using -d would be an idea to prevent people from making mistakes - in that situation it's a brand new pool and it can be quickly destroyed and recreated from your shell history.

FransUrbo · 2016-09-21T19:57:17Z

On Sep 21, 2016, at 8:34 PM, Brian Behlendorf wrote:

# All features EXCEPT large_blocks are enabled (this doesn't work today)
$ zpool create -o feature@large_blocks=disabled mypool
@FransUrbo did you have something different in mind? @DeHackEd would this be inline with your expectations?

Initially I had, but this actually makes more sense. I'll rework the patch and finish the test script (which would be much simpler this way) as soon as I can.

FransUrbo · 2016-09-28T22:29:59Z

I'm struggling with trying to test a specific feature as disabled and all others enabled.

How does this look:

function check_features
{
    feature="${1}"
    feature_set=false
    other_feature_set=false

    ${ZPOOL} get all ${TESTPOOL} | \
        grep feature@ | \
        while read line; do
            set -- $(echo "${line}")

            if [[ "${3}" == "enabled" || "${3}" == "active" ]]; then
                if [[ "feature@${feature}" == "${2}" ]]; then
                    feature_set=true
                else
                    other_feature_set=true
                fi
            fi
        done

    if [[ "${feature_set}" == "true" ]]; then
        # This is a success                                                                                                                                                          
        if [[ "${other_feature_set}" == "true" ]]; then
            # .. but if _any_ of the other features is enabled,                                                                                                                         
            # it's a failure!                                                                                                                                                        
            return 0
        else
            # All good - feature is enabled, all other disabled                                                                                                                      
            return 1
        fi
    else
        # Feature is not set - failure.                                                                                                                                              
        return 1
    fi
}

Then something like this to run the tests:

for feature in async_destroy bookmarks embedded_data empty_bpobj enabled_txg \
               extensible_dataset filesystem_limits hole_birth large_blocks  \
               lz4_compress spacemap_histogram
do
        log_assert "'zpool create' creates pools with ${feature} disabled"

        log_must $ZPOOL create -f -o "feature@${feature}=disabled" $TESTPOOL $DISKS
        check_features ${feature}
        log_must $ZPOOL destroy -f $TESTPOOL

        log_pass "'zpool create' creates pools with ${feature} disabled"
done

FransUrbo · 2016-09-28T23:14:59Z

Updated the PR. Need to test these new changes.

FransUrbo · 2016-09-30T16:46:31Z

Seems to be working just fine:

DebianZFS-Jessie64-Devel-Daily:/usr/src# zpool create -f -o feature@async_destroy=disabled test /dev/sd[bc]
DebianZFS-Jessie64-Devel-Daily:/usr/src# zpool get all test | grep @
test  feature@async_destroy       disabled                    local
test  feature@empty_bpobj         enabled                     local
test  feature@lz4_compress        active                      local
test  feature@spacemap_histogram  active                      local
test  feature@enabled_txg         active                      local
test  feature@hole_birth          active                      local
test  feature@extensible_dataset  enabled                     local
test  feature@embedded_data       active                      local
test  feature@bookmarks           enabled                     local
test  feature@filesystem_limits   enabled                     local
test  feature@large_blocks        enabled                     local
test  feature@large_dnode         enabled                     local
DebianZFS-Jessie64-Devel-Daily:/usr/src# zpool destroy test
DebianZFS-Jessie64-Devel-Daily:/usr/src# zpool create -f -o feature@spacemap_histogram=disabled test /dev/sd[bc]
DebianZFS-Jessie64-Devel-Daily:/usr/src# zpool get all test | grep @
test  feature@async_destroy       enabled                     local
test  feature@empty_bpobj         enabled                     local
test  feature@lz4_compress        active                      local
test  feature@spacemap_histogram  disabled                    local
test  feature@enabled_txg         active                      local
test  feature@hole_birth          active                      local
test  feature@extensible_dataset  enabled                     local
test  feature@embedded_data       active                      local
test  feature@bookmarks           enabled                     local
test  feature@filesystem_limits   enabled                     local
test  feature@large_blocks        enabled                     local
test  feature@large_dnode         enabled                     local
DebianZFS-Jessie64-Devel-Daily:/usr/src# zpool destroy test
DebianZFS-Jessie64-Devel-Daily:/usr/src# zpool create -f -o feature@large_blocks=disabled test /dev/sd[bc]
DebianZFS-Jessie64-Devel-Daily:/usr/src# zpool get all test | grep @
test  feature@async_destroy       enabled                     local
test  feature@empty_bpobj         enabled                     local
test  feature@lz4_compress        active                      local
test  feature@spacemap_histogram  active                      local
test  feature@enabled_txg         active                      local
test  feature@hole_birth          active                      local
test  feature@extensible_dataset  enabled                     local
test  feature@embedded_data       active                      local
test  feature@bookmarks           enabled                     local
test  feature@filesystem_limits   enabled                     local
test  feature@large_blocks        disabled                    local
test  feature@large_dnode         enabled                     local

behlendorf

The existing test cases are failing because all the features are not enabled for the following command.

$ sudo ./cmd/zpool/zpool create -f -o feature@async_destroy=enabled tank vdb
$ sudo ./cmd/zpool/zpool get all tank | grep feature
tank  feature@async_destroy       disabled                    local
tank  feature@empty_bpobj         disabled                    local
tank  feature@lz4_compress        disabled                    local
tank  feature@spacemap_histogram  disabled                    local
tank  feature@enabled_txg         disabled                    local
tank  feature@hole_birth          disabled                    local
tank  feature@extensible_dataset  disabled                    local
tank  feature@embedded_data       disabled                    local
tank  feature@bookmarks           disabled                    local
tank  feature@filesystem_limits   disabled                    local
tank  feature@large_blocks        disabled                    local
tank  feature@large_dnode         disabled                    local

behlendorf · 2016-09-30T19:45:47Z

man/man8/zpool.8

@@ -1,4 +1,4 @@
-'\" te
+n'\" te


Stray 'n' character.

behlendorf · 2016-09-30T19:48:30Z

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

+{
+    feature="${1}"
+    feature_set=false
+    other_feature_set=false


The scripts use the same formatting as the C. Tabs for indents and continued lines indented by 4 spaces.

Sometimes it is desired to specifically disable a feature directly on the 'zpool create' command line. Signed-off-by: Turbo Fredriksson turbo@bayour.com Closes openzfs#3460, openzfs#5142

FransUrbo · 2016-10-09T13:41:28Z

I've been looking at the code for hours now, and I'm at a complete loss! I can't for the life of me remember what I was thinking/planning..

If someone have time/interest in taking a gander, I'd appreciate it.

loli10K · 2016-10-22T07:39:46Z

If someone have time/interest in taking a gander, I'd appreciate it.

I may have cracked this, but can i even push my code in this PR or do i have to open a new one?

rlaager · 2016-10-22T08:13:57Z

@loli10K Open a new one.

behlendorf · 2016-10-24T22:32:37Z

Superseded by #5324.

FransUrbo force-pushed the explicit_feature_disable branch 3 times, most recently from d4388b7 to a007aa3 Compare June 2, 2015 19:40

FransUrbo force-pushed the explicit_feature_disable branch from a007aa3 to 2d6ae3b Compare September 8, 2015 16:21

behlendorf requested changes Sep 17, 2016

View reviewed changes

FransUrbo force-pushed the explicit_feature_disable branch from 2d6ae3b to 943db61 Compare September 19, 2016 12:24

rincebrain mentioned this pull request Sep 21, 2016

zpool can not create a new pool with any features disabled. #5142

Closed

FransUrbo force-pushed the explicit_feature_disable branch from 943db61 to 2e83c4e Compare September 28, 2016 23:14

FransUrbo force-pushed the explicit_feature_disable branch from 2e83c4e to c6704a1 Compare September 29, 2016 14:16

behlendorf requested changes Sep 30, 2016

View reviewed changes

behlendorf added Type: Feature Feature request or new feature Status: Work in Progress Not yet ready for general review labels Oct 7, 2016

FransUrbo force-pushed the explicit_feature_disable branch from c6704a1 to 2c0f2ef Compare October 9, 2016 10:33

Allow for '-o feature@<feature>=disable' on the command line.

9c9119d

Sometimes it is desired to specifically disable a feature directly on the 'zpool create' command line. Signed-off-by: Turbo Fredriksson turbo@bayour.com Closes openzfs#3460, openzfs#5142

FransUrbo force-pushed the explicit_feature_disable branch from 2c0f2ef to 9c9119d Compare October 9, 2016 11:00

behlendorf mentioned this pull request Oct 17, 2016

Pool features cannot be disabled at time of creation #5294

Closed

loli10K mentioned this pull request Oct 22, 2016

Allow for '-o feature@<feature>=disabled' on the command line. #5324

Merged

behlendorf closed this Oct 24, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for '-o feature@<feature>=disable' on the command line. #3465

Allow for '-o feature@<feature>=disable' on the command line. #3465

FransUrbo commented Jun 2, 2015

FransUrbo commented Jun 2, 2015

behlendorf left a comment

FransUrbo commented Sep 19, 2016

behlendorf commented Sep 19, 2016

FransUrbo commented Sep 20, 2016

behlendorf commented Sep 20, 2016

FransUrbo commented Sep 20, 2016

behlendorf commented Sep 20, 2016

FransUrbo commented Sep 20, 2016

FransUrbo commented Sep 21, 2016

FransUrbo commented Sep 21, 2016 •

edited

behlendorf commented Sep 21, 2016

DeHackEd commented Sep 21, 2016

FransUrbo commented Sep 21, 2016

FransUrbo commented Sep 28, 2016

FransUrbo commented Sep 28, 2016

FransUrbo commented Sep 30, 2016

behlendorf left a comment

behlendorf Sep 30, 2016

behlendorf Sep 30, 2016

FransUrbo commented Oct 9, 2016

loli10K commented Oct 22, 2016

rlaager commented Oct 22, 2016

behlendorf commented Oct 24, 2016

		@@ -1,4 +1,4 @@
		'\" te
		n'\" te

Allow for '-o feature@<feature>=disable' on the command line. #3465

Allow for '-o feature@<feature>=disable' on the command line. #3465

Conversation

FransUrbo commented Jun 2, 2015

FransUrbo commented Jun 2, 2015

behlendorf left a comment

Choose a reason for hiding this comment

FransUrbo commented Sep 19, 2016

behlendorf commented Sep 19, 2016

FransUrbo commented Sep 20, 2016

behlendorf commented Sep 20, 2016

FransUrbo commented Sep 20, 2016

behlendorf commented Sep 20, 2016

FransUrbo commented Sep 20, 2016

FransUrbo commented Sep 21, 2016

FransUrbo commented Sep 21, 2016 • edited

behlendorf commented Sep 21, 2016

DeHackEd commented Sep 21, 2016

FransUrbo commented Sep 21, 2016

FransUrbo commented Sep 28, 2016

FransUrbo commented Sep 28, 2016

FransUrbo commented Sep 30, 2016

behlendorf left a comment

Choose a reason for hiding this comment

behlendorf Sep 30, 2016

Choose a reason for hiding this comment

behlendorf Sep 30, 2016

Choose a reason for hiding this comment

FransUrbo commented Oct 9, 2016

loli10K commented Oct 22, 2016

rlaager commented Oct 22, 2016

behlendorf commented Oct 24, 2016

FransUrbo commented Sep 21, 2016 •

edited