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

zpool state module needs support for disk vdev #34762

Closed
aphor opened this issue Jul 18, 2016 · 7 comments
Closed

zpool state module needs support for disk vdev #34762

aphor opened this issue Jul 18, 2016 · 7 comments
Labels
Execution-Module Feature new functionality including changes to functionality and code refactors, etc. fixed-pls-verify fix is linked, bug author to confirm fix State-Module
Milestone

Comments

@aphor
Copy link
Contributor

aphor commented Jul 18, 2016

salt.modules.zpool does not support explicit (the second simplest example) "disk" type zpool vdev

Since the salt.states.zpool requires a vdev in layout kwarg in state.sls files, it is impossible to use this state module to create a simple single disk vdev zpool.

This should be a valid state

newpool:
  zpool.present:
    - config:
        import: false
        force: true
    - properties:
        comment: salty storage pool
    - layout:
        disk-0:
          /dev/disk0
          /dev/disk1

but produces an ambiguous error, assuming that the "disk" vdev is supposed to be interpreted by the salt.modules.zpool.create() as a device node on the filesystem

             if not os.path.exists(vdev):
                 ret[zpool][vdev] = 'not present on filesystem'

salt.states.zpool also does a similar (broken) check (DRY?) to validate the state layout.

@rallytime
Copy link
Contributor

Thanks @aphor for the suggestion. Sounds good to me.

FYI @sjorge or @nmadhok - you guys might be interested in this feature request.

@rallytime rallytime added Feature new functionality including changes to functionality and code refactors, etc. Execution-Module State-Module labels Jul 18, 2016
@rallytime rallytime added this to the Approved milestone Jul 18, 2016
@sjorge
Copy link
Contributor

sjorge commented Jul 18, 2016

@aphor if I understand correctly, you're looking for the equivalent of zpool create tank /dev/disk0 /dev/disk1 using the state module?

I'm pretty sure I tested this with the execution module, I think that should work fine. Not sure I tested that with the state module. (Can't check for another week+, traveling with limited internet)

As for the error, It could be improved yeah. Although I couldn't find a better fitting one.
the lower level vdev can be a file, device node or a character device. Depending on the operating system they can live almost anywhere (with the exception of device nodes of course), perhaps a shorter 'not present' or 'could not locate' would be slightly less confusing.

Edit:
After having a look at the code, it should work fine with the state module also:

newpool:
  zpool.present:
    - config:
        import: false
        force: true
    - properties:
        comment: salty storage pool
    - layout:
        /dev/disk0
        /dev/disk1

@aphor
Copy link
Contributor Author

aphor commented Jul 19, 2016

@sjorge yes, and I just realized I had botched my example syntax.

BTW: I have a PR on the way.

@aphor
Copy link
Contributor Author

aphor commented Jul 19, 2016

I think this is more about consistency in the YAML requirements to use the zpool state module.

#34770 <-- not too hairy.

@sjorge
Copy link
Contributor

sjorge commented Jul 19, 2016

PR with a small docs update + support for the fake 'disk' vdev is going through my internal testing, will take a bit as I am on holiday on 3G internet.

Alternative PR that only makes small accommodations in the state module is here: #34791
Took a while to boot my slugish test vm to run some tests, but all seems well.

@sjorge
Copy link
Contributor

sjorge commented Jul 24, 2016

The fix seems to have made it into both 2016.3 and develop. @aphor is everything working 'as expected' now?

@rallytime rallytime added the fixed-pls-verify fix is linked, bug author to confirm fix label Jul 25, 2016
@aphor
Copy link
Contributor Author

aphor commented Jul 27, 2016

This is also working for me now. Thanks!

@aphor aphor closed this as completed Jul 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Execution-Module Feature new functionality including changes to functionality and code refactors, etc. fixed-pls-verify fix is linked, bug author to confirm fix State-Module
Projects
None yet
Development

No branches or pull requests

3 participants