Add support to ZFSConnection for ZFS properties, add property support to zsnap

[mdcurtis]

This pull request was born out of a desire to make rolling snapshots that are compatible with Samba vfs_shadow_copy2 (which requires consistent names for all snapshots). This is achieved in this PR by adding a property to the snapshot which allows the code to identify which rolling set the snapshot belongs to.

I use it like this:

/usr/local/bin/zsnap -k KEEP -p zsnap- -P com.sun:snaptype=TYPE DATASET

Where TYPE might be hourly, daily, weekly, etc, depending on the frequency that zsnap is executed from cron.

[Rudd-O]

The mode of some of the files changed to 0755. This needs to be corrected.

[Rudd-O]

Other than these observations, I think it looks good to merge. I'm hoping we don't have to circle back to review. Thanks for the contribution, looks very useful!

[mdcurtis]

PR updated to address your comments. I could not find a succinct way to describe the filtering behaviour in the help text for --property, so I have added a paragraph after the help options (epilog) that describes how the deletion candidates are filtered in both cases. I think this should make things clear for everyone.

[Rudd-O]

Thanks for updating the pull requests. The tests don't pass. Run nosetests on the directory and it will become obvious. I am trying to figure out why.

[Rudd-O]

I think I know how we can get the tests to pass easily.

When you set the _properties member to the __init__ of the ZFSConnection class, you can simply do

self._properties = ["creation"] + [ p for p in properties if p != "creation" ] if properties else ["creation"]

(This is the only place where the list of properties to retrieve is set, truthfully, so this is okay.)

Then, later on in the _get_poolset method, you no longer need to fabricate the properties variable -- simply refer to the connection's self._properties one. Don't pass the properties list to the parse_zfs_output method -- rather, make that method directly use the self._properties of the connection in order to do the parsing. In other words, the signature of the parse_zfs_output method returns to what it was (except perhaps the now-incorrect name creations parameter gets a better name).

This will (a) reduce the places where we muck with the properties list to retrieve (b) simplify the code (c) get the tests to pass.

Once we are there, I will merge this ASAP. I see how much better the code is with these enhancements.

[mdcurtis]

I'd realised (a few days after submitting) that my PR would break the tests, but hadn't had time to look at it.

I've had a quick look now, it seems that the tests have actually been broken since https://github.com/Rudd-O/zfs-tools/commit/5252eb972d4fbccf99b6b8aadc056c713cbc9471 where the signature of parse_zfs_r_outrput() changed from

   parse_zfs_r_output(self, output, creationtimes)

to

   parse_zfs_r_output(self, creationtimes)

In light of this, I would propose:

• removing the first of the two arguments to parse_zfs_r_output in the tests (the extra information is now used anyway)
• defaulting the properties list to [name,creation] in parse_zfs_r_output to minimise the verbosity in the tests

I'll work up a patch to this effect, perhaps as a separate PR and then rebase this one.

[Rudd-O]

Sorry for the absolutely dumb delay -- I was busy moving to a different nation (not kidding about that).

I'm evaluating this now.

[Rudd-O]

Oh god man, thank you very much for the work you've done in bringing these features into zfs-tools. This really does rock. I am so, so sorry that I didn't merge them earlier, and I'm happy to have them gotten merged now.

Please, keep contributing. Don't let my happenstance block you. And thanks again.