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

Fixes performance and timeout issues on win_pkg.install #30249

Merged
merged 2 commits into from Jan 12, 2016

Conversation

Projects
None yet
3 participants
@mpreziuso
Contributor

mpreziuso commented Jan 8, 2016

Whenever there are no changes in the registry, win_pkg.install will check for any difference applied to it 1001 times before giving up, this causes an unnecessary CPU spike that may lead to performance degradation on the host or in any of its running applications, and if the host it's an AWS burstable instance this may cause unnecessary CPU credit consumption.

This commit changes that behaviour: the module will now check for any difference in the registry for a maximum of 10 times with an interval of 3 seconds before giving up.

See #30230.

Fixes performance and timeout issues on win_pkg.install
Whenever there are no changes in the registry, win_pkg.install will check for any difference applied to it 1001 times before giving up, this causes an unnecessary CPU spike that may lead to performance degradation on the host or in any of its running applications, and if the host it's an AWS burstable instance this may cause unnecessary CPU credit consumption. 
This commit changes that behaviour: the module will now check for any difference for a maximum of 10 times with an interval of 3 seconds before giving up.
See #30230.
@@ -695,11 +696,13 @@ def install(name=None, refresh=False, pkgs=None, saltenv='base', **kwargs):
new = list_pkgs()
tries = 0
difference = salt.utils.compare_dicts(old, new)
while not all(name in difference for name in changed) and tries <= 1000:
while not all(name in difference for name in changed) and tries < 10:

This comment has been minimized.

@jfindlay

jfindlay Jan 11, 2016

Contributor

I think this should be tries <= 10 otherwise the if tries == 10: line below will never evaluate to True.

@jfindlay

jfindlay Jan 11, 2016

Contributor

I think this should be tries <= 10 otherwise the if tries == 10: line below will never evaluate to True.

This comment has been minimized.

@mpreziuso

mpreziuso Jan 11, 2016

Contributor

Hmmm I think that should be ok:

  • tries = 9
  • 9 < 10; so evaluates to True and enters while statement
  • tries += 1 so tries = 10
  • 10 == 10 to evaluates to True and enters if statement
@mpreziuso

mpreziuso Jan 11, 2016

Contributor

Hmmm I think that should be ok:

  • tries = 9
  • 9 < 10; so evaluates to True and enters while statement
  • tries += 1 so tries = 10
  • 10 == 10 to evaluates to True and enters if statement

This comment has been minimized.

@jfindlay

jfindlay Jan 11, 2016

Contributor

You are right, sorry.

@jfindlay

jfindlay Jan 11, 2016

Contributor

You are right, sorry.

@jfindlay

This comment has been minimized.

Show comment
Hide comment
@jfindlay

jfindlay Jan 11, 2016

Contributor

@mpreziuso, thanks for fixing this. There are a few lint errors. Everything else should be unrelated.

Contributor

jfindlay commented Jan 11, 2016

@mpreziuso, thanks for fixing this. There are a few lint errors. Everything else should be unrelated.

@mpreziuso

This comment has been minimized.

Show comment
Hide comment
@mpreziuso

mpreziuso Jan 11, 2016

Contributor

Hi @jfindlay, sorry, didn't know about lint parameters. That should be ok now, thank you!

Contributor

mpreziuso commented Jan 11, 2016

Hi @jfindlay, sorry, didn't know about lint parameters. That should be ok now, thank you!

cachedout added a commit that referenced this pull request Jan 12, 2016

Merge pull request #30249 from mpreziuso/patch-2
Fixes performance and timeout issues on win_pkg.install

@cachedout cachedout merged commit 3bd02a8 into saltstack:2015.8 Jan 12, 2016

2 of 5 checks passed

default Merged build finished.
Details
jenkins/salt-pr-rs-cent7-n Salt PR - RS CentOS 7 #11207 — FAILURE
Details
jenkins/salt-pr-rs-ubuntu14.04-n Salt PR - RS Ubuntu 14 #8699 — FAILURE
Details
jenkins/salt-pr-clone Salt PR - Clone Repository #12618 — SUCCESS
Details
jenkins/salt-pr-lint-n Salt PR - Code Lint #12317 — SUCCESS
Details

@mpreziuso mpreziuso deleted the mpreziuso:patch-2 branch Jan 15, 2016

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