Merge lp:~canonical-ci-engineering/adt-cloud-worker/enable-testing into lp:adt-cloud-worker

[Thomi Richards]

Add tests, refactor code.

This branch makes several changes, all with an eye to adding some tests to this service:

* Move the bulk of the code into an adt_cloud_worker package, so it can be imported.
* Create the 'tests' sub-package, and create a file with a few simple tests.
* Refactor code which can easily be written in a functional style out of the main body of the code so it can be tested.
* Update README with instructions on how to run the tests.

While this can land right now, I still want to add some system-level tests to ensure that the service actually does something useful!

[Celso Providelo (cprov)]

Thomi,

Thanks for working on tests for the project. I have no objections to the code refactoring, it is still small e reads nicely.

There is the py3 deps discussion on going, but this change does really affects it, so my vote is for landing once you consider my inline comments (not blocking, though).

review: Approve

[lp:~canonical-ci-engineering/adt-cloud-worker/enable-testing]

11. By Thomi Richards on 2015-03-04

Merge code that uses pip to get dependencies.

12. By Thomi Richards on 2015-03-04

Merged trunk.

13. By Thomi Richards on 2015-03-04

Fix up code from review.

[Celso Providelo (cprov)]

review: Approve

=== modified file '.bzrignore'
--- .bzrignore	2015-03-04 19:28:28 +0000
+++ .bzrignore	2015-03-04 20:24:52 +0000
@@ -1,1 +1,2 @@
+adt_cloud_worker.egg-info
 ve

=== modified file 'README.rst'
--- README.rst	2015-03-04 19:38:23 +0000
+++ README.rst	2015-03-04 20:24:52 +0000
@@ -42,6 +42,18 @@
   $ python setup.py develop
 
 
+
+Run the tests!
+==============
+
+Install dependencies::
+
+  $ pip install -r test_requirements.txt
+
+Run those tests - with vigour!::
+
+  $ python setup.py test
+
 Run the Service
 ===============
 

=== modified file 'adt-cloud-worker.py'
--- adt-cloud-worker.py	2015-03-03 19:03:34 +0000
+++ adt-cloud-worker.py	2015-03-04 20:24:52 +0000
@@ -19,168 +19,7 @@
 
 """Main entry point for adt-cloud-worker."""
 
-
-from __future__ import print_function
-
-import argparse
-import configparser
-import kombu
-from kombu.log import get_logger
-from kombu.mixins import ConsumerMixin
-from kombu.utils.debug import setup_logging
-import os
-import shutil
-import subprocess
-from swiftclient import client
-import tarfile
-import tempfile
-
-logger = get_logger(__name__)
-
-
-class AdtNovaWorker(ConsumerMixin):
-
-    def __init__(self, connection, config):
-        self.connection = connection
-        self.config = config
-        self.name = self.config.get('adt', 'name')
-
-    def get_consumers(self, Consumer, channel):
-        """Return consumers instances for all configured queues.""" 
-        exchange = kombu.Exchange("adt.exchange", type="topic")
-        queues = [
-            kombu.Queue(
-                'adt.requests.{}'.format(tag), exchange, routing_key=tag)
-            for tag in self.config.get('adt', 'tags').split()
-        ]
-        return [Consumer(queues=queues, callbacks=[self.process])]
-
-    def process(self, body, message):
-        """Process incomming test request.
-
-        Run requested tests and posts results to the 'adt_results' queue
-        for later checking.
-        """
-        logger.info('Got: {}'.format(body))
-        # TODO: body validation
-        body['worker'] = self.name
-        # Run requested tests safely.
-        try:
-            # Create and temporary directory for storing adt results.
-            result_dir = tempfile.mkdtemp(
-                prefix='adt-{}-{}'.format(self.name, body['request_id']))
-            # Run `adt-run`.
-            adt_kwargs = body.copy()
-            adt_kwargs['result_dir'] = result_dir
-            body['exit_code'] = self.run_adt(**adt_kwargs)
-            # Compress the results directory in a temporary file.
-            _unused, tarball_path = tempfile.mkstemp()
-            with tarfile.open(tarball_path, "w:gz") as tar:
-                for entry in os.listdir(result_dir):
-                    tar.add(os.path.join(result_dir, entry), arcname=entry)
-            # Upload the results tarball to swift with a temporary name
-            # 'adt-<req_id>/results.tgz.tmp', result checker will rename
-            # if after verification.
-            swift_client = client.Connection(
-                authurl=self.config.get('nova', 'os_auth_url'),
-                user=self.config.get('nova', 'os_username'),
-                key=self.config.get('nova', 'os_password'),
-                os_options={
-                    'tenant_name': self.config.get('nova', 'os_tenant_name'),
-                    'region_name': self.config.get('nova', 'os_region_name'),
-                }, auth_version=(
-                    '/v1.' in self.config.get('nova','os_auth_url') and '1.0'
-                    or '2.0'))
-            container_name = 'adt-{}'.format(body['request_id'])
-            swift_client.put_container(container_name)
-            with open(tarball_path) as fd:
-                swift_client.put_object(
-                    container_name, obj='results.tgz.tmp', contents=fd,
-                    content_type='application/x-compressed')
-        except Exception as e:
-            # Unexpected failures ... 
-            logger.error(e, exc_info=True)
-            body['exit_code'] = '100'
-        finally:
-            # The post results
-            # TODO: send error logging to result-checker in the message
-            queue = self.connection.SimpleQueue('adt.results')
-            queue.put(body)
-            queue.close()
-            # Drop the result directory and result tarball.
-            shutil.rmtree(result_dir)
-            os.remove(tarball_path)
-            message.ack()
-            logger.info('Done!')
-
-    def run_adt(self, **kwargs):
-        """Run adt-run according to the given request parameters.
-
-        Always resets the environment with configured nova variables.
-
-        Returns the exit code from adt-run.
-        """
-        # TODO: We probably want something more clever here:
-
-        # what information do we need from the message?
-        adt_run_args = [
-            '--apt-source', kwargs['package_name'],
-            '--output-dir', kwargs['result_dir'],
-        ]
-        if 'apt_pocket' in kwargs:
-            adt_run_args += [
-                '--apt-pocket', kwargs['apt_pocket'],
-                '--apt-upgrade',
-        ]
-        adt_ssh_nova_args = [
-            '--',
-            '--flavor', kwargs['nova_flavor'],
-            '--image', kwargs['nova_image'],
-        ]
-
-        arguments = (
-            adt_run_args +
-            ['---', 'ssh', '-s', 'nova'] +
-            adt_ssh_nova_args +
-            self.config.get('nova', 'extra_args').split()
-        )
-
-        # Setup nova environment variables based on the configuration file.
-        for k, v in self.config.items('nova'):
-            if not k.startswith('os_'):
-                continue
-            os.environ[k.upper()] = str(v)
-
-        try:
-            subprocess.check_call(['adt-run'] + arguments)
-        except subprocess.CalledProcessError as e:
-            # log?
-            # TODO: filter log content to avoid leaking cloud credentials.
-            return e.returncode
-        return 0
-
-
-def main():
-    setup_logging(loglevel='DEBUG', loggers=[''])
-
-    parser = argparse.ArgumentParser(
-        description='ADT cloud worker ...')
-    parser.add_argument('-c', '--conf', default='.adt-service.conf',
-                        help='Configuration file path')
-    args = parser.parse_args()
-
-    # Load configuration options.
-    config = configparser.ConfigParser()
-    config.read(args.conf)
-    amqp_uris = config.get('amqp', 'uris').split()
-
-    with kombu.Connection(amqp_uris) as conn:
-        try:
-            worker = AdtNovaWorker(conn, config)
-            worker.run()
-        except KeyboardInterrupt:
-            print('Bye!')
-
+from adt_cloud_worker import main
 
 if __name__ == '__main__':
     main()

=== added directory 'adt_cloud_worker'
=== added file 'adt_cloud_worker/__init__.py'
--- adt_cloud_worker/__init__.py	1970-01-01 00:00:00 +0000
+++ adt_cloud_worker/__init__.py	2015-03-04 20:24:52 +0000
@@ -0,0 +1,207 @@
+# adt-cloud-worker
+# Copyright (C) 2015 Canonical
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+"""ADT Cloud worker functional code."""
+
+
+from __future__ import print_function
+
+import argparse
+import configparser
+import kombu
+from kombu.log import get_logger
+from kombu.mixins import ConsumerMixin
+from kombu.utils.debug import setup_logging
+import os
+import shutil
+import subprocess
+from swiftclient import client
+import tarfile
+import tempfile
+
+logger = get_logger(__name__)
+
+
+class AdtNovaWorker(ConsumerMixin):
+
+    def __init__(self, connection, config):
+        self.connection = connection
+        self.config = config
+        # Set the nova configuration env vars:
+        _set_nova_environment_variables(self.config.items('nova'))
+        self.name = self.config.get('adt', 'name')
+
+    def get_consumers(self, Consumer, channel):
+        """Return consumers instances for all configured queues."""
+        exchange = kombu.Exchange("adt.exchange", type="topic")
+        queues = [
+            kombu.Queue(
+                'adt.requests.{}'.format(tag), exchange, routing_key=tag)
+            for tag in self.config.get('adt', 'tags').split()
+        ]
+        return [Consumer(queues=queues, callbacks=[self.process])]
+
+    def process(self, body, message):
+        """Process incomming test request.
+
+        Run requested tests and posts results to the 'adt_results' queue
+        for later checking.
+        """
+        logger.info('Got: {}'.format(body))
+        # TODO: body validation
+        body['worker'] = self.name
+        # Run requested tests safely.
+        try:
+            # Create and temporary directory for storing adt results.
+            result_dir = tempfile.mkdtemp(
+                prefix='adt-{}-{}'.format(self.name, body['request_id']))
+            # Run `adt-run`.
+            adt_kwargs = body.copy()
+            adt_kwargs['result_dir'] = result_dir
+            adt_kwargs['nova_extra_args'] = self.config.get('nova', 'extra_args').split()
+            arguments = _make_adt_argument_list(adt_kwargs)
+
+            body['exit_code'] = run_adt(arguments)
+            # Compress the results directory in a temporary file.
+            _unused, tarball_path = tempfile.mkstemp()
+            with tarfile.open(tarball_path, "w:gz") as tar:
+                for entry in os.listdir(result_dir):
+                    tar.add(os.path.join(result_dir, entry), arcname=entry)
+            # Upload the results tarball to swift with a temporary name
+            # 'adt-<req_id>/results.tgz.tmp', result checker will rename
+            # if after verification.
+            swift_client = client.Connection(
+                authurl=self.config.get('nova', 'os_auth_url'),
+                user=self.config.get('nova', 'os_username'),
+                key=self.config.get('nova', 'os_password'),
+                os_options={
+                    'tenant_name': self.config.get('nova', 'os_tenant_name'),
+                    'region_name': self.config.get('nova', 'os_region_name'),
+                }, auth_version=(
+                    '/v1.' in self.config.get('nova','os_auth_url') and '1.0'
+                    or '2.0'))
+            container_name = 'adt-{}'.format(body['request_id'])
+            swift_client.put_container(container_name)
+            with open(tarball_path) as fd:
+                swift_client.put_object(
+                    container_name, obj='results.tgz.tmp', contents=fd,
+                    content_type='application/x-compressed')
+        except Exception as e:
+            # Unexpected failures ...
+            logger.error(e, exc_info=True)
+            body['exit_code'] = '100'
+        finally:
+            # The post results
+            # TODO: send error logging to result-checker in the message
+            queue = self.connection.SimpleQueue('adt.results')
+            queue.put(body)
+            queue.close()
+            # Drop the result directory and result tarball.
+            shutil.rmtree(result_dir)
+            os.remove(tarball_path)
+            message.ack()
+            logger.info('Done!')
+
+
+def run_adt(arguments):
+    """Run adt-run according to the given request parameters.
+
+    Always resets the environment with configured nova variables.
+
+    Returns the exit code from adt-run.
+    """
+    # TODO: We probably want something more clever here:
+    try:
+        subprocess.check_call(['adt-run'] + arguments)
+    except subprocess.CalledProcessError as e:
+        # log?
+        # TODO: filter log content to avoid leaking cloud credentials.
+        return e.returncode
+    return 0
+
+
+def _make_adt_argument_list(request_configuration):
+    """Generate the argument list to adt-run, given the
+    requestion_configuration dictionary.
+
+    :returns: A tuple of arguments.
+    """
+    adt_run_args = [
+        '--apt-source', request_configuration['package_name'],
+        '--output-dir', request_configuration['result_dir'],
+    ]
+    if 'apt_pocket' in request_configuration:
+        adt_run_args += [
+            '--apt-pocket', request_configuration['apt_pocket'],
+            '--apt-upgrade',
+    ]
+    adt_ssh_nova_args = [
+        '--',
+        '--flavor', request_configuration['nova_flavor'],
+        '--image', request_configuration['nova_image'],
+    ]
+    if 'nova_extra_args' in request_configuration:
+        adt_ssh_nova_args += request_configuration['nova_extra_args']
+
+
+    return (
+        adt_run_args +
+        ['---', 'ssh', '-s', 'nova'] +
+        adt_ssh_nova_args
+    )
+
+
+def _set_nova_environment_variables(nova_config):
+    """Set environment variables for nova configuration.
+
+    'nova_config' will be read, and any keys that start with 'os_' will be
+    set as environment variables.
+
+    'nova_config' must be an iterable of (key, value) pairs.
+
+    """
+    # Setup nova environment variables based on the configuration file.
+    for k, v in nova_config:
+        if not k.startswith('os_'):
+            continue
+        os.environ[k.upper()] = str(v)
+
+
+def main():
+    setup_logging(loglevel='DEBUG', loggers=[''])
+
+    parser = argparse.ArgumentParser(
+        description='ADT cloud worker ...')
+    parser.add_argument('-c', '--conf', default='.adt-service.conf',
+                        help='Configuration file path')
+    args = parser.parse_args()
+
+    # Load configuration options.
+    config = configparser.ConfigParser()
+    config.read(args.conf)
+    amqp_uris = config.get('amqp', 'uris').split()
+
+    with kombu.Connection(amqp_uris) as conn:
+        try:
+            worker = AdtNovaWorker(conn, config)
+            worker.run()
+        except KeyboardInterrupt:
+            print('Bye!')
+
+
+if __name__ == '__main__':
+    main()

=== added directory 'adt_cloud_worker/tests'
=== added file 'adt_cloud_worker/tests/__init__.py'
=== added file 'adt_cloud_worker/tests/test_cloud_worker.py'
--- adt_cloud_worker/tests/test_cloud_worker.py	1970-01-01 00:00:00 +0000
+++ adt_cloud_worker/tests/test_cloud_worker.py	2015-03-04 20:24:52 +0000
@@ -0,0 +1,122 @@
+# adt-cloud-worker
+# Copyright (C) 2015 Canonical
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+from fixtures import Fixture
+import testtools
+
+from adt_cloud_worker import (
+    _make_adt_argument_list,
+    _set_nova_environment_variables,
+)
+
+class TestADTArgumentGeneration(testtools.TestCase):
+
+    def createRequiredAdtRequestArgumets(self, **kwargs):
+        """Generate a dict that contains all the required parts of the
+        adt-cloud-worker request.
+
+        You may pass in additional arguments to customise the contents of the
+        dictionary.
+        """
+        defaults = {
+            'package_name': "some_package_name",
+            'result_dir': '/tmp/some/dir',
+            'nova_flavor': 'some-nova-flavour',
+            'nova_image': 'some-nova-image',
+        }
+        defaults.update(kwargs)
+        return defaults
+
+    def test_required_arguments(self):
+        observed = _make_adt_argument_list(
+            self.createRequiredAdtRequestArgumets()
+        )
+        self.assertEqual(
+            [
+                '--apt-source', 'some_package_name',
+                '--output-dir', '/tmp/some/dir',
+                '---',
+                'ssh',
+                '-s', 'nova',
+                '--',
+                '--flavor', 'some-nova-flavour',
+                '--image', 'some-nova-image',
+            ],
+            observed)
+
+    def test_includes_pocket_args(self):
+        observed = _make_adt_argument_list(
+            self.createRequiredAdtRequestArgumets(apt_pocket='foo')
+        )
+        self.assertEqual(
+            [
+                '--apt-source', 'some_package_name',
+                '--output-dir', '/tmp/some/dir',
+                '--apt-pocket', 'foo', '--apt-upgrade',
+                '---',
+                'ssh',
+                '-s', 'nova',
+                '--',
+                '--flavor', 'some-nova-flavour',
+                '--image', 'some-nova-image',
+            ],
+            observed)
+
+
+class RestoreEnvironmentVariableFixture(Fixture):
+
+    def __init__(self, env_var_name):
+        self.name = env_var_name
+
+    def setUp(self):
+        # TODO: Make this python3-ised
+        super(RestoreEnvironmentVariableFixture, self).setUp()
+        old_value = os.environ.get(self.name)
+        self.addCleanup(self._restore_value,  old_value)
+
+    def _restore_value(self, value):
+        if value is None:
+            try:
+                del os.environ[self.name]
+            except KeyError:
+                # key was never set, nbothing to do:
+                pass
+        else:
+            os.environ[self.name] = value
+
+
+class NovaEnvironmentVariableTests(testtools.TestCase):
+
+    def test_does_nothing_with_empty_dict(self):
+        _set_nova_environment_variables({})
+
+    def test_sets__os_keys(self):
+        with RestoreEnvironmentVariableFixture('OS_FOO'):
+            _set_nova_environment_variables([('os_foo', 'bar')])
+            self.assertEqual('bar', os.getenv('OS_FOO'))
+
+    def test_ignores_non_os_keys(self):
+        with RestoreEnvironmentVariableFixture('FOO'):
+            os.environ['FOO'] = 'bar'
+            _set_nova_environment_variables([('foo', 'baz')])
+            self.assertEqual('bar', os.getenv('FOO'))
+
+    def test_stringifies_values(self):
+        with RestoreEnvironmentVariableFixture('OS_FOO'):
+            _set_nova_environment_variables([('os_foo', 123)])
+            self.assertEqual('123', os.getenv('OS_FOO'))

=== modified file 'setup.py'
--- setup.py	2015-03-03 00:30:40 +0000
+++ setup.py	2015-03-04 20:24:52 +0000
@@ -37,6 +37,6 @@
     url='https://launchpad.net/adt-cloud-worker',
     license='GPLv3',
     packages=find_packages(),
-    # test_suite='autopilot.tests',
+    test_suite='adt_cloud_worker.tests',
     scripts=['adt-cloud-worker.py']
 )