Skip to content

Lazily gather the lists of available sls data from the master #54468

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

Merged
merged 1 commit into from
Apr 22, 2020

Conversation

sathieu
Copy link
Contributor

@sathieu sathieu commented Sep 13, 2019

What does this PR do?

This improves significantly the performance of a state.apply when using
GitFS and a lot of branches.

What issues does this PR fix or reference?

none

Previous Behavior

3 minutes with 276 branches.

New Behavior

25 seconds

Tests written?

No. Already covered by existing tests.

Commits signed with GPG?

No

@sathieu sathieu requested a review from a team as a code owner September 13, 2019 08:15
@ghost ghost requested a review from twangboy September 13, 2019 08:15
@sathieu
Copy link
Contributor Author

sathieu commented Sep 13, 2019

Note : I have tested a similar patch for pillars (in salt/pillar/__init__.py:462 Pillar.__gather_avail()), but the performance improvement is not that significant. Probably because the loop in master-only, while the loop in BaseHighState is on the minion but talks to the master for each env.

@sathieu sathieu force-pushed the lazy_envs branch 2 times, most recently from 3a2d096 to 581a810 Compare September 18, 2019 09:57
@sathieu sathieu changed the title Lazyly gather the lists of available sls data from the master Lazily gather the lists of available sls data from the master Sep 18, 2019
@sathieu
Copy link
Contributor Author

sathieu commented Oct 15, 2019

@twangboy Could you please review?

@sathieu sathieu changed the base branch from develop to master October 15, 2019 12:50
@dwoz dwoz added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Dec 23, 2019
@sathieu
Copy link
Contributor Author

sathieu commented Dec 27, 2019

@dwoz I've added tests

@sathieu sathieu force-pushed the lazy_envs branch 2 times, most recently from b571779 to 332de26 Compare December 27, 2019 15:38
DmitryKuzmenko
DmitryKuzmenko previously approved these changes Apr 6, 2020
@DmitryKuzmenko DmitryKuzmenko self-assigned this Apr 6, 2020
@waynew
Copy link
Contributor

waynew commented Apr 6, 2020

@thatch45 can you take a look at this? It looks pretty straightforward to me but I'm not 100% familiar with the implications of futzing with the states.

thatch45
thatch45 previously approved these changes Apr 6, 2020
@thatch45
Copy link
Contributor

thatch45 commented Apr 6, 2020

I really like this approach, it is clean, and it can be pulled out if there are issues. But this does look like it should work.
I love simple, clean additions :)

@DmitryKuzmenko
Copy link
Contributor

@twangboy your turn!

@DmitryKuzmenko
Copy link
Contributor

@sathieu could you please resolve merge conflicts?

@dwoz dwoz removed the Merge Ready label Apr 11, 2020
@DmitryKuzmenko
Copy link
Contributor

@sathieu could you please resolve the merge conflicts and run the pre-commit script on your code to re-format it with isort and black we're using now.

pip install pre-commit
pre-commit install
FILES="$(git show --pretty="" --name-only master..HEAD)"
pre-commit run isort $FILES
pre-commit run black $FILES

next time you'll commit it will be ran automatically.

@sathieu
Copy link
Contributor Author

sathieu commented Apr 15, 2020

@DmitryKuzmenko I've rebased and used pre-commit hook. I had to use virtualenv -p /usr/bin/python3 DESTDIR, otherwise python2 was used.

Then, it failed with:

  Attempting uninstall: wrapt
    Found existing installation: wrapt 1.10.11
ERROR: Cannot uninstall 'wrapt'. It is a distutils installed project and thus we cannot accurately determine which files belong to it which would lead to only a partial uninstall.
Session lint-tests-pre-commit failed.

I've commited again (black and isort already runt) with --no-verify and pushed here.

This improves significantly the performance of a state.apply when using
GitFS and a lot of branches.
@DmitryKuzmenko
Copy link
Contributor

DmitryKuzmenko commented Apr 15, 2020

@sathieu thank you for report. We'll work on it.
I've just checked your code formatting is good now.

@waynew
Copy link
Contributor

waynew commented Apr 15, 2020

@sathieu did you use pip or python3 -m pip install --user pre-commit? It should be defaulting to python3 when installing the hooks.

If just installing pre-commit that way doesn't work running rm -rf ~/.cache/pre-commit/ may help.

@DmitryKuzmenko
Copy link
Contributor

@waynew ah, it looks it's my fault. I should recommend python3 pip next time.

@dwoz dwoz merged commit 0bb9e37 into saltstack:master Apr 22, 2020
@sagetherage sagetherage added the ZRelease-Sodium retired label label May 18, 2020
@sathieu sathieu deleted the lazy_envs branch May 30, 2020 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants