zfsbackup-go icon indicating copy to clipboard operation
zfsbackup-go copied to clipboard

Solaris port: zfs list without -p

Open lhoward opened this issue 5 years ago • 9 comments

Solaris does not support the -p option to zfs list. Parse dates manually.

NB – I'm not a go programmer so this is probably not idiomatic.

lhoward avatar Jan 22 '19 05:01 lhoward

Thanks for the PR - I'll have to debug what happened with the tests here. Can you please give your system information and zfs version so I can better look at the proposed changes?

someone1 avatar Jan 24 '19 14:01 someone1

Solaris 11.3, with whatever version of ZFS comes with it (I'm running pool version 28, but that's not really relevant to command line options).

lhoward avatar Jan 24 '19 22:01 lhoward

This is a bit uglier than I imagined. The output of "zfs get -H -p -o value creation" is accurate to the second, whereas on Solaris (which lacks -p and returns a string) it is only accurate to the minute. For snapshots to be found during an incremental backup, I needed to truncate snapshot creation times to the second when comparing.

A better fix for Solaris would be to use GetZFSProperty(creation) on each snapshot returned by zfs list. Or I suppose I could compile up a new version of the ZFS binary from OpenSolaris, but I'd rather avoid that if possible.

lhoward avatar Jan 29 '19 03:01 lhoward

Coverage Status

Coverage increased (+0.09%) to 62.415% when pulling 42b4e2430c46d057e1ebdf3ee3f6c8f73b0588f6 on lhoward:solaris-zfs-list into 12847913f8f59a95a883053ec10bf8aa702289c9 on someone1:master.

coveralls avatar Jan 29 '19 04:01 coveralls

This introduces a breaking change - one that I'm not comfortable making. I think a better solution would be to break apart the helpers package into better named ones (e.g. a zfs package) and use Go's build tags/constraints to have different modes of operation depending on the target OS. That is, the code that exists today will remain when compiled for Linux/FreeBSD, but we can introduce a Solaris specific implementation for GetSnapshots that may operate as per your PR (or, as your comments suggest, we can possibly do some extra work to get more granularity of the creation property)

Either way, I appreciate the PR and will probably be using it to test as I spin up my own Solaris VM to implement a fix here.

someone1 avatar Jan 29 '19 04:01 someone1

That sounds like a good plan. I'm sure you can test on Linux/FreeBSD, as that will work of course without the -p option.

Can I leave it with you? I'm not a Go programmer so I'm sure it will be quicker for you to fix!

lhoward avatar Jan 29 '19 04:01 lhoward

To-the-minute granularity (on Solaris) is fine for me, I'm more concerned actually about the timezone issue vis-a-vis changing to summer time.

lhoward avatar Jan 29 '19 04:01 lhoward

I can take it from here - thanks for all the work you've put in thus far!

I'd like to try testing against Solaris as that's the target in question here. There could be other things that I notice while testing aside from just the dates here (e.g. ZoL has a guid per snapshot available but FreeBSD keeps this internal, hence why we're dealing with name and date matches currently)

someone1 avatar Jan 29 '19 04:01 someone1

I found that on some opensource Solaris based OS, zfs list supports -p, like the OmniOS.

usage:
        list [-Hp] [-r|-d max] [-o property[,...]] [-s property]...
            [-S property]... [-t type[,...]] [filesystem|volume|snapshot] ...

So maybe we should check the availability of -p instead of detecting OS?

gnattu avatar Oct 04 '20 22:10 gnattu