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

(MODULES-10726) Add support for ashift/autoexpand/failmode to the zpool provider. #30

Merged

Conversation

KeithWard
Copy link
Contributor

This PR adds support for setting ashift (linux only), autoexpand, and failmode on zpools,

Both setting them individually, and also as part of creation
ashift (Only valid on linux)
autoexpand
failmode.

@KeithWard KeithWard requested a review from a team June 12, 2020 19:47
@KeithWard
Copy link
Contributor Author

As far as I can tell ashift appears to be valid on ZoL, but couldn't find any information about it being supported on Solaris? If it turns out ashift is supported there, then it's a trivial change to enable it for other OS.

@KeithWard KeithWard changed the title Add support for additional properties for Zpool. Add support for additional properties to zpool. Jun 12, 2020
@GabrielNagy
Copy link
Contributor

Hi @KeithWard,

Thanks for your contribution. I just ran the acceptance test suite on our Solaris platforms and we ran into some issues.
From the looks of it, Solaris 10 and 11 do not support flags for zpool get (neither -o or -H).

Here's some output on how zpool properties are shown on Solaris 10/11:

root@racy-saturation:~# zpool get -H -o autoexpand rpool
bad property list: invalid property '-H'
For more info, run: zpool help get
root@racy-saturation:~# zpool help get
usage:
        get <"all" | property[,...]> <pool> ...
root@racy-saturation:~# zpool get autoexpand rpool
NAME   PROPERTY    VALUE  SOURCE
rpool  autoexpand  off    default

However... Solaris 11.4 comes with a newer version of zfs which does support these flags:

root@smug-nationhood:~# zpool get -H -o value autoexpand rpool
off

To fix this, I'd suggest either executing zpool get <property> <pool>, and parsing the output to get the value of the property, or execute zpool get -H -o value <property> <pool> and fall back to the simpler syntax if the command fails.

Let me know if you need any help with this. Also, it'd be good to have a MODULES ticket opened to track the work on this PR.

@GabrielNagy
Copy link
Contributor

As far as I can tell ashift appears to be valid on ZoL, but couldn't find any information about it being supported on Solaris? If it turns out ashift is supported there, then it's a trivial change to enable it for other OS.

I just did a quick check and the ashift property does not seem to be supported on any of our supported Solaris versions.

@KeithWard
Copy link
Contributor Author

To fix this, I'd suggest either executing zpool get <property> <pool>, and parsing the output to get the value of the property, or execute zpool get -H -o value <property> <pool> and fall back to the simpler syntax if the command fails.

Let me know if you need any help with this. Also, it'd be good to have a MODULES ticket opened to track the work on this PR.

I'll probably do the former for this, and switch to parsing zpool get <property> <pool> as that's supported by all OS, as and when the EOL for Soalris 10 comes around we can look at switching it.

I'll update the PR with my changes and let you know when i'm done, Thanks.

@KeithWard
Copy link
Contributor Author

@GabrielNagy Apologies for the delay, I've updated the PR switching it to parsing the output from zpool get prop pool. Please try acceptance against this and let me know.

Thanks.

@KeithWard KeithWard changed the title Add support for additional properties to zpool. (MODULES-10726) Add support for ashift/autoexpand/failmode to the zpool provider. Jul 5, 2020
@KeithWard
Copy link
Contributor Author

I've raised an explicit ticket in JIRA to reference this PR MODULES-10726 , and updated the title to match.

lib/puppet/type/zpool.rb Show resolved Hide resolved
spec/unit/provider/zpool/zpool_spec.rb Outdated Show resolved Hide resolved
@KeithWard KeithWard force-pushed the add_additional_pool_options branch 2 times, most recently from 931f0b7 to 230b86e Compare July 6, 2020 08:52
@KeithWard
Copy link
Contributor Author

I've squashed squashed my commits down to one for ease of merging

@KeithWard KeithWard removed the request for review from a team July 6, 2020 08:55
@KeithWard
Copy link
Contributor Author

OK so aparently me re-requesting review removed the request for a review from puppetlabs/night-s-watch, not sure how to go about re-adding that or if i need to.

Copy link

@smortex smortex left a comment

Choose a reason for hiding this comment

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

My last suggestion was not wise, I'm sorry about this :-(

lib/puppet/type/zpool.rb Outdated Show resolved Hide resolved
@gimmyxd gimmyxd requested a review from a team July 7, 2020 07:24
Copy link
Contributor

@GabrielNagy GabrielNagy left a comment

Choose a reason for hiding this comment

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

This looks good now, and acceptance tests are passing! Thanks for also raising a ticket.

@GabrielNagy
Copy link
Contributor

@KeithWard one last thing, please squash your commits and prepend (MODULES-10726) to the commit title as well. Thanks!

@KeithWard KeithWard force-pushed the add_additional_pool_options branch 2 times, most recently from 33bcf5e to fd3f8f9 Compare July 7, 2020 17:15
ashift (Only valid on linux)
autoexpand
failmode.
@KeithWard KeithWard force-pushed the add_additional_pool_options branch from fd3f8f9 to 431e351 Compare July 7, 2020 17:16
@KeithWard
Copy link
Contributor Author

@GabrielNagy Thats all done for you. Thanks.

@GabrielNagy GabrielNagy merged commit bc987d0 into puppetlabs:master Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants