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

Zpool zol fixes #47224

Merged
merged 8 commits into from May 9, 2018

Conversation

Projects
None yet
4 participants
@sjorge
Contributor

sjorge commented Apr 21, 2018

What does this PR do?

Was going though some of the zfs/zpool tickets, looks liked I did not get tagged on #38671

This is also because the ZFS on Linux version on Ubuntu 16.04 is rather old.
We fix this by not using -o to request the fields we want, we simple requests all the fields.

Then manually strip 'name'.

What issues does this PR fix or reference?

#38671

Previous Behavior

The call to zpool used the newish -o flag, some minor docs update are also included.

New Behavior

We do not use -o

Tests written?

Tests update to include new command output.

Commits signed with GPG?

No

Backports

  • 2018.3

@sjorge sjorge force-pushed the sjorge:zpool_zol_fixes branch from a4fe3b9 to 675c646 Apr 21, 2018

@pruiz

This comment has been minimized.

Contributor

pruiz commented Apr 21, 2018

IMHO, this is a bad idea, as not limiting properties names makes zfs states/module quite slow on systems with lots of datasets (mor specifically snapshots), as I already stated at issue #47225.

A better option would be to only ask for specific property names if/when the installed zfs utilities allow for it (something which can be tested by pseudo-parsing "zfs/zpool -h" output).

@sjorge

This comment has been minimized.

Contributor

sjorge commented Apr 27, 2018

@pruiz we're still limiting property names!

## after change
[tachyon :: sjorge][/tmp]
[.]$ zpool get -H size tank
tank	size	112M	-
## before change
[tachyon :: sjorge][/tmp]
[.]$ zpool get -H -o property,value,source size tank
size	112M	-

The older ZFS does not allow limiting of the returned fields for the property we request. This should not introduce a noticable performance impact.

@sjorge

This comment has been minimized.

Contributor

sjorge commented Apr 28, 2018

@rallytime test failures seem not related to this change.

@pruiz

This comment has been minimized.

Contributor

pruiz commented Apr 29, 2018

@sjorge yeah, I understand older zfs versions do not allow for '-o' argument. However, on new zfs versions supporting it, not passing '-o' (and sometimes -s too) when querying/filtering datasets with lots of snapshots, the command takes minutes to complete.

So my request was: instead of removing the '-o' usage here. Try to guess wether the installed zfs command supported it or not (for example by parsing zfs help output with a regex like '^\slist .[-o .*'), so you can dynamically decide wether to pass '-o' or not.

@sjorge

This comment has been minimized.

Contributor

sjorge commented Apr 30, 2018

@pruiz Do you have any examples I have not seen any performance impact of this. I only had up to about a thousand datasets though.

time zpool get -H size tank
time zpool get -H -o property,value,source size tank

Difference would be nice data to have. (Now mind you dropping the 'size' one and fetching all properties does have an impact but that is not what this particular change does.

@pruiz

This comment has been minimized.

Contributor

pruiz commented May 1, 2018

Hi @sjorge,

My main concern is not with 'zpool', but with 'zfs' command, see this for example:

# zfs list -t all |wc -l
15058

# zfs list -t snapshot |wc -l
15051

# time zfs list -t all > /dev/null

real	1m22.034s
user	0m6.107s
sys	1m20.927s

# time zfs list -t all -o name > /dev/null

real	1m2.041s
user	0m9.097s
sys	0m51.944s

# time zfs list -t all -o name -s name > /dev/null

real	0m3.067s
user	0m0.040s
sys	0m2.630s

This machine is somewhat old (Xeon Server from 2012), with 4xSAS disks on raidz1. But still not uncommon, nor specially slow on anything else, but listing zfs datasets..

@sjorge

This comment has been minimized.

Contributor

sjorge commented May 1, 2018

Hi @pruiz

This PR does not touch the zfs list. I went over all the commands and zpool list was the only one that is currently broken on ZoL due to the use of -o.

Not as large as your pool, but here is a largish one. Same as you it is slowish when using zfs list.

[root@carbon ~]# time zfs list -t snap -r tank | wc -l
   50011

real	0m14.096s
user	0m4.348s
sys	0m9.667s
[root@carbon ~]# time zfs list -r tank | wc -l
    5002

real	0m1.210s
user	0m0.571s
sys	0m0.644s

For zpool get there does not a significant difference. It's also not possible to use recursion with get. So the total count of dataset/snapshot should not not make a difference.

[root@carbon ~]# time zpool get -H -o name,property,value,source size tank
tank	size	1.88G	-

real	0m0.016s
user	0m0.005s
sys	0m0.011s
[root@carbon ~]# time zpool get -H  size tank
tank	size	1.88G	-

real	0m0.018s
user	0m0.006s
sys	0m0.012s
@pruiz

This comment has been minimized.

Contributor

pruiz commented May 1, 2018

Then, my bad, I thought the PR was against zfs command, instead of zpool. I should have digged a little bit deeper.. ;)

@cachedout

This comment has been minimized.

Contributor

cachedout commented May 1, 2018

@pruiz Are you all right with this PR as it sits then?

@pruiz

This comment has been minimized.

Contributor

pruiz commented May 1, 2018

@cachedout yep, I myself have not seen any perf issues with zpool commands with vs without -o/-s (unlike zfs command).

@sjorge

This comment has been minimized.

Contributor

sjorge commented May 3, 2018

@cachedout bump

sjorge added some commits May 3, 2018

@sjorge

This comment has been minimized.

Contributor

sjorge commented May 7, 2018

@rallytime can you take a look at this? It should probably also get backported to 2018.3 eventually.
I actually started on this before the ZFS stuff got backported from develop.

@rallytime

This comment has been minimized.

Contributor

rallytime commented May 7, 2018

@sjorge Yeah, sure thing. Looks good to me and i've marked this for back-porting.

@rallytime rallytime merged commit de8e370 into saltstack:develop May 9, 2018

5 of 10 checks passed

jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #9651 — ABORTED
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #18763 — ABORTED
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #4708 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #22611 — FAILURE
Details
WIP ready for review
Details
codeclimate All good!
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #24883 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #17003 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #21623 — SUCCESS
Details

rallytime added a commit that referenced this pull request May 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment