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

Run saltcheck tests in parallel #56097

Merged
merged 18 commits into from
Apr 23, 2020
Merged

Run saltcheck tests in parallel #56097

merged 18 commits into from
Apr 23, 2020

Conversation

mchugh19
Copy link
Contributor

@mchugh19 mchugh19 commented Feb 8, 2020

What does this PR do?

Adds multiprocessing capability to saltcheck. I'm not sure if this is worth the additional complexity or if this is the way the core-team would like multiprocessing supported. Thoughts?

Tests will be run in parallel by adding saltcheck_parallel: True in minion config.
Setting this value to an integer will set the maximum parallel processes.

For each state, tests are multiprocessed batched by
Optional value set in saltcheck_parallel
or
The minimum between the number of processors or the number of tests for the state

Previous Behavior

Given tests

test time:
  module_and_function: test.sleep
  args:
    - 3
  assertion: assertTrue

test time 2:
  module_and_function: test.sleep
  args:
    - 3
  assertion: assertTrue

test time 3:
  module_and_function: test.sleep
  args:
    - 3
  assertion: assertTrue

test time 4:
  module_and_function: test.sleep
  args:
    - 3
  assertion: assertTrue

test time 5:
  module_and_function: test.sleep
  args:
    - 3
  assertion: assertTrue

test time 6:
  module_and_function: test.sleep
  args:
    - 3
  assertion: assertTrue

and

from second:
  module_and_function: test.sleep
  args:
    - 3
  assertion: assertTrue

from second2:
  module_and_function: test.sleep
  args:
    - 3
  assertion: assertTrue

saltcheck processing runs sequentially

      TEST RESULTS:
          ----------
          Execution Time:
              28.9358
          Failed:
              0
          Missing Tests:
              0
          Passed:
              8
          Skipped:
              0

real    0m31.508s
user    0m6.565s
sys     0m0.794s

26 seconds of sleep completes in 31 seconds total

New Behavior

Same tests now finish faster

      TEST RESULTS:
          ----------
          Execution Time:
              59.6738
          Failed:
              0
          Missing Tests:
              0
          Passed:
              8
          Skipped:
              0

real    0m17.431s
user    0m36.822s
sys     0m1.494s

Same 26 seconds of sleep completes in 17 seconds (individual test duration time is no longer very accurate as it includes multiprocessing overhead and since many tests run in parallel, the sum into execution time is even more off (especially for a series of short tests))
Adjusting the sleeps to larger values totaling 59 seconds, completes in 30 seconds

Tests written?

No
Not sure how to write a test as the output is unchanged. Is it possible to write assertions against the debug log?

Commits signed with GPG?

No

@mchugh19 mchugh19 requested a review from a team as a code owner February 8, 2020 15:01
@ghost ghost requested a review from Akm0d February 8, 2020 15:01
@mchugh19
Copy link
Contributor Author

re-run pr-windows2016-py3

@mchugh19
Copy link
Contributor Author

re-run pr-pre-commit

@Ch3LL Ch3LL removed the request for review from a team April 15, 2020 14:22
Akm0d
Akm0d previously approved these changes Apr 15, 2020
cmcmarrow
cmcmarrow previously approved these changes Apr 15, 2020
Copy link
Contributor

@dwoz dwoz left a comment

Choose a reason for hiding this comment

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

@mchugh19 I'm not a fan of overloading the meaning of config values or allowing them to have multiple types. I'd prefer that saltcheck_parallel remain a boolean and use a second config value for the number of potential processes, that value can default to number of available processes.

@mchugh19
Copy link
Contributor Author

I'm not a fan of overloading the meaning of config values or allowing them to have multiple types. I'd prefer that saltcheck_parallel remain a boolean and use a second config value for the number of potential processes, that value can default to number of available processes.

Sure. That's easy enough. Maybe you could look over #56101 first, since either will need to be rebased after the other one is merged anyway.

@mchugh19 mchugh19 dismissed stale reviews from cmcmarrow and Akm0d via d22cabf April 23, 2020 05:55
@mchugh19
Copy link
Contributor Author

@dwoz as suggested, another saltcheck_processes parameter is now used to limit parallelization. Once tests pass, this should be good to go.

@dwoz dwoz merged commit 38ec644 into saltstack:master Apr 23, 2020
@sagetherage sagetherage added the ZRelease-Sodium retired label label May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ZRelease-Sodium retired label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants