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

Fixes to check_requisite to account for parallelization #58976

Conversation

garethgreenaway
Copy link
Contributor

@garethgreenaway garethgreenaway commented Nov 18, 2020

What does this PR do?

When we are checking requisites, run reconcile_procs just on those requisite states not all running states.

What issues does this PR fix or reference?

Fixes: #49273

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@garethgreenaway garethgreenaway requested a review from a team as a code owner November 18, 2020 19:03
@garethgreenaway garethgreenaway requested review from s0undt3ch and removed request for a team November 18, 2020 19:03
@sagetherage sagetherage added the Aluminium Release Post Mg and Pre Si label Nov 18, 2020
s0undt3ch
s0undt3ch previously approved these changes Nov 19, 2020
Copy link
Collaborator

@s0undt3ch s0undt3ch left a comment

Choose a reason for hiding this comment

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

I would also prefer, and this is a personal preference, if we stopped piling up .sls files in out test suite state tree.

See https://github.com/saltstack/salt/blob/master/tests/integration/states/test_pkgrepo.py#L356

The reason behind this preference is because it's easy to stop using an sls file on a test and not delete the actual sls file, leaving it in the state tree forever, not being used.
Additionally, the bigger the state tree the less performat it is? ... This is almost a question.
Anyway, I tend to prefer that a test is self contained in it's test function so we always know, by reading the test function, and not having to look elsewhere, what's going on.

tests/integration/files/file/base/issue-49273.sls Outdated Show resolved Hide resolved
tests/integration/files/file/base/issue-49273.sls Outdated Show resolved Hide resolved
s0undt3ch
s0undt3ch previously approved these changes Nov 20, 2020
@s0undt3ch
Copy link
Collaborator

If we make it only sleep 2 or 3 seconds, it still tests the issue right? Even if they only sleep 1 second we can still test it right?

Just trying to make it not so slow...

@garethgreenaway
Copy link
Contributor Author

@s0undt3ch Yes. The issue is tested if the sleep is only for 2 or 3 seconds as opposed to 5.

Update sleep from 5 seconds to 2 seconds to bump up the run time.
@garethgreenaway
Copy link
Contributor Author

re-run full all

@max-arnold
Copy link
Contributor

So I tried to run the following state:

barrier:
  test.nop

{%- for x in [1,2,3] %}
blah-{{x}}:
  cmd.run:
    - name: sleep 10
    - require:
      - barrier
    - parallel: true
    - require_in:
      - barrier2
{% endfor %}

barrier2:
  test.nop

The parallelization works as expected, but the total run time is confusing, because it is still about ~30 seconds:

time salt-call state.apply aluminium.parallel --state-output terse
local:
  Name: barrier - Function: test.nop - Result: Clean Started: - 18:38:10.795775 Duration: 0.61 ms
  Name: sleep 10 - Function: cmd.run - Result: Changed Started: - 18:38:10.797259 Duration: 10019.814 ms
  Name: sleep 10 - Function: cmd.run - Result: Changed Started: - 18:38:10.801307 Duration: 10027.63 ms
  Name: sleep 10 - Function: cmd.run - Result: Changed Started: - 18:38:10.805268 Duration: 10024.311 ms
  Name: barrier2 - Function: test.nop - Result: Clean Started: - 18:38:20.865393 Duration: 3.41 ms

Summary for local
------------
Succeeded: 5 (changed=3)
Failed:    0
------------
Total states run:     5
Total run time:  30.076 s       <----

real	0m11.558s
user	0m0.978s
sys	0m0.313s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Aluminium Release Post Mg and Pre Si
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Salt parallelization is sequential if the parallel state object has any requisites
5 participants