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

On Mon, Mar 6, 2017 at 10:22 PM, Heber Parrucci <
<email address hidden>> 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.
>

That can be done on current trunk right now, SSHManager.get_instance() is
for that.

> * 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.
>

I thought these methods already existed in the extended SSHClient, does
putting them behind a driver make things simpler/easier to consume ?

> * 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.
>

If we don't run tests over the internet, the chances of connection drop are
practically zero, unless we are hit by a nasty bug.

>
>
> 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.
>

run_command is indeed redundant in the presence of
SSHManager.get_instance(), we can remove that and *not* introduce
SSHClientDriver if it does not solve any problems we are facing.

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

Thanks for the detailed response. :)

>
> --
> https://code.launchpad.net/~canonical-platform-qa/snappy-
> ecosystem-tests/adding-ssh-client-driver/+merge/318948
> You are reviewing the proposed merge of lp:~canonical-platform-qa/
> snappy-ecosystem-tests/adding-ssh-client-driver into
> lp:snappy-ecosystem-tests.
>