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

Solaris port: zfs list without -p #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lhoward
Copy link

@lhoward lhoward commented Jan 22, 2019

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.

@someone1
Copy link
Owner

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?

@lhoward
Copy link
Author

lhoward commented Jan 24, 2019

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).

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

parse time in current location

Solaris zfs list only accurate to minute
@lhoward lhoward force-pushed the solaris-zfs-list branch 2 times, most recently from 697d052 to 42b4e24 Compare January 29, 2019 03:51
@lhoward
Copy link
Author

lhoward commented Jan 29, 2019

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.

@coveralls
Copy link

Coverage Status

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

@someone1
Copy link
Owner

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.

@lhoward
Copy link
Author

lhoward commented Jan 29, 2019

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
Copy link
Author

lhoward commented Jan 29, 2019

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.

@someone1
Copy link
Owner

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)

@gnattu
Copy link

gnattu commented Oct 4, 2020

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?

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.

4 participants