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

Make Paramiko backend try to reconnect if connection became closed #66

Closed
wants to merge 1 commit into from
Closed

Make Paramiko backend try to reconnect if connection became closed #66

wants to merge 1 commit into from

Conversation

AndreiPashkin
Copy link

For example, user might want host to be reverted to a pristine state
before each test run. In that case, the backend would fail, because
before this patch it was simply reused the old closed connection.

@philpep
Copy link
Contributor

philpep commented Feb 21, 2016

Hi, thanks for this PR.

I just wonder what the cost of calling get_transport().is_active() ? Maybe I'll be more efficient to catch the exception (socket.error I presume) and reinit the connection.

@AndreiPashkin
Copy link
Author

Exception occurs in different place. I think the cost is not that big to worry about it.

@AndreiPashkin
Copy link
Author

Offtop:
I'm trying to install my fork, but pbr throws an error:
Exception: Versioning for this project requires either an sdist tarball, or access to an upstream git repository. Are you sure that git is installed?

And I can't find a solution, maybe you know, how to fix it?

For example, user might want host to be reverted to a pristine state
before each test run. In that case, the backend would fail, because
before this patch it was simply reused the old closed connection.
@philpep
Copy link
Contributor

philpep commented Feb 21, 2016

Yes, pbr require a git repo or a sdist (as generated by python setup.py sdist) to get package version.
A solution to install your fork is to use pip. eg.

pip install 'git+https://github.com/AndrewPashkin/testinfra@make_paramiko_backend_not_reuse_collection#egg=testinfra'

@AndreiPashkin
Copy link
Author

@philpep, helped! thx 👍

@philpep
Copy link
Contributor

philpep commented Feb 29, 2016

@AndrewPashkin what's your usecase and what version of paramiko are you using ?

I cannot reproduce the issue (instantiate paramiko connection, kill the established connection with conntrack and reuse the paramiko connection), seems paramiko handle this already (version 1.16.0 here).

@philpep
Copy link
Contributor

philpep commented Feb 29, 2016

Ok with tcpkill I get this:

>>> b.run("ls")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "testinfra/backend/paramiko.py", line 84, in run
    chan = self.client.get_transport().open_session()
  File "/home/phil/venvs/testinfra/local/lib/python2.7/site-packages/paramiko/transport.py", line 702, in open_session
    timeout=timeout)
  File "/home/phil/venvs/testinfra/local/lib/python2.7/site-packages/paramiko/transport.py", line 786, in open_channel
    raise SSHException('SSH session not active')
paramiko.ssh_exception.SSHException: SSH session not active

@philpep
Copy link
Contributor

philpep commented Feb 29, 2016

I'm definitely not fan of calling is_active() for each command to run, this could result in a "time to check to time to use" issue. I've implemented an alternative patch in #71 , can you confirm that it solve your issue ? Thanks!

@AndreiPashkin
Copy link
Author

@philpep, my use case is that I'm trying to use Vagrant for testing SaltStack formulas and before each test run, my test suite reverts VM to a defined snapshot. After each rollback, SSH connection gets closed.

@philpep
Copy link
Contributor

philpep commented Mar 8, 2016

Closed in flavor of #71

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants