-
-
Notifications
You must be signed in to change notification settings - Fork 746
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
Close proxycommand socket when its not needed anymore and resolve ssh zombies #4881
Conversation
@StackStorm/maintainers Can someone comment on how to update the Unit tests based on this find? |
@eedgar are you able to help us update the CI tests for this? |
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 think I spotted a bug preventing this from being adequately run and tested, as well as a lint issue.
Check out st2actions/tests/unit/test_paramiko_ssh.py
for examples of unit tests of this code.
Co-Authored-By: blag <[email protected]>
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.
@eedgar Can you please add a changelog here: https://github.com/StackStorm/st2/blob/master/CHANGELOG.rst
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.
Looking much better. Would like some TODO comments so we can come back and improve this code once we're developing for Python 3.6+ only (eg: after ST2 v3.2 is released, which should be "soon").
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.
Thanks!
Really thanks to @eedgar for the 2nd effort here! |
Needs confirmation that this is written correctly from @eedgar (-- @punkrokk )
Paramiko has a bug that is not patched, that causes ssh proxy connected to not get closed.
eg. zombie ssh processes are left from the proxycommand
If I were to modify my st2.conf file as follows:
We end up with zombie ssh connections. Paramiko has a bug, but it is not patched: paramiko/paramiko#789
This PR checks for sockets that are left open and closes them.