Merge lp:~doanac/utah/run-cleanups into lp:utah

[Andy Doan]

this cleans up the _run function of run.py into something easier to manage.

I've done testing on acer-veriton-04 and vm's at home.

[Max Brustkern (nuclearbob)]

I'm wondering if any of what's currently in the various scripts should be in run.py instead?

I say that because I just started working on a branch to eliminate all but one of those. Maybe we should try to land this first, and then move any extra functionality into run.py. I don't like that we're passing args in; I feel like there should be some more specific and defined thing that gets handed to run.py, and all the actual command line handling should be maybe in the script. Either that or basically everything should be in run.py, and the script should just be handling picking an inventory and getting a machine? Not sure of the best approach for that, and we can just leave it as is for now if nothing else is compelling.

[Andy Doan (doanac)]

On 04/19/2013 02:48 PM, Max Brustkern wrote:
> I'm wondering if any of what's currently in the various scripts
> should be in run.py instead?

probably

> I say that because I just started working on a branch to eliminate
> all but one of those. Maybe we should try to land this first, and
> then move any extra functionality into run.py.

+1

> I don't like that
> we're passing args in; I feel like there should be some more specific
> and defined thing that gets handed to run.py, and all the actual
> command line handling should be maybe in the script.

I agree. Although there are soooooo many options we might wind up
passing a new object that looks just like "args". I wasn't sure if that
type of cleanup made sense in the scope of these changes or not. What do
you think?

> Either that or
> basically everything should be in run.py, and the script should just
> be handling picking an inventory and getting a machine? Not sure of
> the best approach for that, and we can just leave it as is for now if
> nothing else is compelling.
>

[Max Brustkern (nuclearbob)]

I think this is a good first step, and actually, I can probably go ahead and collapse the scripts into one in parallel with this. After that, we'll having one script calling functions from one file, and it'll maybe be easier to sort out which should be where from a design perspective rather than the current method of convenience and accretion.

[Javier Collado (javier.collado)]

The code looks good. A few comments regarding the test cases:

- tests/test_run.py

  I think `MockMachine` isn't really needed. In particular, I think the
  `downloadfiles` method shouldn't copy any file. Please, see below for more
  about this.

  - _files_equal:
    As explained above, I think files shouldn't be copied. Anyway, it that part
    isn't removed, it probably makes sense to take a look at `filecmp.cmp` in
    the standard library.

  - test__write:
    What about checking just that `machine.downloadfiles` has been called?
    Otherwise, it looks like isn't just the `_write` function what is being
    tested.

  - test_copy_preseed:
    For the invalid logpath part, it would be nice to check if
    `logging.warning` has been called.

    For the file being copied, making sure that `shutil.copyfile` has been
    called would be just fine.

[lp:~doanac/utah/run-cleanups]

876. By Andy Doan on 2013-04-22

clean up test_run as per javier's suggestions

[Andy Doan (doanac)]

I'm not that good with Mock, but I *think* this is what you were suggesting.

[Javier Collado (javier.collado)]

The update looks good to me.

A few more comments:

- All the test cases in `test_run.py` are inside the
  `TestMasterRunlistArgument` class. I think that they should be splitted into
  different classes to make sure it's clear what's the purpose of each set of
  test cases.

- There's no need to call configure mock for just one attribute, it can set
  directly:

   machine.configure_mock(name='name') => machine.name = 'name'

- If you think it's valuable you can get the arguments that were used to call
  the mock objects to test more specific things like:
  - machine.donwload_files was called with the same path that was added to logs
  - logging.warning message matches the expected pattern (i.e. failed to copy
    preseed file)

  I've pasted a diff with some of those things here just in case:
  http://pastebin.ubuntu.com/5595348/

[lp:~doanac/utah/run-cleanups]

877. By Andy Doan on 2013-04-23

patch from javier

plus I broke up my new tests into its own class TestRun

[Andy Doan (doanac)]

try it out now. I took your patch, but also added a new class as you suggested.

[Javier Collado (javier.collado)]

Looks good now. Thanks for the update.

review: Approve

=== modified file 'tests/test_run.py'
--- tests/test_run.py	2013-04-16 15:22:20 +0000
+++ tests/test_run.py	2013-04-23 17:20:34 +0000
@@ -15,22 +15,45 @@
 
 """Test command line argument functions."""
 
+import shutil
+import tempfile
 import unittest
+import urllib
 
 from argparse import ArgumentTypeError
 from jsonschema import ValidationError
-from mock import patch
+from mock import patch, Mock
 
+from utah import config
 from utah.client.exceptions import (
     YAMLEmptyFile,
     YAMLParsingError,
 )
-from utah.run import master_runlist_argument
+from utah.exceptions import (
+    UTAHException,
+)
+from utah.run import (
+    _copy_preseed,
+    _get_runlist,
+    _write,
+    master_runlist_argument,
+)
 
 
 class TestMasterRunlistArgument(unittest.TestCase):
+
     """Test master_runlist_argument function."""
 
+    def setUp(self):
+        """Peform temporary setup required by tests."""
+        self.logpath = config.logpath
+        config.logpath = tempfile.mkdtemp(prefix='utah_run')
+
+    def tearDown(self):
+        """Undo temporary stuff from setUp."""
+        shutil.rmtree(config.logpath)
+        config.logpath = self.logpath
+
     @patch('utah.run.parse_yaml_file')
     @patch('utah.run.url_argument')
     def test_empty_file(self, url_argument, parse_yaml_file):
@@ -68,3 +91,65 @@
         url_argument.side_effect = [expected_filename]
         filename = master_runlist_argument('url')
         self.assertEqual(filename, expected_filename)
+
+
+class TestRun(unittest.TestCase):
+
+    """Unit tests on utility functions in run.py."""
+
+    @patch('urllib.urlretrieve')
+    def test_get_runlist(self, urlretrieve):
+        """Ensure _get_runlist behaves properly on errors."""
+
+        def _error(url):
+            # the "url" passed is the exception class we want to throw
+            raise url('blah', None)
+
+        urlretrieve.side_effect = _error
+        pattern = 'IOError when downloading'
+        self.assertRaisesRegexp(UTAHException, pattern, _get_runlist, IOError)
+
+        pattern = 'Error when downloading'
+        self.assertRaisesRegexp(
+            UTAHException, pattern, _get_runlist, urllib.ContentTooShortError)
+
+    def test__write(self):
+        """Ensure proper behavior of the _write function."""
+        logs = []
+        args = Mock()
+        args.configure_mock(
+            report_type='yaml', runlist='tmp.txt', dumplogs=True)
+        machine = Mock()
+        machine.name = 'name'
+        report_path = '/path/to/report'
+        _write(machine, args, logs, report_path)
+        self.assertTrue(machine.downloadfiles.called)
+        call_args, _call_kwargs = machine.downloadfiles.call_args
+        self.assertListEqual([call_args[1]], logs)
+        self.assertEqual(call_args[0], report_path)
+
+    @patch('logging.warning')
+    def test_copy_preseed_bad(self, warning):
+        """Ensure _copy_preseed works for bad data."""
+        logs = []
+        args = Mock()
+        args.outputpreseed = True
+
+        machine = Mock()
+        machine.configure_mock(name='name', finalpreseed='/bad file name')
+        _copy_preseed(machine, machine, logs)
+        self.assertEqual(0, len(logs))
+        self.assertTrue(warning.called)
+        call_args, _call_kwargs = warning.call_args
+        self.assertRegexpMatches(call_args[0],
+                                 r'Failed to copy preseed file: %s')
+
+    @patch('shutil.copyfile')
+    def test_copy_preseed_good(self, copyfile):
+        """Ensure _copy_preseed works for good data."""
+        logs = []
+        machine = Mock()
+        machine.configure_mock(name='name', finalpreseed='/bin/true')
+        _copy_preseed(machine, machine, logs)
+        self.assertEqual(1, len(logs))
+        self.assertTrue(copyfile.called)

=== modified file 'utah/run.py'
--- utah/run.py	2013-04-18 14:11:57 +0000
+++ utah/run.py	2013-04-23 17:20:34 +0000
@@ -238,6 +238,114 @@
     return parser
 
 
+def _get_runlist(url):
+    try:
+        return urllib.urlretrieve(url)[0]
+    except urllib.ContentTooShortError as e:
+        err = ('Error when downloading {} (probably interrupted): {}'
+               .format(url, e))
+    except IOError as e:
+        err = 'IOError when downloading {}: {}'.format(url, e)
+
+    raise UTAHException(err, external=True)
+
+
+def _run(machine, runlist, extraopts):
+    """Execute a single runlist on the machine.
+
+    :returns: a tuple (exit_status, report_remote_path)
+
+    """
+    try:
+        runlist_local_path = _get_runlist(runlist)
+        target_dir = '/tmp'
+        machine.uploadfiles([runlist_local_path], target_dir)
+
+        runlist_file = os.path.basename(runlist_local_path)
+        runlist_remote_path = os.path.join(target_dir, runlist_file)
+        report_remote_path = '/var/lib/utah/utah.out'
+
+        options = '-r {} -o {}'.format(runlist_remote_path, report_remote_path)
+        utah_command = 'utah {} {}'.format(extraopts, options)
+
+        exitstatus, _stdout, _stderr = machine.run(utah_command, root=True)
+        # Check state file to make sure client finished
+        # when some reboot commands where executed during the test run
+        if exitstatus == ClientReturnCodes.REBOOT:
+            logging.info('System under test is rebooting')
+            exitstatus = timeout(config.runtimeout, retry,
+                                 is_utah_done, machine, 180,
+                                 logmethod=logging.info)
+        # Make sure that it's possible to know when the server failed
+        # and when the client failed just checking the return code
+        if exitstatus != 0:
+            exitstatus = ReturnCodes.client_error(exitstatus)
+    except UTAHException as error:
+        logging.error('Failed to run test: %s', str(error))
+        exitstatus = ReturnCodes.UTAH_EXCEPTION_ERROR
+
+    return exitstatus, report_remote_path
+
+
+def _write(machine, args, locallogs, report_remote_path):
+    """Write runlist report.
+
+    Report will be written using appropriate filename based on the runlist.
+
+    :param machine: Path to the runlist file
+    :param args: options from argparse
+    :param locallogs: array of files resulting from this job
+    :param report_remote_path:
+        Path to the report file in the machine being tested
+
+    """
+    timestamp = time.strftime('%Y-%m-%d_%H-%m-%S', time.gmtime())
+
+    listname = os.path.basename(args.runlist)
+    log_filename = ('{machine}_{runlist}_{timestamp}.{suffix}'
+                    .format(machine=machine.name,
+                            runlist=listname,
+                            timestamp=timestamp,
+                            suffix=args.report_type))
+    report_local_path = os.path.join(config.logpath, log_filename)
+    report_local_path = os.path.normpath(report_local_path)
+    try:
+        machine.downloadfiles(report_remote_path, report_local_path)
+    except UTAHException:
+        logging.error('Test log not retrieved')
+    else:
+        locallogs.append(report_local_path)
+        logging.info('Test log copied to %s', report_local_path)
+        if args.dumplogs:
+            try:
+                with open(report_local_path, 'r') as f:
+                    print(f.read())
+            except IOError as err:
+                logging.warning('Test log could not be dumped: {}'
+                                .format(err))
+
+
+def _copy_preseed(machine, args, locallogs):
+    """Copy preseed to locallogs.
+
+    If we are provisioning a system, we can optionally copy its preseed along
+    with other locallogs from the job. Systems already provisioned will not
+
+    """
+    if (not isinstance(machine, ProvisionedMachine) and
+            (args.outputpreseed or config.outputpreseed)):
+        if args.outputpreseed:
+            logging.debug('Capturing preseed due to command line option')
+
+        p = os.path.join(config.logpath, '{}-preseed.cfg'.format(machine.name))
+        try:
+            shutil.copyfile(machine.finalpreseed, p)
+        except (IOError, shutil.Error) as err:
+            logging.warning('Failed to copy preseed file: %s', err)
+        else:
+            locallogs.append(p)
+
+
 def run_tests(args, machine):
     """Run a runlist and retrieve results.
 
@@ -247,104 +355,23 @@
     """
     install_sigterm_handler()
 
+    args.report_type = 'yaml'
     if args.json:
-        report_type = 'json'
-    else:
-        report_type = 'yaml'
-
-    def _run(runlist):
-        """Run a single runlist."""
-        try:
-            try:
-                runlist_local_path = urllib.urlretrieve(runlist)[0]
-            except IOError as err:
-                raise UTAHException(
-                    'IOError when downloading {}: {}'
-                    .format(runlist, err), external=True)
-            except urllib.ContentTooShortError as err:
-                raise UTAHException(
-                    'Error when downloading {} (probably interrupted): {}'
-                    .format(runlist, err), external=True)
-            target_dir = os.path.normpath('/tmp')
-            machine.uploadfiles([runlist_local_path], target_dir)
-
-            runlist_file = os.path.basename(runlist_local_path)
-            runlist_remote_path = os.path.join(target_dir, runlist_file)
-            report_remote_path = os.path.join('/', 'var', 'lib', 'utah',
-                                              'utah.out')
-            options = '-r {} -o {}'.format(runlist_remote_path,
-                                           report_remote_path)
-            utah_command = 'utah {} {}'.format(extraopts, options)
-
-            exitstatus, _stdout, _stderr = machine.run(utah_command, root=True)
-            # Check state file to make sure client finished
-            # when some reboot commands where executed during the test run
-            if exitstatus == ClientReturnCodes.REBOOT:
-                logging.info('System under test is rebooting')
-                exitstatus = timeout(config.runtimeout, retry,
-                                     is_utah_done, machine, 180,
-                                     logmethod=logging.info)
-            # Make sure that it's possible to know when the server failed
-            # and when the client failed just checking the return code
-            if exitstatus != 0:
-                exitstatus = ReturnCodes.client_error(exitstatus)
-        except UTAHException as error:
-            logging.error('Failed to run test: %s', str(error))
-            exitstatus = ReturnCodes.UTAH_EXCEPTION_ERROR
-        else:
-            _write(runlist, report_remote_path)
-
-        return exitstatus
-
-    def _write(runlist, report_remote_path):
-        """Write runlist report.
-
-        Report will be written using appropriate filename based on the runlist.
-
-        :param runlist: Path to the runlist file
-        :type runlist: str
-        :param report_remote_path:
-            Path to the report file in the machine being tested
-        :type report: str
-
-        """
-        timestamp = time.strftime('%Y-%m-%d_%H-%m-%S', time.gmtime())
-
-        listname = os.path.basename(runlist)
-        log_filename = ('{machine}_{runlist}_{timestamp}.{suffix}'
-                        .format(machine=machine.name,
-                                runlist=listname,
-                                timestamp=timestamp,
-                                suffix=report_type))
-        report_local_path = os.path.join(config.logpath, log_filename)
-        report_local_path = os.path.normpath(report_local_path)
-        try:
-            machine.downloadfiles(report_remote_path, report_local_path)
-        except UTAHException:
-            logging.error('Test log not retrieved')
-        else:
-            logging.info('Test log copied to %s', report_local_path)
-            locallogs.append(report_local_path)
-            if args.dumplogs:
-                try:
-                    with open(report_local_path, 'r') as f:
-                        print(f.read())
-                except IOError as err:
-                    logging.warning('Test log could not be dumped: {}'
-                                    .format(err))
+        args.report_type = 'json'
 
     # Write the machine name to standard out for log gathering
     print('Running on machine: {}'.format(machine.name))
 
     extraopts = ('-f {} --install-type {}'
-                 .format(report_type, machine.installtype))
+                 .format(args.report_type, machine.installtype))
 
     locallogs = []
     machine.installclient()
 
     # Server will return success code only if the execution
     # of every runlist was successful
-    exitstatus = _run(args.runlist)
+    exitstatus, remote_path = _run(machine, args.runlist, extraopts)
+    _write(machine, args, locallogs, remote_path)
 
     if args.files is not None:
         try:
@@ -352,23 +379,7 @@
         except UTAHException as err:
             logging.warning('Failed to download files: %s', str(err))
 
-    # Provisioned systems have an image already installed
-    # and the preseed file is no longer available
-    if (not isinstance(machine, ProvisionedMachine) and
-            (args.outputpreseed or config.outputpreseed)):
-        if args.outputpreseed:
-            logging.debug('Capturing preseed due to command line option')
-        elif config.outputpreseed:
-            logging.debug('Capturing preseed due to config option')
-        preseedfile = os.path.join(config.logpath,
-                                   '{}-preseed.cfg'.format(machine.name))
-        try:
-            shutil.copyfile(machine.finalpreseed, preseedfile)
-        except (IOError, shutil.Error) as err:
-            logging.warning('Failed to copy preseed file: {}'
-                            .format(err))
-        else:
-            locallogs.append(preseedfile)
+    _copy_preseed(machine, args, locallogs)
 
     return exitstatus, locallogs