Code review comment for lp:~canonical-platform-qa/snappy-ecosystem-tests/adding-ssh-client-driver

Revision history for this message
Heber Parrucci (heber013) wrote :

> Excuse my ignorance, can you tell what problem this branch tries to solve ?
> From the look of it, it seems to be making things more complex. The
> SSHClientDriver is now the only consumer of SSHManager.
>
> I do see the SSHClientDriver gets a connected instance from the manager each
> time a command is run, this does solve a problem, though that will only be
> relevant if our tests were running over the internet, right now they are
> running on the local network.
>
> Do you know if in future our tests will be running over the internet instead
> of the local network where the risk of ssh connection drop practically
> diminishes.

Sure no problem!
The change description might not be clear enough. So I will provide more details on each item:

* Avoid passing the host, username, port in every run_command execution. --> Currently we have a run_command at module level so we need to provide host, username, etc every time we call it. Encapsulating the logic in a class and providing the parameters in the object initialization, we ensure that the ssh specific parameters will be provided once per remote helper.
* Customize run command with cwd option --> per your request, I added cwd named parameter in run_command so you can specify the dir in which the command will be executed.
* Add pull method --> the pull was also at module level.
* Ensure we execute each command against an active connection provided by the SSHManager --> the fact that the ssh connections are more likely to drop if tests are executed over internet, does not alter the design (the keywords are *more likely*). I mean, we could use the same client object as well, but in this case we would me assuming the risk. Do you note any performance issues of requesting an active connection in every command? Remember that it is stored in the Manager connections pool. Furthermore, if I change the logic to use the same connection on the ssh driver, I will not be using the feature that the SshManager provides.

Finally, we currently have on trunk a custom SShClient class that extends the paramiko one and overrides exec_command just to provide a dir in which the command will be executed, and it is not using our manager to get the ssh connection.

Let me know if that make more sense for you now.

« Back to merge proposal