Feature request #4715: configurable ssh connect timeout #4914
Conversation
pull-request-size
bot
added
the
size/M
…pgithub.com:winem/st2 into feature-4715-make-ssh-connect-timeout-configurable
|
I thought about the unit tests as well and back then I just knew how to test it manually. It's probably a bit tricky but I think I just got an idea for a proper unit test that can differentiate between failures due to breaching the |
…out default of 60s
This reverts commit 4cb122d which passed the timeout parameter to the wrong method
…den ssh_connect_timeout
|
Marked as draft because a merge of the master branch into my feature branch went wrong. Will provide the proper solution before end of this week. In the meantime any input regarding my previous comment and the unit tests is highly appreciated! |
…pgithub.com:/winem/st2 into feature-4715-make-ssh-connect-timeout-configurable
…t via ssh" This reverts commit ecaf779.
Hi @armab, first of all, I fixed all the lint issues so that the current tests pass. I thought about the tests a bit more and am not sure what a proper implementation would look like. I know that some people use google.com:81 in tests to run a timeout but I think that every request to foreign infrastructure should be avoided (and is just the wrong way). Only thing that seems to be an option is to use Mocks side_effects and raise an exception. In this case we would see the exception but it's just a mocked one and does not actually prove that the action failed after the ssh_connect_timeout which would have to be lower than the action timeout in this case. So I ran out of ideas but am all in for ideas and every input is appreciated. Please let me know if you have any ideas or any hints for something I could look into. Cheers, |
|
Anytime! What do you think about the (missing) tests? Do you see any chance to cover this feature by a test? |
|
Currently I can't help much here, but I really appreciate the effort trying to add the test scenarios. If nothing worked, - I'm good to merge it as is. |
This PR adds support for ssh_connect_timeout as new configuration parameter in the ssh_runner group. It was suggested in #4715 and follows the fail-fast principle when the SSH connection can not even be established.
The runner will fail if the TCP connect to the target host does not succeed within the specified timeout. (see: http://docs.paramiko.org/en/stable/api/client.html -> SSHClient -> connect -> timeout).
This was tested with a high number of test cases with different defaults, ssh_connect_timeouts in the st2.conf and a variation of action timeouts.
This one makes #4913 which I just closed obsolete.