-
Notifications
You must be signed in to change notification settings - Fork 199
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
Handles a wait_for_connection
call right after a task caused a shutdown
#710
Conversation
/aside to travis: oh travis! Please report results! |
Travis does not seem to wanna launch a test 🤦 |
New plan: |
@cognifloyd can you review this one as well? :) 🙏 |
ooh I'll squash commits into 1 actually since there's no current chat history |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand all of the code involved (I haven't been able to grok all of the innards of mitogen so far), but from what I can tell, this looks reasonable.
Just one question about the test playbook.
# since things are ran on localhost; Azure DevOps loses connection and fails | ||
# TODO: do we want to install docker a different way to be able to do this for other tests too | ||
--- | ||
# this should only run on our Mac hosts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What prevents this from running on non-mac hosts? I don't see any safety checks to bail if the host is not a mac, instead it will just go up in smoke when the dmg can't be unpacked, or the /Volumes directory doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The target
host only runs on our Mac setup because Mac is the only test that runs localhost_ansible
tests (see azure-pipelines.yml
). Technically someone could run it themselves on a non-mac and it might not work but the tests are really set up for running through CI mostly so I didn't worry about that. I left a note so that if we end up running localhost_ansible
on another host then we'd know to refactor this file a bit. It's not super clear though; if you'd like I could edit the comment to reflect something different!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah. The comment as-is is enough. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
fixes the |
Fixes #655
wait_for_connection
in Ansible calls both aconnection.reset()
as well as an_execute_module
.In the case of issue 655, a shutdown call right before the
wait_for_connection
caused Mitogen to disconnect from the host and then be stuck in a bad state when_execute_module
was called.wait_for_connection
is set up to retry continually to connect, which was masking errors that Mitogen was throwing when it couldn't set up a connection.To fix I forced Mitogen to reconnect if the
ping
module is used withwait_for_connection
and made sure that if we're trying to create aPool
that we have arouter
.