Add "compatibility" property for zpool feature sets [2021 version]

[colmbuckley]

Property to allow sets of features to be specified; for compatibility with specific versions / releases / external systems.
Influences the behavior of 'zpool upgrade' and 'zpool create'.
Initial man page changes included.

Brief synopsis:

zpool create -o compatibility=off|legacy|file[,file...] pool vdev...

compatibility = off : disable compatibility mode (enable all features)
compatibility = legacy : request that no features be enabled
compatibility = file[,file...] : read features from the specified files.
Only features present in all files will be enabled on the resulting pool. Filenames may be absolute, or relative to /etc/zfs/compatibility.d or /usr/share/zfs/compatibility.d (/etc checked first).

Only affects zpool create, zpool upgrade and zpool status.

ABI changes in libzfs:

• New function zpool_load_compat to load and parse compatibility sets.
• Add zpool_compat_status_t typedef for compatibility parse status.
• Add ZPOOL_PROP_COMPATIBILITY to the pool properties enum
• Add ZPOOL_STATUS_COMPATIBILITY_ERR to the pool status enum

An initial set of base compatibility sets are included in cmd/zpool/compatibility.d; the Makefile.am for cmd/zpool is modified to install these in $pkgdatadir/compatibility.d and to create symbolic links to a reasonable set of aliases.

Signed-off-by: Colm Buckley colm@tuatha.org

Motivation and Context

(See previous PR for discussion and further context. I needed to recreate the git branch from scratch; this branch is a clean patch against head as of 2021-02-08.)

With the advent of zpool features, it is sometimes desirable to specify 'sets' of features to be applied to a zpool - for example, "all features supported by the current version of grub" and to limit the features applied by (eg) zpool create or zpool upgrade to not inadvertently enable unwanted features (in the case of grub incompatibility, this might result in an unbootable system).

Ubuntu have addressed this by hard-coding the pool name bpool to be immune to zpool upgrade and suppressing the warning about disabled features in zpool status when this pool name is encountered. Obviously, this is a very limited way of solving the problem.

After a discussion with @rlaager on the Debian ZFS package maintainers mailing list, I became aware of the "default features" proposal discussed at the OpenZFS leadership team meeting of 2019-03-26. This PR is an attempt to implement the necessary functionality.

Description

An on-disk zpool property "compatibility" is added, with the enum value ZPOOL_PROP_COMPATIBILITY.
This property may be unset, take one of the special values off or legacy, or be set to a comma-separated list of "feature set" names.
Feature set names refer to files in the current filesystem, either absolute pathnames, or relative to /etc/zfs/compatibility.d or /usr/share/zfs/compatibility.d
If the referenced file exists, desired features are loaded from it; it is expected to contain the user-facing feature names, whitespace or comma-separated. Comments are allowed. If multiple set names are supplied, only features present in all sets are included in the final set.
If a valid feature set is read, zpool create modifies its behavior to only apply features present in the set. (the -d flag inhibits automatic feature creation as usual), and zpool upgrade modifies its behavior likewise. zpool status will not print warnings about disabled features unless they are present in the set. The intention is that, when a feature set is specified, only those features are considered to be "all features" for the zpool command.

Example:

# cat /etc/zfs/compatibility.d/grub2
# Only features which are supported by GRUB2
async_destroy
bookmarks
embedded_data
empty_bpobj
enabled_txg
extensible_dataset
filesystem_limits
hole_birth
large_blocks
lz4_compress
spacemap_histogram
# zpool create -o ashift=12 -o compatibility=grub2 -O mountpoint=/ -R /mnt bootpool /dev/disk/bootdisk

How Has This Been Tested?

This only affects the operation of the command-line zpool create, zpool upgrade and zpool status commands (with a smaller influence on zpool import). I created various pools with suitable combinations of the -d flag, the -o compatibility property, features files containing valid and invalid feature names, and verified that correct behavior resulted in all cases. Similarly, the behavior of zpool upgrade was observed to work as designed.

Tests in the zfs-test suite are work in progress.

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)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• 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.

[colmbuckley]

It's possible that the BSD build failure is caused by fcntl macros being exposed by __BSD_VISIBLE and that #undef __BSD_VISIBLE before #include <fcntl.h> would help, but I don't have enough FreeBSD experience to understand whether there might be unintended consequences.

[behlendorf]

It's possible that the BSD build failure is caused by fcntl macros being exposed by __BSD_VISIBLE

For the moment, don't worry about the FreeBSD HEAD build failure. PR #11458 has been opened to address it. We're just waiting on an additional needed follow up fix before merging it. Sorry about the inconvenience.

[colmbuckley]

What's the preferred workflow for handling the CheckABI checks? Should this PR directly include a new libzfs.abi, add expected changes to libzfs.suppr or something else?

[behlendorf]

What's the preferred workflow for handling the CheckABI checks?

We've only recently introduced the automatic ABI checks so we haven't really settled in to a usual workflow just yet. However, what we had in mind was that the author of the PR should:

1. Run make abistore to generate a new .abi file for the relevant libraries.
2. Include the updated .abi files in the PR.
3. Mention in the PR which interfaces have been changed for the benefit of reviewers.

Which aside from 3) it looks like you've already done, which is great. Then before we make a new release we can review all the ABI changes made to the master branch and bump the library versions numbers accordingly.

[colmbuckley]

What's the preferred workflow for handling the CheckABI checks?

We've only recently introduced the automatic ABI checks so we haven't really settled in to a usual workflow just yet. However, what we had in mind was that the author of the PR should:

1. Run make abistore to generate a new .abi file for the relevant libraries.
2. Include the updated .abi files in the PR.
3. Mention in the PR which interfaces have been changed for the benefit of reviewers.

Which aside from 3) it looks like you've already done, which is great.

Hah, embarrassingly I did take a stab at (3) already; but accidentally squashed the commit during a rebase. Will update the commit.

[colmbuckley]

I'll continue to rebase against upstream/master from time to time, but will only push to here if there's a conflict. When we come closer to a merge, I'll push a clean rebase.

[behlendorf]

This is looking good! A few additional comments.

Since there were only changes to the libzfs ABI there's no needed to update the other .abi files. Let's drop the others from the PR. For future reference, you can run make storeabi in each library directory to generate just that libraries .abi file.

As part of this PR I think we should to provide a default feature file for each of last major OpenZFS releases (openzfs-0.7, openzfs-0.8, openzfs-2.0, openzfs-2.1). This would provide an easy way for many people to silence the zpool upgrade notice by just setting the new features property. For example, if you originally created your pool on v0.8 and never enabled any additional features (which is pretty common), you'd just need to run:

zpool set features=openzfs-0.8 tank

For reference, here are the features enabled for each version. Adding files for freebsd and grub would be handy as well and straight forward given the features table.

For consistency I'd suggest we create a cmd/zpool/features.d directory in the source tree to add the files too. Then simply add and install them form the cmd/zpool/Makefile.am. We already do something very similar for the zpool.d directory. You'll also need to add the new files to the rpm packaging, see around line 473 in the rpm/generic/zfs.spec.in file.

Lastly we're also going to need to add a few functional test cases for this to the ZFS test suite in the tests directory. Here's what immediately comes to mind and where I'd suggest adding them. But feel free to add more.

• tests/zfs-tests/tests/functional/cli_root/zpool_create/ : Create a pool with a feature set and verify only those features are enabled
• tests/zfs-tests/tests/functional/cli_root/zpool_upgrade/ : A new upgrade test which verifies that only the requested features in the feature set are enabled when upgrading.
• tests/zfs-tests/tests/functional/cli_user/zpool_status : Verify it only warns when features which should be enabled aren't, and probably when additional unexpected features are enabled.

Sorry in advance it's all written in ksh but it's what was originally chosen way back when. But adding a new test case is easy and there are a lot of examples you can use as a template. Just add the new test file to the directory, and to that directory's Makefile.am, and lastly to the tests/runfiles/common file.

You can run a group out tests from the source tree with the zfs-tests.sh script. For example, to run all the zpool_create tests:

./scripts/zfs-tests.sh -T zpool_create

If you run in to any problems let me know, I'm happy to help point you in the right direction. Thanks again for working on this.

[ericloewe]

As part of this PR I think we should to provide a default feature file for each of last major OpenZFS releases (openzfs-0.7, openzfs-0.8, openzfs-2.0, openzfs-2.1). This would provide an easy way for many people to silence the zpool upgrade notice by just setting the new features property. For example, if you originally created your pool on v0.8 and never enabled any additional features (which is pretty common), you'd just need to run:

zpool set features=openzfs-0.8 tank

For reference, here are the features enabled for each version. Adding files for freebsd and grub would be handy as well and straight forward given the features table.

If this PR is going to include tables, most of the work figuring them out for popular distros and by year for the compat-20xx ones was done during the summit (see https://docs.google.com/spreadsheets/d/1MR0KGi1oGsvnHc63JoYoJlcJ5RczD5IRYi5y924nRig/edit#gid=1754896756 ).
The compat lists can be lifted almost directly from a CSV export, the distro lists would need to be processed by a script.

[colmbuckley]

As part of this PR I think we should to provide a default feature file for each of last major OpenZFS releases (openzfs-0.7, openzfs-0.8, openzfs-2.0, openzfs-2.1). [...] For consistency I'd suggest we create a cmd/zpool/features.d directory in the source tree to add the files too. Then simply add and install them form the cmd/zpool/Makefile.am. We already do something very similar for the zpool.d directory. You'll also need to add the new files to the rpm packaging, see around line 473 in the rpm/generic/zfs.spec.in file.

Good suggestion; I will get this done later this week.

Lastly we're also going to need to add a few functional test cases for this to the ZFS test suite in the tests directory. Here's what immediately comes to mind and where I'd suggest adding them [...]

Thank you; those seem like sensible tests to have. That might take a little longer, but should be able to get to it at the weekend.

If you run in to any problems let me know, I'm happy to help point you in the right direction. Thanks again for working on this.

My pleasure! It's been good exercise for me!

[colmbuckley]

If this PR is going to include tables, most of the work figuring them out for popular distros and by year for the compat-20xx ones was done during the summit (see https://docs.google.com/spreadsheets/d/1MR0KGi1oGsvnHc63JoYoJlcJ5RczD5IRYi5y924nRig/edit#gid=1754896756 ).
The compat lists can be lifted almost directly from a CSV export, the distro lists would need to be processed by a script.

Thanks Eric; I can work with that sheet.

[ahrens]

The overall design seems good, my only concern with the user interface so far is when compositing features. If you didn't read the manpage carefully and saw something like zpool create -o features=2019,freebsd-8, you might think it means “I want these features”, i.e. “the features from 2019 AND the features in freebsd 8". More features listed means I get more features, right?

But really it means (and should mean) “I want my pool to be compatible with 2019 AND freebsd 8”. So I wonder if a different name for the property like compatibility=2019,freebsd-8 would make the compositing more intuitive. compat could be the shortcut for compatibility.

Assuming we change the property name to compatibility, we should rethink the special values all and none. I'm not sure that compatibility=none has a good meaning, we might want to drop it. compatibility=all could mean that we have the features that are listed in every file in the special dir. But in practice that's equivalent to compatibility=2017 or whatever the earliest date is. Or compatibility=all could mean "all the current platforms", the same as compatibility=2021. We could sidestep this problem by having folks use -d to disable all features (maximum compatibility). We can handle something like compat=current by having current be a symlink to the current year (2021).

[ahrens]

Once we have some feature-files in the PR, we should probably talk about what they are here, and our philosophy around what the different feature sets (compatibility sets) are. Unless we want to do that in zpool-features.5?

[colmbuckley]

See the most recent commit for an attempt at an initial set of feature/compatiblity files (nb: they are not yet installed by the Makefiles). Broadly, I've tried to include a minimal set of "base" feature sets, with sensible aliases handled by symlinks (eg: freenas-11.3 -> freebsd-11.3, compat-2018 -> zol-0.6.5). This is based from the spreadsheet provided above by @ericloewe - Eric, do you have an opinion on what the explanatory text might look like?

[ericloewe]

I think it's good practice to mention that compatibility files exist, but leave the long-form details to zpool-features.5, where the details of how feature flags operate are already described.

I'm thinking we can keep most of this change, but move the example file off to zpool-features.5

[colmbuckley]

I agree that having a "Compatibility feature sets" subsection in zpool-features is the best way to handle this. I've started this but am stumbling on mdoc syntax. Will hammer some more on it later.

[colmbuckley]

PTAL; moved the bulk of the discussion from zpoolprops(8) to zpool-features(5) - and reformatted from mdoc to groffman. Updated zpool-create(8) and zpoolprops(8) to reference the new section.

[colmbuckley]

The overall design seems good, my only concern with the user interface so far is when compositing features. If you didn't read the manpage carefully and saw something like zpool create -o features=2019,freebsd-8, you might think it means “I want these features”, i.e. “the features from 2019 AND the features in freebsd 8". More features listed means I get more features, right?

But really it means (and should mean) “I want my pool to be compatible with 2019 AND freebsd 8”. So I wonder if a different name for the property like compatibility=2019,freebsd-8 would make the compositing more intuitive. compat could be the shortcut for compatibility.

Assuming we change the property name to compatibility, we should rethink the special values all and none. I'm not sure that compatibility=none has a good meaning, we might want to drop it. compatibility=all could mean that we have the features that are listed in every file in the special dir. But in practice that's equivalent to compatibility=2017 or whatever the earliest date is. Or compatibility=all could mean "all the current platforms", the same as compatibility=2021. We could sidestep this problem by having folks use -d to disable all features (maximum compatibility). We can handle something like compat=current by having current be a symlink to the current year (2021).

I like this suggestion; would anyone else like to weigh in? It seems to be a clearer name for what the feature actually does.

[colmbuckley]

Thinking about how configuration files tend to be managed on modern Linux distributions, I see a pattern of putting distribution-managed files under /usr/lib/package-name and installation-managed files (ie: local changes) under /etc/package-name, with the /etc version taking precedence.

Any thoughts on adopting a similar pattern here - ie: looking first for /etc/zfs/features.d/filename and then /usr/lib/zfs-linux/features.d/filename ?

Related; are these configuration root directories managed anywhere in our Makefile system? I wouldn't like to hardcode them.

[rlaager]

Thinking about how configuration files tend to be managed on modern Linux distributions, I see a pattern of putting distribution-managed files under /usr/lib/package-name and installation-managed files (ie: local changes) under /etc/package-name, with the /etc version taking precedence.

Any thoughts on adopting a similar pattern here - ie: looking first for /etc/zfs/features.d/filename and then /usr/lib/zfs-linux/features.d/filename ?

That seems good to me, except that it should be the same subdir in both places, so /usr/lib/zfs not /usr/lib/zfs-linux.

Related; are these configuration root directories managed anywhere in our Makefile system? I wouldn't like to hardcode them.

The standard autoconf names for /etc and /usr/lib are sysconfdir and libdir.

[colmbuckley]

Assuming we change the property name to compatibility, we should rethink the special values all and none. I'm not sure that compatibility=none has a good meaning, we might want to drop it. compatibility=all could mean that we have the features that are listed in every file in the special dir. But in practice that's equivalent to compatibility=2017 or whatever the earliest date is. Or compatibility=all could mean "all the current platforms", the same as compatibility=2021. We could sidestep this problem by having folks use -d to disable all features (maximum compatibility). We can handle something like compat=current by having current be a symlink to the current year (2021).

There's a subtle difference between -o features=none and -d though - the former means "don't enable any features, don't complain about them in zpool status, and don't enable them on zpool upgrade" while the latter has more of a "don't enable them now, but they might be added later" effect. I'd like to keep this functionality, but possibly with a better name than "none" - something like "nofeatures" maybe?

-o features=all is slightly different; I view it as a mechanism to say "I no longer care about compatibility on this pool; give me all the features" - ie: a pool might have been created with -o features=2018 but the need for legacy support has gone, so now we do zpool set features=all tank; zpool upgrade tank. Again, I think that this can be boiled down to the choice of a suitable name, like "allfeatures". I don't think we should be doing things like parsing all files in the features.d directory, that makes the feature parse logic much more complex for an unlikely edge case.

My suggestion - change -o features to -o compatibility and change the special values none and all to nofeatures and allfeatures respectively; WDYT?

[colmbuckley]

Thinking about how configuration files tend to be managed on modern Linux distributions, I see a pattern of putting distribution-managed files under /usr/lib/package-name and installation-managed files (ie: local changes) under /etc/package-name, with the /etc version taking precedence.
Any thoughts on adopting a similar pattern here - ie: looking first for /etc/zfs/features.d/filename and then /usr/lib/zfs-linux/features.d/filename ?

That seems good to me, except that it should be the same subdir in both places, so /usr/lib/zfs not /usr/lib/zfs-linux.

Related; are these configuration root directories managed anywhere in our Makefile system? I wouldn't like to hardcode them.

The standard autoconf names for /etc and /usr/lib are sysconfdir and libdir.

Hmm. On Debian, seems that we set zfsexecdir to /usr/lib/zfs-linux and the various shell scripts called by zed etc. live there. I'd propose using datadir/@PACKAGE@/features.d (or compatibility.d) for the predefined feature sets.

[behlendorf]

Any thoughts on adopting a similar pattern here - ie: looking first for /etc/zfs/features.d/filename and then /usr/lib/zfs-linux/features.d/filename ?

Having two locations makes sense to me, and I agree with @rlaager that we want the subdir name to the be same if possible. But I'm not sure libdir is really the correct location based on the standard FHS descriptions in the autoconf documentation. Either libexecdir or datadir seem more appropriate, though in practice it seems each distribution has their own standard conventions. Here's what the autconf docs have to say on the matter:

https://www.gnu.org/prep/standards/html_node/Directory-Variables.html

The easiest route would be to put them under libexecdir since this is already set in cmd/zpool/Makefile.am. See the zpoolexecdir variable. However, datadir probably is really the most correct location (at least by my reading of the docs).

[rlaager]

There's a subtle difference between -o features=none and -d though - the former means "don't enable any features, don't complain about them in zpool status, and don't enable them on zpool upgrade" while the latter has more of a "don't enable them now, but they might be added later" effect. I'd like to keep this functionality, but possibly with a better name than "none" - something like "nofeatures" maybe?

What's the use case for having a feature-flag enabled pool but features=none? I'm not sure I see a point in having no features. But it might be useful to use features=none (to avoid upgrade nagging) and then enable some specific feature(s). So in that case, I'm not sure compatibility=nofeatures sounds quite right, as there would be features enabled. I don't have a better suggestion. But I'm also not sure there will be real demand for this scenario.

-o features=all is slightly different; I view it as a mechanism to say "I no longer care about compatibility on this pool; give me all the features"

This would also be the default, right? Any thoughts on compatibility=off as the name?

[rlaager]

However, datadir probably is really the most correct location (at least by my reading of the docs).

I agree that datadir (e.g. /usr/share) is the most correct. systemd is a notable example that uses /usr/lib (or /lib), as does udev, even before it was part of the systemd project. FWIW, I think that's wrong, as the files are architecture-independent. (And I'm not anti-systemd generally.) There are various other things on my system doing /usr/share//.d, so there is plenty of precedent that way too.

[behlendorf]

I like compatibility=off to indicate the compatibility is not a concern for this pool. When new features are available zpool status recommends they upgrade. I'd think this would be the default.

As for compatibility=nofeatures I could see this being useful if (for some reason) compatibility is still needed with a non-OpenZFS system, for example a Solaris system. But it seems like an empty compatibility.d/nofeatures file would effectively serve the same purpose so perhaps it doesn't need to be a special case in the code?

[rlaager]

As for compatibility=nofeatures I could see this being useful if (for some reason) compatibility is still needed with a non-OpenZFS system, for example a Solaris system.

Solaris compatibility would also require version=28. I suppose there is an upgrade nag in that case, too? I’m not sure if we should try to support that with compatibility or not.

[colmbuckley]

As for compatibility=nofeatures I could see this being useful if (for some reason) compatibility is still needed with a non-OpenZFS system, for example a Solaris system.

Solaris compatibility would also require version=28. I suppose there is an upgrade nag in that case, too? I’m not sure if we should try to support that with compatibility or not.

There's virtually no code-complexity cost to having the "none" option (it's four lines of code); and I can see some value in having a simple option to inhibit upgrades and silence nagging from zpool status (which doesn't depend on an external file); if it's just a case of coming up with a name which is semantically copacetic with changing -o features to -o compatibility, I'm sure that' not beyond us...

Maybe -o compatibility=null to make it clear that it's a "strange" option. Documented along the lines of "don't enable any features on pool creation or upgrade."

[behlendorf]

if it's just a case of coming up with a name which is semantically copacetic with changing -o features to -o compatibility, I'm sure that' not beyond us...

Yes, it's just the name. I'm fine with the described behavior and your originally suggested name, -o compatibility=nofeatures. That has the advantage of being descriptive, but better suggestions are welcome.

@rlaager good point, it's probably best to not conflate this with pool version numbers.

[ahrens]

I agree we need to have a value of compatibility= that maintains the current behavior of enabling all features (minimum compatibility). I think that compatibility=off makes sense.

If we want a value of compatibility= that has "maximum" compatibility (within the OpenZFS on-disk features paradigm), I think that could be implemented with an empty feature-set-file. As for the name of this file (and hence the property value), I think that null and nofeatures don't really evoke the intended behavior. One option would be to kick this down the road - just don't provide this file for now. I don't think it would be that big of a loss of functionality, and if someone really wants it they can always create the file with whatever name makes sense to them. Another option would be to fit this into the proposed classes of feature sets. For example, maybe compatibility=2013, because I think that on Jan 1 2013, feature flags existed (in illumos and ZoL master branches, at least) but no releases had shipped with any supported features.

[behlendorf]

Ok, let me know how you get on in #11605 and whether you'd prefer me to pull those changes back in here or whether you'll merge from your repo instead.

It might take a little bit to iterate on those changes. The initial version of this PR I pushed was a little too ambitious, I've cut it down to just what we need for this change and so far it's looking much better. Since it's a standalone change I think once it passes the CI and the FreeBSD guys are okay with it we can merge it, then rebase this PR on top if to resolve that test failure. If you want to pull in to this PR sooner to verify that it does fix the test that'd fine with me.

As a slight aside, since all enforcement of which features can be enabled is happening at the kernel level I don't think it's absolutely critical that we do these checks for valid feature names in the zpool command. But I agree it's a nice to have since we can generate a much more descriptive error message by catching it there.

I don't think we should prohibit the enabling of features beyond the current compatibility set

Agreed!

[behlendorf]

@colmbuckley can you also git cherry-pick behlendorf/zfs@910f30a0 from my repo to resolve the zpool_upgrade_features_001_pos and zpool_status_features_001_pos failures. I see you just picked up the updated version of #11605.

[colmbuckley]

@colmbuckley can you also git cherry-pick behlendorf/zfs@910f30a from my repo to resolve the zpool_upgrade_features_001_pos and zpool_status_features_001_pos failures. I see you just picked up the updated version of #11605.

Done. Let's see how the tests go now.

[colmbuckley]

I don't think we should prohibit the enabling of features beyond the current compatibility set

Agreed!

Thinking about something like the following functionality:

• Check for features being enabled beyond the compatibility set on:
  • zpool create
  • zpool import
  • zpool status
  • zpool set compatibility
  • zpool set feature@xx=enabled
  • ... and maybe zpool upgrade

In all cases, print a warning but allow the user to proceed. I think this can be achieved in a relatively straightforward fashion without getting in to complicated propagation of errors up and down, and without any ABI changes apart from a new status enum.

Due to timing and my availability to work on it, I still think we should decouple this from the current PR.

[colmbuckley]

@colmbuckley can you also git cherry-pick behlendorf/zfs@910f30a from my repo to resolve the zpool_upgrade_features_001_pos and zpool_status_features_001_pos failures. I see you just picked up the updated version of #11605.

Done. Let's see how the tests go now.

The test failures don't seem relevant to the changes in this PR, with the possible exception of FreeBSD's zpool_import_features_003_pos failure; maybe the change in the feature list messed up zhack somehow? (Seems a bit unlikely.)

I'll revert the edonr changes here, assuming you're going to merge them upstream at some point, then I'll rebase. At that point, I think/hope we're good.

[colmbuckley]

Thinking about something like the following functionality:

• Check for features being enabled beyond the compatibility set on:

  • zpool create
  • zpool import
  • zpool status
  • zpool set compatibility
  • zpool set feature@xx=enabled

... I think that this one should maybe be an error as opposed to a warning, as it would be an irreversible change which is contrary to the intention of the compatibility setting. If the user really wants to enable a feature not in the compatibility set, they should set compatibility=off, enable the feature, then re-set compatibility and accept the resulting warning.

[colmbuckley]

The test failures don't seem relevant to the changes in this PR, with the possible exception of FreeBSD's zpool_import_features_003_pos failure; maybe the change in the feature list messed up zhack somehow? (Seems a bit unlikely.)

Failing tests from earlier:

• Builder FreeBSD head amd64 (TEST) Build #3528
• Builder FreeBSD stable/12 amd64 (TEST) Build #3949
• Builder Debian 10 x86_64 (TEST) Build #6716
• zloop / tests

[ericloewe]

Thinking about something like the following functionality:

• Check for features being enabled beyond the compatibility set on:

  • zpool create
  • zpool import
  • zpool status
  • zpool set compatibility
  • zpool set feature@xx=enabled

... I think that this one should maybe be an error as opposed to a warning, as it would be an irreversible change which is contrary to the intention of the compatibility setting. If the user really wants to enable a feature not in the compatibility set, they should set compatibility=off, enable the feature, then re-set compatibility and accept the resulting warning.

Agreed, the error can tell the user to combine compat files and achieve the desired effect without negating some of the administrative benefits of having the compat option.

[ericloewe]

We are packaging the feature files along with the zpool commands and libraries so we can keep them all in sync. If a user ends up providing their own custom file then it seems reasonable they are responsible for it only referencing known features in that software version. If it doesn't then I think we do want to generate exactly that error message.

I think it's entirely reasonable for distros (especially LTS ones) to not update ZFS with support for new feature flags, but still provide updated compat files. (e.g. Ubuntu 22.04 is going to see support until 2027+). It's not unreasonable to end up with files that have unsupported features, let's imagine that in compat-2023 adds support for feature@compress-zstd, which the system supports, and feature@bpr, which the system does not yet support.

There are two aspects to this:

1. If there's an error or a warning, it has to be user-friendly
2. It would be less-than-ideal to force the user to edit the compat files to be able to enable "everything that I have and which is on the latest compat file"

My suggestion is to either warn the user that feature@bpr is unknown and was not enabled or error out and allow the user to force the known features to be enabled (e.g. --ignore-unknown-features).

[colmbuckley]

I think it's entirely reasonable for distros (especially LTS ones) to not update ZFS with support for new feature flags, but still provide updated compat files. (e.g. Ubuntu 22.04 is going to see support until 2027+). It's not unreasonable to end up with files that have unsupported features, let's imagine that in compat-2023 adds support for feature@compress-zstd, which the system supports, and feature@bpr, which the system does not yet support.

Ok, I think the best compromise here would be to weaken the "badword" detection in zpool_load_compat() so that it basically results in warnings rather than errors, this postpones the "hard" feature checking until applied in the kernel (ie: trying to specify a feature in a compat file which is not actually supported by the current system should still result in an error).

There are two aspects to this:

1. If there's an error or a warning, it has to be user-friendly

Something like Warning: feature "fantasticfeature" in compatibility file "zol-0.6.5" is not recognized by this system and will not be enabled. The only issue here will be that I won't be able to distinguish between "features not supported by this system" and typos/syntax errors, the latter will get the same error message.

2. It would be less-than-ideal to force the user to edit the compat files to be able to enable "everything that I have and which is on the latest compat file"

Fair point.

My suggestion is to either warn the user that feature@bpr is unknown and was not enabled or error out and allow the user to force the known features to be enabled (e.g. --ignore-unknown-features).

The problem with a flag is that this logic would invoked in several different places; zpool create, zpool set compatibility, zpool upgrade etc. I'd prefer to have a single flow which can be consistent across them all.

Do you want this PR to be held until this is resolved, or are you ok to proceed and fix afterwards? I think it won't be an issue until we get new features post-2.1.

[behlendorf]

Failing tests from earlier:

I double checked all the failures and they're ones we've seen sporadically before and are unrelated

From my perspective this is ready to be merged. As for relaxing the badword detection, I think converting those errors in to warnings sounds like a reasonable solution to me. I agree that adding a flag might get a bit awkward in the existing code. I don't have any objection to making the change to a warning after this is merged.

[colmbuckley]

Failing tests from earlier:

I double checked all the failures and they're ones we've seen sporadically before and are unrelated

From my perspective this is ready to be merged.

I'll rebase on master now and clean up the commit message.

As for relaxing the badword detection, I think converting those errors in to warnings sounds like a reasonable solution to me. I agree that adding a flag might get a bit awkward in the existing code. I don't have any objection to making the change to a warning after this is merged.

I'll work on this and the enhancement you suggested about warnings/errors when features are enabled outside the current compatibility set, when I next get a block of time (most likely in a few weeks). In the event that new features are added between now and then, I hope we can be sensibly reactive...

[colmbuckley]

Dear everyone who's reading this from the 2.1.0 announcement mail - please see also #11861 which improves the behavior of this property in some (hopefully) useful ways, resolving the open questions from above.

Enjoy!

[behlendorf]

Thanks for pointing that out! I went ahead and added a link to #11861 in the GitHub release notes.