Merge lp:~jtv/maas/bug-1324237 into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Report stderr output from failing commands as part of the exception, so that we can see what went wrong. Fold call_capture_and_check into call_and_check, since they do the same thing except that one returns meaningful data.

There really was no need for call_capture_and_check: it did the exact same thing as call_and_check, except it returned standard output where call_and_check returned the command's return code. Both raise ExternalProcessError if the return code is nonzero, so there was no point in returning it. Just return stdout and now we have only one function.

In one place, when verifying GPG signatures for downloaded boot loaders, we passed stderr=subprocess.STDOUT. I'm not sure if that actually went anywhere. At any rate it should no longer be needed: a traceback will now show the error text.

Jeroen

[Julian Edwards (julian-edwards)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

 review: approve

Tip top.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlOH5RUACgkQWhGlTF8G/HcPxgCfaZoiHpq9rsdRdzL2enKjRr4s
QGoAn3ei7ULYIgqgRJHWyXHwmA3IHOqz
=cKta
-----END PGP SIGNATURE-----

review: Approve

=== modified file 'src/provisioningserver/boot/utils.py'
--- src/provisioningserver/boot/utils.py	2014-03-28 04:06:27 +0000
+++ src/provisioningserver/boot/utils.py	2014-05-29 23:37:13 +0000
@@ -24,11 +24,10 @@
 from platform import linux_distribution
 import re
 import StringIO
-import subprocess
 import urllib2
 
 from provisioningserver.utils import (
-    call_capture_and_check,
+    call_and_check,
     tempdir,
     )
 
@@ -71,14 +70,13 @@
         with open(data_out, 'wb') as stream:
             stream.write(data_file)
 
-        args = [
+        call_and_check([
             "gpgv",
             "--keyring",
             "/etc/apt/trusted.gpg",
             sig_out,
             data_out
-            ]
-        call_capture_and_check(args, stderr=subprocess.STDOUT)
+            ])
 
 
 def decompress_packages(packages):

=== modified file 'src/provisioningserver/dns/config.py'
--- src/provisioningserver/dns/config.py	2014-02-14 10:47:19 +0000
+++ src/provisioningserver/dns/config.py	2014-05-29 23:37:13 +0000
@@ -39,7 +39,6 @@
 from provisioningserver.utils import (
     atomic_write,
     call_and_check,
-    call_capture_and_check,
     incremental_write,
     locate_config,
     )
@@ -102,7 +101,7 @@
     # Generate the configuration:
     # - 256 bits is the recommended size for the key nowadays.
     # - Use urandom to avoid blocking on the random generator.
-    rndc_content = call_capture_and_check(
+    rndc_content = call_and_check(
         ['rndc-confgen', '-b', '256', '-r', '/dev/urandom',
          '-k', key_name, '-p', unicode(port).encode("ascii")])
     named_comment = extract_suggested_named_conf(rndc_content)
@@ -149,8 +148,7 @@
     rndc_conf = get_rndc_conf_path()
     rndc_cmd = ['rndc', '-c', rndc_conf]
     rndc_cmd.extend(arguments)
-    with open(os.devnull, "ab") as devnull:
-        call_and_check(rndc_cmd, stdout=devnull)
+    call_and_check(rndc_cmd)
 
 
 # Location of DNS templates, relative to the configuration directory.

=== modified file 'src/provisioningserver/import_images/tests/test_boot_resources.py'
--- src/provisioningserver/import_images/tests/test_boot_resources.py	2014-05-27 08:21:33 +0000
+++ src/provisioningserver/import_images/tests/test_boot_resources.py	2014-05-29 23:37:13 +0000
@@ -272,7 +272,7 @@
         # at a low level, so that we exercise all the function calls that a
         # unit test might not put to the test.
         self.patch_logger()
-        self.patch(boot_resources, 'call_and_check').return_code = 0
+        self.patch(boot_resources, 'call_and_check')
 
         # We'll go through installation of a PXE boot loader here, but skip
         # all other boot loader types.  Testing them all is a job for proper

=== modified file 'src/provisioningserver/omshell.py'
--- src/provisioningserver/omshell.py	2014-02-27 03:30:05 +0000
+++ src/provisioningserver/omshell.py	2014-05-29 23:37:13 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Python wrapper around the `omshell` utility which amends objects
@@ -28,7 +28,7 @@
 from textwrap import dedent
 
 from provisioningserver.utils import (
-    call_capture_and_check,
+    call_and_check,
     ExternalProcessError,
     parse_key_value_file,
     tempdir,
@@ -42,7 +42,7 @@
     path = os.environ.get("PATH", "").split(os.pathsep)
     path.append("/usr/sbin")
     env = dict(os.environ, PATH=os.pathsep.join(path))
-    return call_capture_and_check(
+    return call_and_check(
         ['dnssec-keygen', '-r', '/dev/urandom', '-a', 'HMAC-MD5',
          '-b', '512', '-n', 'HOST', '-K', tmpdir, '-q', 'omapi_key'],
         env=env)

=== modified file 'src/provisioningserver/tests/test_omshell.py'
--- src/provisioningserver/tests/test_omshell.py	2014-02-27 03:29:12 +0000
+++ src/provisioningserver/tests/test_omshell.py	2014-05-29 23:37:13 +0000
@@ -271,12 +271,13 @@
     """Tests for omshell.call_dnssec_keygen."""
 
     def test_runs_external_script(self):
-        check_output = self.patch(subprocess, 'check_output')
+        call_and_check = self.patch(
+            provisioningserver.omshell, 'call_and_check')
         target_dir = self.make_dir()
         path = os.environ.get("PATH", "").split(os.pathsep)
         path.append("/usr/sbin")
         call_dnssec_keygen(target_dir)
-        check_output.assert_called_once_with(
+        call_and_check.assert_called_once_with(
             ['dnssec-keygen', '-r', '/dev/urandom', '-a', 'HMAC-MD5',
              '-b', '512', '-n', 'HOST', '-K', target_dir, '-q', 'omapi_key'],
             env=ANY)

=== modified file 'src/provisioningserver/utils/__init__.py'
--- src/provisioningserver/utils/__init__.py	2014-05-05 23:55:46 +0000
+++ src/provisioningserver/utils/__init__.py	2014-05-29 23:37:13 +0000
@@ -15,14 +15,15 @@
 __all__ = [
     "ActionScript",
     "atomic_write",
+    "call_and_check",
     "deferred",
+    "ensure_dir",
     "filter_dict",
     "find_ip_via_arp",
     "import_settings",
     "incremental_write",
     "locate_config",
     "MainScript",
-    "ensure_dir",
     "parse_key_value_file",
     "read_text_file",
     "ShellTemplate",
@@ -115,25 +116,18 @@
 def call_and_check(command, *args, **kwargs):
     """A wrapper around subprocess.check_call().
 
-    When an error occurs, raise an ExternalProcessError.
-    """
-    try:
-        return subprocess.check_call(command, *args, **kwargs)
-    except subprocess.CalledProcessError as error:
-        error.__class__ = ExternalProcessError
-        raise
-
-
-def call_capture_and_check(command, *args, **kwargs):
-    """A wrapper around subprocess.check_output().
-
-    When an error occurs, raise an ExternalProcessError.
-    """
-    try:
-        return subprocess.check_output(command, *args, **kwargs)
-    except subprocess.CalledProcessError as error:
-        error.__class__ = ExternalProcessError
-        raise
+    :param command: Command line, as a list of strings.
+    :return: The command's output from standard output.
+    :raise ExternalProcessError: If the command returns nonzero.
+    """
+    process = subprocess.Popen(
+        command, *args, stdout=subprocess.PIPE, stderr=subprocess.PIPE,
+        **kwargs)
+    stdout, stderr = process.communicate()
+    stderr = stderr.strip()
+    if process.returncode != 0:
+        raise ExternalProcessError(process.returncode, command, output=stderr)
+    return stdout
 
 
 def locate_config(*path):
@@ -822,7 +816,7 @@
     :param mac: The mac address, e.g. '1c:6f:65:d5:56:98'.
     """
 
-    output = call_capture_and_check(['arp', '-n']).split('\n')
+    output = call_and_check(['arp', '-n']).split('\n')
 
     for line in sorted(output):
         columns = line.split()

=== modified file 'src/provisioningserver/utils/tests/test_utils.py'
--- src/provisioningserver/utils/tests/test_utils.py	2014-05-19 12:40:56 +0000
+++ src/provisioningserver/utils/tests/test_utils.py	2014-05-29 23:37:13 +0000
@@ -68,7 +68,6 @@
     atomic_write,
     AtomicWriteScript,
     call_and_check,
-    call_capture_and_check,
     classify,
     ensure_dir,
     escape_py_literal,
@@ -1204,33 +1203,39 @@
             classify(is_even, subjects))
 
 
-class TestSubprocessWrappers(MAASTestCase):
-    """Tests for the subprocess.* wrapper functions."""
-
-    def test_call_and_check_returns_returncode(self):
-        self.patch(subprocess, 'check_call', FakeMethod(0))
-        self.assertEqual(0, call_and_check('some_command'))
-
-    def test_call_and_check_raises_ExternalProcessError_on_failure(self):
-        self.patch(subprocess, 'check_call').side_effect = (
-            CalledProcessError('-1', 'some_command'))
-        error = self.assertRaises(
-            ExternalProcessError, call_and_check, "some command")
-        self.assertEqual('-1', error.returncode)
-        self.assertEqual('some_command', error.cmd)
-
-    def test_call_capture_and_check_returns_returncode(self):
-        self.patch(subprocess, 'check_output', FakeMethod("Some output"))
-        self.assertEqual("Some output", call_capture_and_check('some_command'))
-
-    def test_call_capture_and_check_raises_ExternalProcessError_on_fail(self):
-        self.patch(subprocess, 'check_output').side_effect = (
-            CalledProcessError('-1', 'some_command', "Some output"))
-        error = self.assertRaises(
-            ExternalProcessError, call_capture_and_check, "some command")
-        self.assertEqual('-1', error.returncode)
-        self.assertEqual('some_command', error.cmd)
-        self.assertEqual("Some output", error.output)
+class TestCallAndCheck(MAASTestCase):
+    """Tests `call_and_check`."""
+
+    def patch_popen(self, returncode=0, stderr=''):
+        """Replace `subprocess.Popen` with a mock."""
+        popen = self.patch(subprocess, 'Popen')
+        process = popen.return_value
+        process.communicate.return_value = (None, stderr)
+        process.returncode = returncode
+        return process
+
+    def test__returns_standard_output(self):
+        output = factory.getRandomString()
+        self.assertEqual(output, call_and_check(['/bin/echo', '-n', output]))
+
+    def test__raises_ExternalProcessError_on_failure(self):
+        command = factory.make_name('command')
+        message = factory.getRandomString()
+        self.patch_popen(returncode=1, stderr=message)
+        error = self.assertRaises(
+            ExternalProcessError, call_and_check, command)
+        self.assertEqual(1, error.returncode)
+        self.assertEqual(command, error.cmd)
+        self.assertEqual(message, error.output)
+
+    def test__reports_stderr_on_failure(self):
+        nonfile = os.path.join(self.make_dir(), factory.make_name('nonesuch'))
+        error = self.assertRaises(
+            ExternalProcessError,
+            call_and_check, ['/bin/cat', nonfile], env={'LC_ALL': 'C'})
+        self.assertEqual(
+            "/bin/cat: %s: No such file or directory" % nonfile,
+            error.output)
 
 
 class TestExternalProcessError(MAASTestCase):
@@ -1303,8 +1308,8 @@
 class TestFindIPViaARP(MAASTestCase):
 
     def patch_call(self, output):
-        """Replace `call_capture_and_check` with one that returns `output`."""
-        fake = self.patch(provisioningserver.utils, 'call_capture_and_check')
+        """Replace `call_and_check` with one that returns `output`."""
+        fake = self.patch(provisioningserver.utils, 'call_and_check')
         fake.return_value = output
         return fake
 
@@ -1321,11 +1326,9 @@
         192.168.0.1 ether   90:f6:52:f6:17:92   C                     eth0
         """
 
-        call_capture_and_check = self.patch_call(sample)
+        call_and_check = self.patch_call(sample)
         ip_address_observed = find_ip_via_arp("90:f6:52:f6:17:92")
-        self.assertThat(
-            call_capture_and_check,
-            MockCalledOnceWith(['arp', '-n']))
+        self.assertThat(call_and_check, MockCalledOnceWith(['arp', '-n']))
         self.assertEqual("192.168.0.1", ip_address_observed)
 
     def test__returns_consistent_output(self):