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

(FACT-1712) Improve zpool_version fact resolution #1597

Merged
merged 2 commits into from Jul 24, 2017

Conversation

Projects
None yet
5 participants
@smortex
Contributor

smortex commented Jul 10, 2017

When zpool upgrade -v does not output 'ZFS pool version XXX.',
fall-back to the last version reported.

This mimics the behavior of the Ruby version of Facter and makes the
fact reported when feature flags are enabled.

@smortex smortex force-pushed the smortex:fix-zpool_version branch from 08a4900 to e93af7b Jul 10, 2017

@puppetcla

This comment has been minimized.

puppetcla commented Jul 10, 2017

CLA signed by all contributors.

@Magisus

This does now seem to match the Ruby implementation.

@smortex

This comment has been minimized.

Contributor

smortex commented Jul 19, 2017

For information, here is the output of zpool upgrade -v on my system:

This system supports ZFS pool feature flags.

The following features are supported:

FEAT DESCRIPTION
-------------------------------------------------------------
async_destroy                         (read-only compatible)
     Destroy filesystems asynchronously.
empty_bpobj                           (read-only compatible)
     Snapshots use less space.
lz4_compress                         
     LZ4 compression algorithm support.
multi_vdev_crash_dump                
     Crash dumps to multiple vdev pools.
spacemap_histogram                    (read-only compatible)
     Spacemaps maintain space histograms.
enabled_txg                           (read-only compatible)
     Record txg at which a feature is enabled
hole_birth                           
     Retain hole birth txg for more precise zfs send
extensible_dataset                   
     Enhanced dataset functionality, used by other features.
embedded_data                        
     Blocks which compress very well use even less space.
bookmarks                             (read-only compatible)
     "zfs bookmark" command
filesystem_limits                     (read-only compatible)
     Filesystem and snapshot limits.
large_blocks                         
     Support for blocks larger than 128KB.
sha512                               
     SHA-512/256 hash algorithm.
skein                                
     Skein hash algorithm.

The following legacy versions are also supported:

VER  DESCRIPTION
---  --------------------------------------------------------
 1   Initial ZFS version
 2   Ditto blocks (replicated metadata)
 3   Hot spares and double parity RAID-Z
 4   zpool history
 5   Compression using the gzip algorithm
 6   bootfs pool property
 7   Separate intent log devices
 8   Delegated administration
 9   refquota and refreservation properties
 10  Cache devices
 11  Improved scrub performance
 12  Snapshot properties
 13  snapused property
 14  passthrough-x aclinherit
 15  user/group space accounting
 16  stmf property support
 17  Triple-parity RAID-Z
 18  Snapshot user holds
 19  Log device removal
 20  Compression using zle (zero-length encoding)
 21  Deduplication
 22  Received properties
 23  Slim ZIL
 24  System attributes
 25  Improved scrub stats
 26  Improved snapshot deletion performance
 27  Improved snapshot creation performance
 28  Multiple vdev replacements

For more information on a particular version, including supported releases,
see the ZFS Administration Guide.

I guess the main difference is that my system has features flags support, and maybe reporting them in facter would make sense (i.e. zpool_feature_flags => async_destroy,empty_bpobj,lz4_compress,multi_vdev_crash_dump,spacemap_histogram,enabled_txg,hole_birth,extensible_dataset,embedded_data,bookmarks,filesystem_limits,large_blocks,sha512,skein)?

According to zpool-features(7):

DESCRIPTION
     ZFS pool on-disk format versions are specified via "features" which
     replace the old on-disk format numbers (the last supported on-disk format
     number is 28).  To enable a feature on a pool use the upgrade subcommand
     of the zpool(8) command, or set the feature@feature_name property to
     enabled.

So, maybe it would make sense to change zpool_version => 28 to zpool_version => feature_flags in this case?

@Magisus

This comment has been minimized.

Contributor

Magisus commented Jul 20, 2017

In general we try not to change the types of fact values, except on major version boundaries. However, I also don't have a good understanding of how this fact might be used in the real world. @MikaelSmith or @branan, thoughts on the utility vs. surprising results of the above suggestion?

@MikaelSmith

This comment has been minimized.

Member

MikaelSmith commented Jul 20, 2017

We specify zfs_version is a string. Since the nature of version changed in ZFS, I'm ok if it becomes something besides a number. The less breaking way would be to give 28 if we're on a newer version, and have a new variable for the feature flags so people can opt-in to the changed behavior.

@Magisus

This comment has been minimized.

Contributor

Magisus commented Jul 21, 2017

@smortex so whatever information you think would be the most useful to the user, go ahead and update this to report that. Thanks for providing that perspective!

smortex added some commits Jul 24, 2017

(FACT-1712) Rename ZFS / zpool related variables
ZFS and zpool versions are called "features" internaly, which will lead
us to a lot of confusion with newer versions of zpool havind a new
concept of feature flags.

Rename all these variables to prevent such confusion, but do not change
the fact names (zfs_featurenumbers, zpool_featurenumbers) for now to
avoid breaking thigs.
(FACT-1712) Add new zpool_featureflags fact
Extend zpool facts with feature flags on supported systems.

If feature flags are enabled, make the zpool_version flag return "1000"
as per [2].

References:
  1. http://www.open-zfs.org/wiki/Feature_Flags
  2. http://web.archive.org/web/20160419064650/http://blog.delphix.com/csiden/files/2012/01/ZFS_Feature_Flags.pdf

@smortex smortex force-pushed the smortex:fix-zpool_version branch from e93af7b to b67d888 Jul 24, 2017

@smortex

This comment has been minimized.

Contributor

smortex commented Jul 24, 2017

Hi @Magisus!

Just found this presentation where feature flags are reported to correspond to legacy zfs version 1000, so I dropped my previous commit and with the pushed changes, zpool_version is "1000" when feature flags are supported (the presentation is linked from the feature flags page of Open-ZFS).

I took the liberty to rename a bunch of variables related to ZFS / zpool versions that where named features in a first commit to make the changes more easy to read. The second commit actually add the new zpool_featureflags fact and fix zpool_version, using a state machine for better readability.

Here is what is reported on my system:

zfs_featurenumbers => 1,2,3,4,5
zfs_version => 5
zpool_featureflags => async_destroy,empty_bpobj,lz4_compress,multi_vdev_crash_dump,spacemap_histogram,enabled_txg,hole_birth,extensible_dataset,embedded_data,bookmarks,filesystem_limits,large_blocks,sha512,skein
zpool_featurenumbers => 1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28
zpool_version => 1000
// Get the zpool version and features
static boost::regex zpool_version("ZFS pool version (\\d+)[.]");
static boost::regex zpool_feature("^\\s*(\\d+)[ ]");
static boost::regex zpool_version("^This system is currently running ZFS pool version (\\d+)\\.$");

This comment has been minimized.

@Magisus

Magisus Jul 24, 2017

Contributor

Are these messages affected by the user's locale, or are they always in English?

This comment has been minimized.

@smortex

smortex Jul 24, 2017

Contributor

On FreeBSD, they are currently not localized. BTW, it looks like leatherman sets LC_ALL=C in the command environment, so this should not be an issue.

This comment has been minimized.

@Magisus

Magisus Jul 24, 2017

Contributor

Perfect, thanks.

@Magisus

Thanks for researching this and helping to make this fact more meaningful!

@Magisus Magisus merged commit de6f054 into puppetlabs:master Jul 24, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.1%) to 86.317%
Details

@smortex smortex deleted the smortex:fix-zpool_version branch Jul 24, 2017

@rlaager

This comment has been minimized.

rlaager commented Jul 26, 2017

Feature flags is actually version 5000, not 1000.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment