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

Speed up zfs.filesystem_present #59970

Merged
merged 6 commits into from
Feb 4, 2022

Conversation

asomers
Copy link
Contributor

@asomers asomers commented Apr 6, 2021

Only look up the requested properties in ZFS, instead of all properties.
This affects zfs.volume_present, too.

Sponsored by: Axcient

What does this PR do?

Speeds up the zfs.filesystem_present and zfs.volume_present states.

Previous Behavior

zfs.filesystem_present would run zfs get -Hp -o name,value,source all <dataset> to look up all properties of the dataset. That's slow, because some of those properties can be overridden at mount time, and libzfs searches through all mounted file systems to find them.

New Behavior

A state like this:

mypool/mydataset:
  zfs.filesystem_present:
    - properties:
      compression: lz4

Will run zfs get -Hp -o name,value,source compression <dataset>, which is much faster.

Merge requirements satisfied?

Commits signed with GPG?

Yes

@asomers asomers requested a review from a team as a code owner April 6, 2021 02:38
@asomers asomers requested review from xeacott and removed request for a team April 6, 2021 02:38
alek-p
alek-p previously approved these changes Apr 12, 2021
@asomers
Copy link
Contributor Author

asomers commented Apr 21, 2021

Hm. The macosx and freebsd test failures are unrelated, but the debian9 failure is my fault. I'll try to fix that.

@asomers asomers force-pushed the filesystem_present branch 2 times, most recently from 2a136fe to cd8f5bd Compare April 21, 2021 18:22
@asomers
Copy link
Contributor Author

asomers commented Apr 21, 2021

I just rebased onto master, which I hope will fix the libgit2-related test failures on freebsd.

@asomers asomers force-pushed the filesystem_present branch 2 times, most recently from 144294a to 0166848 Compare April 21, 2021 22:24
@asomers
Copy link
Contributor Author

asomers commented Apr 22, 2021

Ok, I don't think any of the remaining test failures are related to the PR. @xeacott if you approve the changes, I'll squash my commits.

@xeacott
Copy link
Contributor

xeacott commented Apr 28, 2021

No worries on squashing the commits, keeping all the history is fine but I think I'm good with this

@xeacott
Copy link
Contributor

xeacott commented Apr 28, 2021

@krionbsd review once more?

xeacott
xeacott previously approved these changes Apr 28, 2021
krionbsd
krionbsd previously approved these changes Apr 29, 2021
@waynew
Copy link
Contributor

waynew commented Nov 22, 2021

@asomers looks like there's a conflict, do you think you can rebase this?

asomers and others added 5 commits December 7, 2021 14:24
Only look up the requested properties in ZFS, instead of all properties.
This affects zfs.volume_present, too.

Sponsored by: Axcient
Remove an assumption about the order of python's hash function.
* a "filesystem.present" or "volume.present" state that specified no
  properties would fail because it would execute "zfs get" with no property
  argument.  Fix this (and improve performance too) by skipping the "zfs
  get" if the user does not request any property values.

* A "volume.present" state that specified volume_size but no properties
  would fail to check the volsize.

Also, make the properties argument of _dataset_properties a mandatory
argument rather than a keyword argument.  All callers already specify this
argument, making the default value a red herring.
@asomers-ax
Copy link

rebased, @waynew .

@asomers-ax
Copy link

The test failures look unrelated to this PR.

@waynew
Copy link
Contributor

waynew commented Jan 14, 2022

Thanks! Let's see if the failures are just flaky tests

Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

Looks good - I think there are some improvements that could be made here but given this is blocking a related PR, and everything is passing, I think we should go ahead and merge this and the changes can be made in the other PR

@garethgreenaway garethgreenaway merged commit 389982f into saltstack:master Feb 4, 2022
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.

7 participants