Merge lp:~canonical-platform-qa/snappy-ecosystem-tests/adding-ssh-client-driver into lp:snappy-ecosystem-tests

[Heber Parrucci]

Adding SSHClientDriver to:
* Avoid passing the host, username, port in every run_command execution.
* Customize run command with cwd option
* Add pull method
* Ensure we execute each command against an active connection provided by the SSHManager

Centralize a way to do operations against an ssh client to:

* Avoid passing the host, username, port in every run_command execution.
* Customize run command with cwd option
* Add pull method
* Ensure we execute each command against an active connection provided by the SSHManager

[platform-qa-bot (platform-qa-bot)]

PASSED: Continuous integration, rev:31
https://platform-qa-jenkins.ubuntu.com/job/snappy-ecosystem-tests-ci/246/
Executed test runs:
    None: https://platform-qa-jenkins.ubuntu.com/job/generic-update-mp/2053/console

Click here to trigger a rebuild:
https://platform-qa-jenkins.ubuntu.com/job/snappy-ecosystem-tests-ci/246/rebuild

review: Approve (continuous-integration)

[Omer Akram (om26er)]

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.

review: Needs Information

[Heber Parrucci (heber013)]

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

[Heber Parrucci (heber013)]

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

[Santiago Baldassin (sbaldassin)]

Looks good in general. Minor comment inline

review: Needs Fixing

[Omer Akram (om26er)]

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

[Omer Akram (om26er)]

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

[Heber Parrucci (heber013)]

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

[Heber Parrucci (heber013)]

> Looks good in general. Minor comment inline
Applies the same response that for Omer:
We are going to run the tests over internet in the future for devices like: raspberry Pi, dragonboards, etc. And those devices will be on the lab (at least for running tests on Jenkins). In those cases we have to ensure that every time we execute a command over ssh, the connection is active.
That's why we cannot save the SshClient instance in the helpers, because we cannot ask to the client itself to get an active connection in every command execution.
The driver was created to resolve that problem. You store an instance in your helper and then execute commands, for every command you execute, it ensures that the ssh connection for that client is active by requesting the manager.
How can we achieve that using the SshClient instead of the driver?

[Santiago Baldassin (sbaldassin)]

Thanks for the explanation. What about keeping the ssh client and have something like this?

class EcosystemObject:

    def __init__(ip, user, port):
        self.ip = ip
        self.user = user
        self.port = port

    @property
    def ssh(self):
       return SSHManager.get_instance(self.ip, self.user, self.port)

class Snapcraft(EcosystemObject):

class Snapd(EcosystemObject)

That way every time you instantiate Snapcraft or Snapd, you'll get a property ssh that will always point to an active ssh connection

[lp:~canonical-platform-qa/snappy-ecosystem-tests/adding-ssh-client-driver]

32. By Heber Parrucci on 2017-03-07

Addressing review comments:
* Deleting SShDriver and adding back Sshclient
* Cwd parameter removed from client and adding in helper (To mantain paramiko's method signature)
* adding ssh property in each helper that needs ssh, to ensure an active connection for every command execution.

[Heber Parrucci (heber013)]

> Thanks for the explanation. What about keeping the ssh client and have
> something like this?
>
> class EcosystemObject:
>
> def __init__(ip, user, port):
> self.ip = ip
> self.user = user
> self.port = port
>
> @property
> def ssh(self):
> return SSHManager.get_instance(self.ip, self.user, self.port)
>
> class Snapcraft(EcosystemObject):
>
> class Snapd(EcosystemObject)
>
> That way every time you instantiate Snapcraft or Snapd, you'll get a property
> ssh that will always point to an active ssh connection

Thanks for the feedback. Comments addressed:
* Deleting SShDriver and adding back Sshclient
* Cwd parameter removed from client and adding in helper (To mantain paramiko's method signature)
* adding ssh property in each helper that needs ssh, to ensure an active connection for every command execution.

[platform-qa-bot (platform-qa-bot)]

PASSED: Continuous integration, rev:32
https://platform-qa-jenkins.ubuntu.com/job/snappy-ecosystem-tests-ci/253/
Executed test runs:
    None: https://platform-qa-jenkins.ubuntu.com/job/generic-update-mp/2065/console

Click here to trigger a rebuild:
https://platform-qa-jenkins.ubuntu.com/job/snappy-ecosystem-tests-ci/253/rebuild

review: Approve (continuous-integration)

[Omer Akram (om26er)]

Code looks good to me.

review: Approve

[lp:~canonical-platform-qa/snappy-ecosystem-tests/adding-ssh-client-driver]

33. By Heber Parrucci on 2017-03-07

merge from trunk

[platform-qa-bot (platform-qa-bot)]

PASSED: Continuous integration, rev:33
https://platform-qa-jenkins.ubuntu.com/job/snappy-ecosystem-tests-ci/255/
Executed test runs:
    None: https://platform-qa-jenkins.ubuntu.com/job/generic-update-mp/2067/console

Click here to trigger a rebuild:
https://platform-qa-jenkins.ubuntu.com/job/snappy-ecosystem-tests-ci/255/rebuild

review: Approve (continuous-integration)

[Santiago Baldassin (sbaldassin)]

Looks good

review: Approve

=== modified file 'snappy_ecosystem_tests/environment/setup.py'
--- snappy_ecosystem_tests/environment/setup.py	2017-03-01 14:52:43 +0000
+++ snappy_ecosystem_tests/environment/setup.py	2017-03-07 17:41:57 +0000
@@ -28,6 +28,7 @@
 
 logging.basicConfig(level=logging.DEBUG)
 
+
 def main(mode, profile):
     """Main script"""
     manager = get_manager(mode)

=== modified file 'snappy_ecosystem_tests/helpers/snapcraft/build_snap.py'
--- snappy_ecosystem_tests/helpers/snapcraft/build_snap.py	2017-03-02 16:24:59 +0000
+++ snappy_ecosystem_tests/helpers/snapcraft/build_snap.py	2017-03-07 17:41:57 +0000
@@ -53,13 +53,21 @@
 class SnapBuilder:
     """Class to build a snap on a remote machine and download its content."""
     def __init__(self, hostname, username, port=22):
-        self.client = SSHManager.get_instance(hostname, username, port)
+        self.hostname = hostname
+        self.username = username
+        self.port = port
+
+    @property
+    def ssh(self):
+        """Property tu ensure we get an active connection for every command
+         execution over ssh"""
+        return SSHManager.get_instance(self.hostname, self.username, self.port)
 
     def is_host_setup(self):
         """Return bool representing whether the remote host is setup by
         checking if snapcraft package is installed."""
         try:
-            self.client.exec_command('which snapcraft')
+            self.ssh.exec_command('which snapcraft')
             return True
         except ValueError:
             return False
@@ -68,7 +76,7 @@
         """Setup the host to build a snap, currently only installs
         snapcraft."""
         for command in COMMANDS_SETUP:
-            self.client.exec_command(command)
+            self.ssh.exec_command(command)
 
     def build(self, name, version, summary, description, grade='stable',
               confinement='strict'):
@@ -86,9 +94,9 @@
         """
         if not self.is_host_setup():
             self.setup_host()
-        tempdir = self.client.exec_command('mktemp -d')
-        self.client.exec_command('mkdir snap', cwd=tempdir)
-        self.client.exec_command('echo "{}" > snap/snapcraft.yaml'.format(
+        tempdir = self.ssh.exec_command('mktemp -d')
+        self.ssh.exec_command('mkdir snap', cwd=tempdir)
+        self.ssh.exec_command('echo "{}" > snap/snapcraft.yaml'.format(
             SNAPCRAFT_YAML.format(
                 snap_name=name,
                 version=version,
@@ -98,13 +106,13 @@
                 confinement=confinement
             )
         ), cwd=tempdir)
-        self.client.exec_command('snapcraft', cwd=tempdir)
+        self.ssh.exec_command('snapcraft', cwd=tempdir)
         return os.path.join(
             tempdir,
             '{name}_{version}_{arch}.snap'.format(
                 name=name,
                 version=version,
-                arch=self.client.exec_command('dpkg --print-architecture')
+                arch=self.ssh.exec_command('dpkg --print-architecture')
             )
         )
 
@@ -136,5 +144,5 @@
         output_path_absolute = os.path.abspath(
             os.path.join(output_location, PurePath(remote_snap_path).name)
         )
-        self.client.pull(remote_snap_path, output_path_absolute)
+        self.ssh.pull(remote_snap_path, output_path_absolute)
         return output_path_absolute

=== modified file 'snappy_ecosystem_tests/helpers/snapcraft/client.py'
--- snappy_ecosystem_tests/helpers/snapcraft/client.py	2017-03-03 14:06:17 +0000
+++ snappy_ecosystem_tests/helpers/snapcraft/client.py	2017-03-07 17:41:57 +0000
@@ -25,9 +25,9 @@
 import time
 
 from snappy_ecosystem_tests.models.snap import Snap
-from snappy_ecosystem_tests.utils import ssh
 from snappy_ecosystem_tests.utils.commands import build_command
 from snappy_ecosystem_tests.utils.filters import filter_list
+from snappy_ecosystem_tests.utils.ssh import SSHManager
 from snappy_ecosystem_tests.utils.storeconfig import get_current_store
 
 from snappy_ecosystem_tests.commons.config import CONFIG_STACK
@@ -72,8 +72,13 @@
         self._login = False
         self._cleanup()
 
-    @staticmethod
-    def _run_snapcraft_command_ssh(command, debug=True):
+    @property
+    def ssh(self):
+        """Property tu ensure we get an active connection for every command
+         execution over ssh"""
+        return SSHManager.get_instance(HOSTNAME, USERNAME, PORT)
+
+    def _run_snapcraft_command_ssh(self, command, debug=True):
         """
         Run snapcraft command via ssh
         :param command: the command to be executed.
@@ -85,25 +90,21 @@
             command = build_snapcraft_command(command)
         if debug:
             command += ' -d'
-        return ssh.run_command(command, hostname=HOSTNAME, username=USERNAME,
-                               port=PORT)
+        return self.ssh.exec_command(command)
 
-    @staticmethod
-    def assert_logged_in():
+    def assert_logged_in(self):
         """Assert that an user is logged.
         :raise ValueError: in case login is false
         """
-        if Snapcraft.is_logged_in() is False:
+        if self.is_logged_in() is False:
             raise ValueError("User is not logged in, "
                              "please login before using this command")
 
-    @staticmethod
-    def is_logged_in():
+    def is_logged_in(self):
         """Return bool representing if the user is logged into snapcraft."""
         try:
-            output = ssh.run_command(
-                'cat %s' % SNAPCRAFT_CONFIG_FILE_PATH,
-                hostname=HOSTNAME, username=USERNAME, port=PORT)
+            output = self.ssh.exec_command(
+                'cat %s' % SNAPCRAFT_CONFIG_FILE_PATH)
             return output is not ''
         except ValueError:
             return False
@@ -112,26 +113,23 @@
         """Perform cleanup actions"""
         self.logout()
 
-    @staticmethod
-    def logout():
+    def logout(self):
         """logout of snapcraft store session
         :return: True if logout was successful, False otherwise
         """
-        Snapcraft._run_snapcraft_command_ssh(COMMAND_LOGOUT)
-        return not Snapcraft.is_logged_in()
+        self._run_snapcraft_command_ssh(COMMAND_LOGOUT)
+        return not self.is_logged_in()
 
-    @staticmethod
-    def login(email=LOGIN_EMAIL, password=LOGIN_PASSWORD):
+    def login(self, email=LOGIN_EMAIL, password=LOGIN_PASSWORD):
         """Login to Snapcraft. If email and/or password are not provided,
         use the default ones.
         :param email: the SSO email
         :param password: the email password
         """
-        ssh.run_command(
+        self.ssh.exec_command((
             COMMANDS_LOGIN.format(email=email, password=password,
-                                  snapcraft=PATH_SNAPCRAFT),
-            hostname=HOSTNAME, username=USERNAME, port=PORT)
-        return Snapcraft.is_logged_in()
+                                  snapcraft=PATH_SNAPCRAFT)))
+        return self.is_logged_in()
 
     def list_registered(self, debug=True, raw=False):
         """Call snapcraft list-registered command,
@@ -180,7 +178,7 @@
         if private:
             command += ' --private'
         try:
-            Snapcraft._run_snapcraft_command_ssh(command)
+            self._run_snapcraft_command_ssh(command)
         except subprocess.CalledProcessError as _e:
             LOGGER.error('Unable to register snap: %s', _e.output)
             return False

=== modified file 'snappy_ecosystem_tests/helpers/snapd/snapd.py'
--- snappy_ecosystem_tests/helpers/snapd/snapd.py	2017-03-02 18:36:24 +0000
+++ snappy_ecosystem_tests/helpers/snapd/snapd.py	2017-03-07 17:41:57 +0000
@@ -25,7 +25,7 @@
 
 import yaml
 
-from snappy_ecosystem_tests.utils import ssh
+from snappy_ecosystem_tests.utils.ssh import SSHManager
 from snappy_ecosystem_tests.utils.user import get_snapd_remote_host_credentials
 
 PATH_SNAP = '/usr/bin/snap'
@@ -51,107 +51,116 @@
 HOSTNAME, USERNAME, PORT = get_snapd_remote_host_credentials()
 
 
-def run_snapd_command_ssh(parameters, cwd=''):
-    """Run snapd command over ssh.
-
-    :param parameters: a string containing parameters for the `snap` command.
-    :param cwd: the current working directory on the remote where the command
-        should run.
-    :returns: stdout of the command
-    """
-    return ssh.run_command('{} {}'.format(PATH_SNAP, parameters), cwd=cwd,
-                           hostname=HOSTNAME, username=USERNAME, port=PORT)
-
-
-def login(email, password):
-    """Login to snapd.
-
-    :param email: Ubuntu SSO account email address.
-    :param password: Ubuntu SSO account password.
-    """
-    ssh.run_command(COMMANDS_LOGIN.format(email=email, password=password))
-    return is_logged_in(email)
-
-
-def is_logged_in(email):
-    """Return bool representing if the user is logged into snapd."""
-    try:
-        return json.loads(ssh.run_command('cat ~/.snap/auth.json')).get(
-            'email') == email
-    except ValueError:
+class Snapd:
+    """Contain Snapd specific functionality to use via command
+    line interface"""
+
+    @property
+    def ssh(self):
+        """Property tu ensure we get an active connection for every command
+         execution over ssh"""
+        return SSHManager.get_instance(HOSTNAME, USERNAME, PORT)
+
+    def run_snapd_command_ssh(self, command, cwd=''):
+        """Run snapd command over ssh.
+
+        :param command: a string containing parameters for the `snap` command.
+        :param cwd: the current working directory on the remote where
+        the command should run.
+        :returns: stdout of the command
+        """
+        if cwd:
+            command = '{};{}'.format(cwd, command)
+        return self.ssh.exec_command(command)
+
+    def login(self, email, password):
+        """Login to snapd.
+
+        :param email: Ubuntu SSO account email address.
+        :param password: Ubuntu SSO account password.
+        """
+        self.ssh.exec_command(COMMANDS_LOGIN.format(email=email,
+                                                    password=password))
+        return self.is_logged_in(email)
+
+    def is_logged_in(self, email):
+        """Return bool representing if the user is logged into snapd."""
+        try:
+            return json.loads(
+                self.ssh.exec_command(
+                    'cat ~/.snap/auth.json')).get('email') == email
+        except ValueError:
+            return False
+
+    def logout(self, email):
+        """Logout snapd current user."""
+        if self.is_logged_in(email):
+            self.ssh.exec_command(COMMAND_LOGOUT)
+
+    def download(self, snap, channel=CHANNEL_STABLE):
+        """Download the requested snap.
+
+        :param snap: name of the snap to download.
+        :param channel: name of the release channel to download from.
+        """
+        command = COMMAND_DOWNLOAD.format(snap=snap, channel=channel)
+        self.run_snapd_command_ssh(command,
+                                   cwd=self.ssh.exec_command('mktemp -d'))
+
+    def install(self, snap, channel=CHANNEL_STABLE):
+        """Install the requested snap."""
+        self.ssh.exec_command(COMMAND_INSTALL.format(snap=snap,
+                                                     channel=channel))
+
+    def _is_installed(self, snap):
+        """Return bool representing whether a snap is installed."""
+        for installed_snap in Snapd._parse_output(
+                self.run_snapd_command_ssh(COMMAND_LIST)):
+            if installed_snap['name'] == snap:
+                return True
         return False
 
-
-def logout(email):
-    """Logout snapd current user."""
-    if is_logged_in(email):
-        run_snapd_command_ssh(COMMAND_LOGOUT)
-
-
-def download(snap, channel=CHANNEL_STABLE):
-    """Download the requested snap.
-
-    :param snap: name of the snap to download.
-    :param channel: name of the release channel to download from.
-    """
-    command = COMMAND_DOWNLOAD.format(snap=snap, channel=channel)
-    run_snapd_command_ssh(command, cwd=ssh.run_command('mktemp -d'))
-
-
-def install(snap, channel=CHANNEL_STABLE):
-    """Install the requested snap."""
-    run_snapd_command_ssh(COMMAND_INSTALL.format(snap=snap, channel=channel))
-
-
-def _is_installed(snap):
-    """Return bool representing whether a snap is installed."""
-    for installed_snap in _parse_output(run_snapd_command_ssh(COMMAND_LIST)):
-        if installed_snap['name'] == snap:
-            return True
-    return False
-
-
-def remove(snap):
-    """Remove a snap, if its already installed."""
-    if _is_installed(snap):
-        run_snapd_command_ssh(COMMAND_REMOVE.format(snap=snap))
-
-
-def info(snap, verbose=False):
-    """Query the Ubuntu store of the information about a snap.
-
-    :param snap: Name of the snap for which the info is required.
-    :param verbose: Whether to information should be detailed.
-    :return: Return a dictionary containing information about the snap.
-    """
-    command = COMMAND_INFO.format(snap=snap)
-    if verbose:
-        command = ' '.join([command, '--verbose'])
-    return yaml.load("""{}""".format(run_snapd_command_ssh(command)))
-
-
-def refresh(snap, channel=CHANNEL_STABLE):
-    """Refresh the requested snap."""
-    run_snapd_command_ssh(COMMAND_REFRESH.format(snap=snap, channel=channel))
-
-
-def _parse_output(raw_output):
-    """Pretty parse the output from snapd commands like `find` and `list`.
-
-    :param raw_output: The raw output returned from the command
-    :return: A list of dictionaries containing sorted results from the output.
-    """
-    split_output = raw_output.split('\n')
-    headers = [header.lower() for header in split_output.pop(0).split()]
-    return [dict(zip(headers, line.split())) for line in split_output]
-
-
-def find(keyword):
-    """Find snaps based on the provided filters
-
-    :param keyword: Keyword to use for the query.
-    :return: Return a list of dictionaries containing information about
-        snaps matching the `keyword`.
-    """
-    return _parse_output(
-        run_snapd_command_ssh(COMMAND_FIND.format(search_term=keyword)))
+    def remove(self, snap):
+        """Remove a snap, if its already installed."""
+        if self._is_installed(snap):
+            self.run_snapd_command_ssh(COMMAND_REMOVE.format(snap=snap))
+
+    def info(self, snap, verbose=False):
+        """Query the Ubuntu store of the information about a snap.
+
+        :param snap: Name of the snap for which the info is required.
+        :param verbose: Whether to information should be detailed.
+        :return: Return a dictionary containing information about the snap.
+        """
+        command = COMMAND_INFO.format(snap=snap)
+        if verbose:
+            command = ' '.join([command, '--verbose'])
+        return yaml.load("""{}""".format(self.run_snapd_command_ssh(command)))
+
+    def refresh(self, snap, channel=CHANNEL_STABLE):
+        """Refresh the requested snap."""
+        self.run_snapd_command_ssh(COMMAND_REFRESH.format(snap=snap,
+                                                          channel=channel))
+
+    @staticmethod
+    def _parse_output(raw_output):
+        """Pretty parse the output from snapd commands like `find` and `list`.
+
+        :param raw_output: The raw output returned from the command
+        :return: A list of dictionaries containing sorted results
+        from the output.
+        """
+        split_output = raw_output.split('\n')
+        headers = [header.lower() for header in split_output.pop(0).split()]
+        return [dict(zip(headers, line.split())) for line in split_output]
+
+    def find(self, keyword):
+        """Find snaps based on the provided filters
+
+        :param keyword: Keyword to use for the query.
+        :return: Return a list of dictionaries containing information about
+            snaps matching the `keyword`.
+        """
+        return Snapd._parse_output(
+            self.run_snapd_command_ssh(COMMAND_FIND.format(
+                search_term=keyword)))

=== modified file 'snappy_ecosystem_tests/tests/test_snapd.py'
--- snappy_ecosystem_tests/tests/test_snapd.py	2017-02-22 15:47:09 +0000
+++ snappy_ecosystem_tests/tests/test_snapd.py	2017-03-07 17:41:57 +0000
@@ -22,7 +22,7 @@
 
 import testtools
 
-from snappy_ecosystem_tests.helpers.snapd import snapd
+from snappy_ecosystem_tests.helpers.snapd.snapd import Snapd
 from snappy_ecosystem_tests.utils.storeconfig import get_store_credentials
 
 
@@ -30,13 +30,14 @@
     """Tests for snapd."""
     def setUp(self):
         super().setUp()
+        self.snapd = Snapd()
 
     def test_info(self):
         """Ensure the publisher of core snap in canonical."""
-        self.assertTrue('canonical', snapd.info('core')['publisher'])
+        self.assertTrue('canonical', self.snapd.info('core')['publisher'])
 
     def test_login(self):
         """Login the snapd user."""
         email, password = get_store_credentials()
-        self.assertTrue(snapd.login(email, password))
-        self.addCleanup(snapd.logout, email)
+        self.assertTrue(self.snapd.login(email, password))
+        self.addCleanup(self.snapd.logout, email)

=== modified file 'snappy_ecosystem_tests/utils/ssh.py'
--- snappy_ecosystem_tests/utils/ssh.py	2017-03-02 20:38:49 +0000
+++ snappy_ecosystem_tests/utils/ssh.py	2017-03-07 17:41:57 +0000
@@ -21,9 +21,9 @@
 """Module to connect to ssh client for running tests."""
 
 import logging
+
 import paramiko
 
-from snappy_ecosystem_tests.utils.user import get_snapd_remote_host_credentials
 from snappy_ecosystem_tests.utils.storeconfig import get_current_store
 from snappy_ecosystem_tests.environment.data.snapd import STAGING_VARIABLES
 
@@ -33,13 +33,17 @@
 class SSHClient(paramiko.SSHClient):
     """Extended SSHClient."""
 
-    def exec_command(self, command, bufsize=-1, timeout=None, get_pty=False,  # pylint: disable=arguments-differ
-                     environment=None, cwd=''):
+    def exec_command(self, command, bufsize=-1, timeout=None, get_pty=False,
+                     environment=None):
         """Execute the given command over ssh."""
-        if cwd:
-            command = 'cd {}; {}'.format(cwd, command)
-        _, stdout, stderr = super().exec_command(command, bufsize, timeout,
-                                                 get_pty, environment)
+        if environment is None:
+            # FIXME: get environment variables dinamically
+            if get_current_store() == 'staging':
+                environment = STAGING_VARIABLES
+        _, stdout, stderr = super().exec_command(
+            command, bufsize, timeout,
+            get_pty,
+            environment=environment)
         if stdout.channel.recv_exit_status() != 0:
             raise ValueError(stderr.read().decode().strip())
         response = stdout.read().decode().strip()
@@ -102,27 +106,3 @@
             username == client.get_transport().get_username()
         ]
         return connections[0] if connections else None
-
-
-def run_command(command, cwd=None, hostname=None, username=None, port=None):
-    """Run the given command on remote machine over ssh.
-
-    :param command: a string of the command to run.
-    :param cwd: Current working directory for the command.
-    :param hostname: The host to run command on.
-    :param username: Name of the user on the remote host to login to.
-    :param port: SSH port number.
-    :raises ValueError: if command exits with non-zero status.
-    :return: the stdout of the command.
-    """
-    # Take snapd remote credentials as default if they are not provided.
-    # Just arbitrary... Could have been other ones.
-    _hostname, _username, _port = get_snapd_remote_host_credentials()
-    ssh = SSHManager.get_instance(
-        hostname or _hostname, username or _username, port or _port)
-    return ssh.exec_command(
-        command,
-        cwd=cwd,
-        environment=STAGING_VARIABLES if get_current_store() == 'staging'
-        else None
-    )