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.

I forgot, we currently have on trunk two alternatives to run a ssh command:

1- Calling exec_command of our custom SSHClient (does not use the manager to get an active connection)
2- calling run_command in snappy_ecosystem_tests/utils/ssh.py Which actually uses the manager.

Why would we need 2 ways to execute a ssh command? This change unifies them.

« Back to merge proposal