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

On Mon, Mar 6, 2017 at 10:30 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.
> > * 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)
>

If a function uses SSHManager.get_instance() it would indeed be getting a
connected instance. If a class instance stores a reference to the
SSHClient, then in case tests are run over the internet we have the issue,
given we will be running them locally the issue does not exist to solve :)

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

run_command, indeed needs to go away replaced by SSHManager.get_instance(),
though introduction of a driver that hides the client seemed a little
unneeded change, but that's probably just me :)

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