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

Properly wait on returns in saltnado #48193

Merged
merged 2 commits into from
Jun 22, 2018
Merged

Conversation

jacksontj
Copy link
Contributor

This was broken because the behavior was to simply check the ckminions
and wait for only those returns to complete. This works assuming
ckminions is accurate (which there are many cases where it isn't, such
as syndics).

_disbatch_local's waiting on returns needs to match LocalClient's
behavior (namely that in get_iter_returns). This means we are allowed to
return when (1) we have waitged the min_wait_time (0 if not a syndic)
(2) no minions are running the job (3) all minions we saw running it are
done running the job. The only method allowed for earlier termination is
if the gather_job_timeout is exceeded.

Fixes #42659

What does this PR do?

What issues does this PR fix or reference?

Previous Behavior

Remove this section if not relevant

New Behavior

Remove this section if not relevant

Tests written?

Yes/No

Commits signed with GPG?

Yes/No

Please review Salt's Contributing Guide for best practices.

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

@ghost ghost self-requested a review June 18, 2018 20:15
@rallytime rallytime requested a review from a team June 18, 2018 21:17
@gtmanfred
Copy link
Contributor

Thanks for the help with this issues @jacksontj

@gtmanfred gtmanfred added the bugfix-bckport will be be back-ported to an older release branch by creating a PR against that branch label Jun 18, 2018
# Map of minion_id -> returned for all minions we think we need to wait on
minions = {}
for m in pub_data['minions']:
minions[m] = False
Copy link
Contributor

Choose a reason for hiding this comment

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

we can just do

minions = {minion: False for minion in pub_data['minions']}

Copy link
Contributor Author

@jacksontj jacksontj Jun 18, 2018

Choose a reason for hiding this comment

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

@gtmanfred oh, did we drop python 2.6 support? IIRC dict comprehension isn't in 2.6

Copy link
Contributor

Choose a reason for hiding this comment

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

yup, we dropped 2.6 starting with 2017.7.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated :)

Copy link
Contributor

@DmitryKuzmenko DmitryKuzmenko left a comment

Choose a reason for hiding this comment

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

Awesome!

@jacksontj
Copy link
Contributor Author

Squashed those commits, and rebased on current develop (in hopes of fixing some of the CI failures ;) )

@gtmanfred
Copy link
Contributor

@jacksontj looks like you definitely fixed the syndic.

First differing element 0:
{'minion': True, 'localhost': True}
{'minion': True, 'localhost': True, 'sub_minion': True}

- [{'localhost': True, 'minion': True}]
+ [{'localhost': True, 'minion': True, 'sub_minion': True}]
?                                ++++++++++++++++++++

We are getting the subminion returning on the saltnado syndic tests, can you add that to the test case?

Thanks,
Daniel

@jacksontj
Copy link
Contributor Author

jacksontj commented Jun 19, 2018

When I run these locally I get a bunch of failures in the API tests with the localhost minion not responding. Actually, they fail with or without this patch, are these tests broken?

@gtmanfred
Copy link
Contributor

The localhost minion won't be there if you don't pass --ssh to start the extra sshd process that is used to test salt-ssh, and in this case testing enable_ssh_minions: True

@gtmanfred
Copy link
Contributor

The tests look good to me now.

This was broken because the behavior was to simply check the ckminions
and wait for only those returns to complete. This works assuming
ckminions is accurate (which there are many cases where it isn't, such
as syndics).

_disbatch_local's waiting on returns needs to match LocalClient's
behavior (namely that in get_iter_returns). This means we are allowed to
return when (1) we have waitged the min_wait_time (0 if not a syndic)
(2) no minions are running the job (3) all minions we saw running it are
done running the job. The only method allowed for earlier termination is
if the gather_job_timeout is exceeded.

Fixes saltstack#42659
Without this option we aren't really a syndic and it won't wait
@rallytime rallytime merged commit 08735c9 into saltstack:develop Jun 22, 2018
@jacksontj jacksontj deleted the issue_42659 branch June 22, 2018 19:13
@rallytime rallytime added ZZZ[Done]-back-ported-bf RETIRED The pull request has been back-ported to an older branch. and removed bugfix-bckport will be be back-ported to an older release branch by creating a PR against that branch labels Jun 25, 2018
rallytime pushed a commit that referenced this pull request Jun 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ZZZ[Done]-back-ported-bf RETIRED The pull request has been back-ported to an older branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rest_tornado is not able to return normal result
4 participants