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

Add FreeBSD support for ZFS datasets and pools #1501

Merged
merged 1 commit into from
Mar 1, 2017

Conversation

jbenden
Copy link
Contributor

@jbenden jbenden commented Feb 21, 2017

The following new resources have been added; however, they presently only support FreeBSD and similar.

  • zfs_dataset: tests if a named ZFS dataset is present and/or has certain properties.
  • zfs_pool: tests if a named ZFS pool is present and/or has certain properties.

The mount resource has been refactored to include support for FreeBSD too.

@jbenden jbenden force-pushed the jbenden/freebsd-zfs branch 2 times, most recently from 2cee66d to f353986 Compare February 21, 2017 17:32
Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

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

@jbenden Thanks for your contribution! I'm a huge fan of ZFS... used it a lot on OmniOS at my last job... so adding ZFS support fills me with glee. :)

I added a few comments, and we should add some unit tests to this with some mocked output for all the items changed/added (i.e. BSD mounts, ZFS pools, and ZFS datasets). I'd be happy to help you navigate that process if you need any assistance.

end

class LinuxMounts < MountsInfo
include MountParser
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably a good time to rename this to LinuxMountParser to avoid any confusion.

def initialize(inspec)
@inspec = inspec
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a default #parse_mount_options method here that raises an exception to make it clear for future users that the method is required?

"

def initialize(zfs_dataset)
return skip_resource 'The `zfs_dataset` resource is not supported on your OS yet.' if !inspec.os.bsd?
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to account for it in this PR, but I suspect this resource would work OK as-is on Solaris and any Linux that is using ZFS-on-Linux too, with the small exception of the actual ZFS binary paths may be different. This is worth some investigation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would love to see the Solaris support for this as well!

The following new resources have been added; however, they
presently only support FreeBSD and similar.

* `zfs_dataset`: tests if a named ZFS dataset is present
  and/or has certain properties.
* `zfs_pool`: tests if a named ZFS pool is present and/or
  has certain properties.

Additionally, the `mount` resource has been reworked to
include support for FreeBSD; while the existing class
was renamed to LinuxMountParser.

Unit-tests were added for all of the above.

Signed-off-by: Joseph Benden <joe@benden.us>
@jbenden
Copy link
Contributor Author

jbenden commented Feb 22, 2017

Hello Adam,

I have made all requested modifications; with the exception of supporting Solaris and Linux ZFS. If you have access to either of these operating systems (especially Solaris) I simply need the binary path locations. I am positive this would work with Solaris, but am not sure where the required binaries are... I did see that Linux places them in /usr/sbin, but do not know if the output is exactly the same...

@adamleff
Copy link
Contributor

@chris-rock I think we should accept this as-is and open an enhancement issue for the backlog to extend this to Solaris-variants and even perhaps Linux-on-ZFS users. But since the contributor doesn't have access to a Solaris-based machine, it seems reasonable to accept as-is. Thoughts?

@chris-rock
Copy link
Contributor

I agree with you @adamleff

@adamleff adamleff merged commit f4b1a35 into inspec:master Mar 1, 2017
@adamleff
Copy link
Contributor

adamleff commented Mar 1, 2017

@jbenden thanks again for this contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants