zpool state module needs support for disk vdev #34762

[aphor]

Support zpool "disk" vdevs with explicit syntax, conforming to the same convention for specifying other vdev types.

Previous Behavior

to make a mirror vdev zpool:

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

to make a simple disk vpool

newpool:
  zpool.present:
    - config:
        import: false
        force: true
    - properties:
        comment: quick and dirty
    - layout: /dev/disk0

to make a striped disk zpool

newpool:
  zpool.present:
    - config:
        import: false
        force: true
    - properties:
        comment: quicker and dirtier
    - layout: /dev/disk0 /dev/disk1

New Behavior

old behavior still works

to make a simple disk vpool

newpool:
  zpool.present:
    - config:
        import: false
        force: true
    - properties:
        comment: quick and dirty
    - layout:
        disk-0:
          /dev/disk0

to make a striped disk zpool

newpool:
  zpool.present:
    - config:
        import: false
        force: true
    - properties:
        comment: quicker and dirtier
    - layout:
        disk-0:
          /dev/disk0
          /dev/disk1

Tests written?

No, but these examples are a good start!

Documentation changes?

Let's see

[sjorge]

I'm okay in adding a disk-X to the state module.

But it should not be added to the execution module. I tried to stay as close as possible to the actually underlying cli command.

[aphor]

Thoughts:
If we want to stay close to the zpool cli, we should not even care about parsing (mirror, zmirror, etc.) vdev types out of the arguments, and let the user supply a "vdev spec" as a list of strings that zpool parses, referring to zpool man page for acceptable arguments. That way we reduce the technical debt of supporting future zpool vdev syntax. If we want to provide structure for users, then our priority is trying to adapt the underlying command to the conventions your users expect from our tool.

Maybe we should do the former for the execution module and the latter for the state module? I agree it would be better if this logic did not get implemented twice; there is some duplication in the state module and execution module, which is bad on DRY principles.

[sjorge]

The checks for vdev type in the execution module for is figuring out to see if we need to check the device is the correct type or not.

In the state module it's there to allow for few ways of formatting the layout property.

It could probably be swapped for a are.startswith('/') in the execution module. A clean up of the zpool and zfs modules to simplify some bits is near the end of my todo list. It usually involves a good day of testing for some small changes.

A unit test set is also on the todo for those, but I've no experience in writing those from scratch.

[aphor]

@sjorge thanks! I'll rework my changes to the state module.

[sjorge]

Should have been covered in the same 2 PR's. Targeted for 2016.3 branch so @CacheOut or someone will probably forward merge those to develop branch soon.

[sjorge]

Can't edit on mobile it seems, the vdev parsing is now completely gone in both the exec and state module. I did add a tiny bit of glue to allow disk-n in the state module.

[cachedout]

@aphor We do need a rebase on this since it has merge conflicts now. Thanks!

[cachedout]

Hi @aphor

Do you want to rebase this or open another PR? Please let me know. Thanks!

[sjorge]

I think this got fixed in geh cleanup and refactoring of the zpool module/state.

[cachedout]

@sjorge So should this PR be closed then?

[sjorge]

The state currently supports disk vdevs in the traditional way or using a helper 'disk-n' type. So I think it can be closed.

[cachedout]

Thanks, @sjorge.