Add "features" property for zpool feature sets. #10980
Conversation
|
@colmbuckley Thanks for getting started on this! I know @jpaetzel is hoping to work on this at the OpenZFS Developer Summit hackathon on October 7. Will you be able to coordinate with him then? |
I don't know to be honest what my schedule looks like that week, but I'd of course be perfectly happy to chat with him before then to see whether this PR can be the basis for a final version. I am happy with the functionality at the moment, although my unfamiliarity with the code base and data structures might have led to some inelegance in how things can best be achieved. I of course encourage him and anyone else to give the function a test drive; it wasn't as difficult in the end as I'd feared. |
Oh; I'm sorry I hadn't seen the proposed agenda; I certainly didn't intend to step on Josh's toes by charging ahead with a full implementation before discussion. If this implementation is useful as the basis of work at the hackathon, I'd be delighted (it especially needs a good test suite), or if it's thought to be a dead-end, that's also fine. I think the main innovation in my implementation is the concept of storing each feature set in a separate file; allowing for greater flexibility in comments and explanatory text, and also allowing packaged software like grub to supply and maintain its own compatible feature lists without treading on the toes of other packages. |
|
Quick sample use: Another using Adding a feature by adding it to the file: |
|
It's not clear to me why zimport.sh is failing in some of the tests. Will see if I can replicate locally next week. |
... found a strcmp before a NULL check. Hopefully that's the issue; will take another look on Monday. |
|
The test failures don't seem related to any of the code in this PR; can anyone confirm that? @behlendorf One thing I do need help with; I couldn't fully grasp the relationship between zpool properties as added to the |
Codecov Report
@@ Coverage Diff @@
## master #10980 +/- ##
==========================================
- Coverage 79.56% 79.54% -0.03%
==========================================
Files 398 398
Lines 125754 125754
==========================================
- Hits 100060 100025 -35
- Misses 25694 25729 +35
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Thanks for coding this up! I think we'll want to also add some feature-set files and an explanation of them. Here's a link to my proposal from December 2018. That said, this is kind of orthogonal to the implementation, so we can keep those discussions separate if you like. Perhaps we should open another PR to add the feature-set files and documentation related to the rules around them. |
That's up to the release maintainers. I suspect we don't want to hold up 2.0.0 for this feature, but I'd hope that it'd be able to merge fairly soon. There's a few things I'm still slightly unsure about, and I'd like to see better tests etc. but hopefully it's not in terrible shape. |
Yes, I'd like to see a nice suite of feature sets supplied with the standard distribution; and this can proceed, I think, largely independently of the code support. Your proposal does suggest modifying the default behavior of |
|
@colmbuckley I'm not sure we would want to change the default immediately, but when we do change it, I'd propose |
|
I'd like to hear from @jpaetzel regarding whether this is in any way in line with his thoughts in the lead-up to the Developer Summit. I regret that I wasn't aware of his existing effort before coding this up, but if it's useful to the broader picture, it's given with a glad heart. Josh? |
|
I'm getting slammed at work, I'll take a look at all this over the weekend. |
behlendorf
added
Status: Work in Progress
|
One thing to consider here is that "features supported by grub" is itself not going to be static over time. It would be better, IMHO, to make this feature list for a specific version of grub from the beginning (e.g. "grub-2.0"), so that as newer versions are added (presumably with more features) there doesn't need to be an "either/or" situation in the single "grub" feature list. |
|
Would it be possible to allow something like "-o feature=freenas11.2+ubuntu18.04" and it determines commonality for you (as long as there was a profile for each in the directory)? |
I agree. Minimally, it should be called "grub2" and if grub ever adds more feature flags (its been May 2015 since last addition) we can add another profile. |
Agreed.
I disagree, at least to some extent. To me, having the generic "grub" (or to be more correct, you can substitute "grub2") is an important feature. The idea is that "grub" means "whatever GRUB supports on my system". This is accomplished by having the feature definition of "grub" be a separate file which is installed by the GRUB package (initially e.g. the grub2 distro package on Ubuntu, but ideally GRUB upstream). That way, it is updated independent of the OpenZFS package (and release cycle) and always matches GRUB. Then if I upgrade my system from GRUB 2.0x to 2.0y and the feature list changes, One might argue that we should have both grub-2 and grub-2.0x. I see a few problems with that, though. First off, unless we introduce some alias mechanism (which might be a good idea anyway), this would lead to a lot of duplication of feature definitions (and files; see below). But setting that aside, what is the use case? Even if you are dual booting, you're still only going to have one copy of GRUB, which more-or-less has to be managed by one boot environment. Also, right now, it sounds like the feature definitions are single files. I wonder if we should support multiple feature definitions per file. |
|
Just going through major versions of relevant platforms/distributions there are easily over 30 lists of features, if they were all included. I think that declaring multiple strings for the same list (e.g. FreeNAS 11.0, FreeNAS 11.1, FreeBSD 11.0 and FreeBSD 11.1 support the same feature flags) might be useful to cut down on the spam a bit. |
|
@rlaager This is intended to ensure compatibility with other systems following the creation of pools. To do this, one would target certain OS/distro/zfs release or a compatibility year. For this to be successful there has to be an extensive profile library on the system that is creating the pool. Grub is the only "application", beyond ZFS itself, I can recall that cares about feature flags. Also, there may be people would want to create a pool while on a non-grub system, yet would want to limit flags so it could be compatible with grub in the future. Since ZFS upstream would need to keep track of all the other cases, a grub2 profile wouldn't be difficult to also keep (it hasn't changed in 5 years). While I can see the advantage of upgrading to an improved version of grub and having its install package automatically change the available grub feature flags and allowing a zpool upgrade, I don't see this being useful in any other scenarios. Automatic actions when past explicit protective actions were taken could lead to problems. Even with a distro upgrade, the running operating system cannot ascertain why the current restriction is in place. Was it because of a multi-boot situation? Was it to allow mounting in a different OS? Having the user explicitly do a "zpool set features=" seems to be little burden and allows the user to confirm that lesser restrictions are appropriate. |
GRUB's feature set is very limited (and as you noted, not changing much), so modern practice is to use a separate |
Yes, this is pretty much what I had in mind also; a situation whereby some feature files (eg: "portable") are maintained and distributed by core OpenZFS, and some (eg: "grub2") which are maintained by those packages (or by their per-distro packaging teams). Note of course that individual installations are able (trivially) to edit or add their own feature files, or simply to add or remove specific features to their zpools as needed; feature files are largely a convenience feature.
I chose the current implementation largely for simplicity; it keeps the parsing logic extremely straightforward, allows for very rapid sanity-checking and error detection, while satisfying all the use cases I could easily think of. I can imagine several potential extensions, such as allowing #include-like functionality, or a more complex file format based on a "setX = foo, bar, baz" syntax, but this comes with a substantial step-increase in complexity of the parsing function, and associated faff around accurate reporting of errors etc. What would be gained, do you think, by having multiple feature definitions per file? |
Yes, this would certainly be possible; again at the slight expense of code complexity. The actual parsing is easy, the main difficulty is in coherent error reporting. I'd be slightly concerned about the distinction between AND and OR combinations; and the possibility of getting this wrong either in the code, or as an admin. |
|
|
Aside from the great discussion about the UI; has anyone got any comments on the code? In particular, I have to say I still don't really know whether this property needs to be added as a ZPOOL_CONFIG #define and the spa data structure or logic; the narrative there eludes me. I also suspect I'm missing something as the property always shows as "default" instead of "local" in |
Would OR ever be useful? Realistically, users might say "I want to run on A, B and C" and it feels at least somewhat weird to me to say "I want to run on something else but don't particularly care if it's B or C". Supporting just AND simplifies things a bit. Another concern I have is that relying on these files being present on all systems might confuse users. If I'm reading this right, the current behavior is to error out of zpool upgrade if the file can't be read. Is the intent then that the user set the property to a file which does exist on the new system (or clear it entirely) in order to proceed? |
Agreed, I don't see a sensible use for
These files only need to exist on the system where the pool create or upgrade is done. |
That seems reasonable to me. If the file does not exist, the features mask cannot be enforced. At that point, zpool upgrade can either do the upgrade or refuse. If it does the upgrade, that could easily violate the features mask that the user asked for. A corollary is that feature definitions are not expected to go away. So we should be somewhat judicious in the features groups we create. |
|
Added the relevant bits to the spa_t structure, with suitable copying / freeing and validation logic (largely by copying the code and workflow for spa_comment). Note that it's not possible in |
Agreed, ok. Working on this feature; it'll be something like |
|
Ok; I think this is where it needs to be for review. Note to reviewers: the bulk of the additional logic is the Let me know what you think. |
Much code spelunking later, I think I grok this, and all looks good now. |
Signed-off-by: Colm Buckley <colm@tuatha.org>
|
(A little confused by the test failures; it seems that the new tests are being run with the old code?) |
|
Apologies to anyone who may have been watching this in anticipation; I hit some personal roadblocks preventing work through much of Q4/2020. I will be able to resume work on this shortly, although it might take some time to rebase cleanly against current head. |
|
New version of this PR is #11468 |
Motivation and Context
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 createorzpool upgradeto 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
bpoolto be immune tozpool upgradeand suppressing the warning about disabled features inzpool statuswhen 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 "features" is added, with the enum value
ZPOOL_PROP_FEATURES.This property may be unset, take one of the special values
allornone, or be set to a "feature set" name.Feature sets refer to files in the current filesystem, either absolute pathnames, or relative to
/etc/zfs/features.d(as currently implemented).If the referenced file exists, the desired features are loaded from it; it is expected to contain the user-facing feature names, whitespace or comma-separated. Comments are allowed.
If a valid feature set is read,
zpool createmodifies its behavior to only apply features present in the set. (the-dflag inhibits automatic feature creation as usual), andzpool upgrademodifies its behavior likewise.zpool statuswill 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 thezpoolcommand.Example:
Key Implementation Details
In
zpool_main.c, watch out for thefeaturesproperty and populate a boolean array of desired features when it is seen. When this occurs, modify the behavior of thezpool_do_create()function. Inupgrade_enable_all(), explicitly look up this property and only apply features which are present in the set.In
libzfs_pool.cadd the functionzpool_load_features()which, given a feature property value, locates and reads the specified file, matching keywords against feature unames, and populating a boolean array with TRUE/FALSE accordingly. This function is called inzpool_valid_proplist()to validate that the property file is present and correct.In
libzfs_status.c, thecheck_status()function is modified to likewise read the features property, load the relevant feature sets, and only flag missing features if they are present in the set. Note that I needed to pass the features property explicitly in to this function, as I could find no way to retrieve it using just theconfignvlist which was passed in. Someone more familiar with the data structures might be able to suggest a cleaner approach here.Things I'm unsure about...
The
featuresproperty is always shown asdefaultsource inzpool getfor pools where it is set; I couldn't determine exactly how this happens (vs.localwhich is what I'd expect).I have not had time to understand the test infrastructure for this project, so there are no unit tests for this specific functionality.
The interaction between the
featuresproperty and the-dflag inzpool createis not quite as was previously discussed; after a little back and forth, I feel that my implementation is most consistent.How Has This Been Tested?
This only affects the operation of the command-line
zpool create,zpool upgradeandzpool statuscommands (with a smaller influence onzpool import). I created various pools with suitable combinations of the-dflag, the-o featuresproperty, features files containing valid and invalid feature names, and verified that correct behavior resulted in all cases. Similarly, the behavior ofzpool upgradewas observed to work as designed.Types of changes
Checklist:
Signed-off-by.