MAAS

Merge lp:~gmb/maas/fix-bug-1184589 into lp:~maas-committers/maas/trunk

  1. fix-bug-1184589
  2. Merge into trunk
Proposed by Graham Binns
Status: Merged
Approved by: Graham Binns
Approved revision: no longer in the source branch.
Merged at revision: 1705
Proposed branch: lp:~gmb/maas/fix-bug-1184589
Merge into: lp:~maas-committers/maas/trunk
Diff against target: 719 lines (+214/-56)
16 files modified
src/maasserver/models/tests/test_nodegroup.py (+1/-1)
src/maasserver/tests/test_api_nodegroup.py (+1/-1)
src/maasserver/tests/test_dhcp.py (+1/-1)
src/provisioningserver/dns/config.py (+4/-6)
src/provisioningserver/dns/tests/test_config.py (+6/-2)
src/provisioningserver/import_images/config.py (+5/-3)
src/provisioningserver/import_images/ephemerals_script.py (+3/-3)
src/provisioningserver/import_images/tests/test_config.py (+2/-2)
src/provisioningserver/import_images/tests/test_ephemerals_script.py (+7/-6)
src/provisioningserver/import_images/tgt.py (+5/-4)
src/provisioningserver/omshell.py (+6/-6)
src/provisioningserver/tasks.py (+8/-7)
src/provisioningserver/tests/test_omshell.py (+21/-4)
src/provisioningserver/tests/test_tasks.py (+9/-9)
src/provisioningserver/tests/test_utils.py (+70/-0)
src/provisioningserver/utils.py (+65/-1)
To merge this branch: bzr merge lp:~gmb/maas/fix-bug-1184589
Reviewer Review Type Date Requested Status
Gavin Panella (community) Approve
Review via email: mp+190601@code.launchpad.net

Commit message

Failing external commands run by the provisioning server will now raise errors which contain the output of the command that failed (if available).

Description of the change

This branch fixes bug 1184589, at least for the provisioning server...

The problem is that CalledProcessError doesn't include the output of the
command in its message. Sometimes it doesn't even include any details at
all, which I reckon is something to do with the fact that
CalledProcessError.__str__() is defined and __unicode__() isn't.

To solve this, I've done the following:

 - Created a subclass of CalledProcessError,
   provisioningserver.utils.ExternalActionError. This defines a
   __unicode__() method, which includes the command's output if it's
   available. ExternalActionError.__str__() is a wrapper around
   __unicode__().
 - Created two wrappers, wrapped_check_call() and wrapped_check_output()
   around subprocess.check_call() and .check_output() respectively.
   These catch any CalledProcessErrors in the subprocess commands,
   and create and raise ExternalActionErrors.
 - Updated all the callsites for check_call() and check_output() under
   provisioningserver to use the wrapped_* functions. We should now
   prefer using these.
 - Updated any `raise CalledProcessError(...)` sites under
   provisioningserver to raise ExternalActionError(...) instead.

I've added tests where appropriate.

One question here is whether this should be used outside the provisioning server code. There are relatively few instances of shelling out in other parts of the codebase AFAICT; a couple each in src/metadataserver, services/ and utilities/. I'll let someone with more knowledge than me answer that.

To post a comment you must log in.
Revision history for this message
Gavin Panella (allenap) wrote :
Download full text (5.2 KiB)

Tip top, looks good. There are some things that still need doing I think, but it's the right approach. But first, a Panella service announcement:

I guess there are two ways of addressing this bug:

1. Finding each place that reports CalledProcessError (explicitly or
   by catching a superclass), and update them to treat
   CalledProcessError specially, e.g. by logging e.output too.

2. Finding each place we use check_call() or check_output() and
   switching them to use wrapped_check_{call,output}(), and 'fixing'
   the exception to include the log output when it's dumped later on.

3. Push a change upstream into Python to fix CalledProcessError to be
   like #2.

That's 3 things.

Only #1 and #2 are feasible really, and you've chosen #2 (after asking
all of us, I might add). It's sad when we can't use the standard
library unadorned, but the str/unicode crap in Python 2.x kind of
forces our hand. It's easier to find uses of check_{call,output} in
the codebase than it is to find those points where exceptions leak out
into log files and suchlike, so #2 is the better approach, even though
it's less "pretty" than #1 to my mind.

This long ramble mainly for my own benefit is now over.

[1]

wrapped_check_call and wrapped_check_output are ugly names... but I
can't think of anything better. They talk about mechanism and
implementation instead of what we expect to gain from their use.

Maybe:

  check_call --> call_and_check
  check_output --> call_capture_and_check

?

[2]

I also have problems with ExternalActionError's name. Sorry :-/ This
is a hard problem. I think it's the "Action" bit; it sounds too
specific, and not Unix enough. How about "ExternalProcessError"?

[3]

+from provisioningserver.utils import (
+    read_text_file,
+    )

Try:

  utilities/format-new-and-modified-import -r submit:

and it'll unwrap that. It'll also fix imports in a few other modules
too.

[4]

+class TestCallDnsSecKeygen(MAASTestCase):
+    """Tests for omshell.call_dnssec_keygen."""
+
+    def test_runs_external_script(self):
+        check_output = self.patch(subprocess, 'check_output')
+        target_dir = self.make_dir()
+        path = os.environ.get("PATH", "").split(os.pathsep)
+        path.append("/usr/sbin")
+        env = dict(os.environ, PATH=os.pathsep.join(path))
+        call_dnssec_keygen(target_dir)
+        check_output.assert_called_with([
+            'dnssec-keygen', '-r', '/dev/urandom', '-a', 'HMAC-MD5',
+             '-b', '512', '-n', 'HOST', '-K', target_dir, '-q', 'omapi_key'
+             ], env=env)

Woah, where did that come from? :)

If it's only called once, you could do:

        check_output.assert_called_once_with([
            ...

Also, if the env isn't /that/ important here, you could do:

        from mock import ANY
        check_output.assert_called_with([
            'dnssec-keygen', '-r', '/dev/urandom', '-a', 'HMAC-MD5',
             '-b', '512', '-n', 'HOST', '-K', target_dir, '-q', 'omapi_key'
             ], env=ANY)

[5]

+        recorder = self.patch(tasks, 'wrapped_check_call', Mock())

Not your code, but you can remove the Mock() bit here; it's done
automatically by patch():

        recorder = self.patch...

Read more...

review: Needs Fixing
Revision history for this message
Graham Binns (gmb) wrote :
Download full text (6.1 KiB)

> Tip top, looks good. There are some things that still need doing I think, but
> it's the right approach. But first, a Panella service announcement:
>
> I guess there are two ways of addressing this bug:
>
> 1. Finding each place that reports CalledProcessError (explicitly or
>   by catching a superclass), and update them to treat
>   CalledProcessError specially, e.g. by logging e.output too.
>
> 2. Finding each place we use check_call() or check_output() and
>   switching them to use wrapped_check_{call,output}(), and 'fixing'
>   the exception to include the log output when it's dumped later on.
>
> 3. Push a change upstream into Python to fix CalledProcessError to be
>   like #2.
>
> That's 3 things.
>
> Only #1 and #2 are feasible really, and you've chosen #2 (after asking
> all of us, I might add). It's sad when we can't use the standard
> library unadorned, but the str/unicode crap in Python 2.x kind of
> forces our hand. It's easier to find uses of check_{call,output} in
> the codebase than it is to find those points where exceptions leak out
> into log files and suchlike, so #2 is the better approach, even though
> it's less "pretty" than #1 to my mind.
>
> This long ramble mainly for my own benefit is now over.

It's exactly the same long ramble I had to myself before asking all
y'all :).

> [1]
>
> wrapped_check_call and wrapped_check_output are ugly names... but I
> can't think of anything better. They talk about mechanism and
> implementation instead of what we expect to gain from their use.
>
> Maybe:
>
>  check_call --> call_and_check
>  check_output --> call_capture_and_check
>
> ?

These both seem sane to me. Done.

> [2]
>
> I also have problems with ExternalActionError's name. Sorry :-/ This
> is a hard problem. I think it's the "Action" bit; it sounds too
> specific, and not Unix enough. How about "ExternalProcessError"?

Yeah, I didn't like it much. I nicked action from PowerActionFail, but
that's specific to the context (which I didn't spot when I nicked it).
ExternalProcessError seems fair to me.

> [3]
>
> +from provisioningserver.utils import (
> +    read_text_file,
> +    )
>
> Try:
>
>  utilities/format-new-and-modified-import -r submit:
>
> and it'll unwrap that. It'll also fix imports in a few other modules
> too.

Sweet!

> [4]
>
> +class TestCallDnsSecKeygen(MAASTestCase):
> +    """Tests for omshell.call_dnssec_keygen."""
> +
> +    def test_runs_external_script(self):
> +        check_output = self.patch(subprocess, 'check_output')
> +        target_dir = self.make_dir()
> +        path = os.environ.get("PATH", "").split(os.pathsep)
> +        path.append("/usr/sbin")
> +        env = dict(os.environ, PATH=os.pathsep.join(path))
> +        call_dnssec_keygen(target_dir)
> +        check_output.assert_called_with([
> +            'dnssec-keygen', '-r', '/dev/urandom', '-a', 'HMAC-MD5',
> +             '-b', '512', '-n', 'HOST', '-K', target_dir, '-q', 'omapi_key'
> +             ], env=env)
>
> Woah, where did that come from? :)

Oh, this was from when I was going through and finding check_call
sites. I realised that this function wasn't tested (or rather it's never
tested that it ca...

Read more...

Revision history for this message
Gavin Panella (allenap) wrote :

Looks good! Couple more minor points.

[8]

TestExternalProcessError should probably exercise the _to_* functions
with non-ASCII characters.

[9]

+        self.assertTrue(isinstance(converted_string, unicode))

Here and in a few other places you could use self.assertIsInstance();
it'll give better output on failure.

review: Approve
Revision history for this message
MAAS Lander (maas-lander) wrote :
Download full text (23.7 KiB)

The attempt to merge lp:~gmb/maas/fix-bug-1184589 into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com saucy-security InRelease
Hit http://security.ubuntu.com saucy-security Release.gpg
Hit http://security.ubuntu.com saucy-security Release
Ign http://nova.clouds.archive.ubuntu.com saucy InRelease
Ign http://nova.clouds.archive.ubuntu.com saucy-updates InRelease
Get:1 http://nova.clouds.archive.ubuntu.com saucy Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com saucy-updates Release.gpg
Get:2 http://nova.clouds.archive.ubuntu.com saucy Release [49.6 kB]
Hit http://nova.clouds.archive.ubuntu.com saucy-updates Release
Hit http://security.ubuntu.com saucy-security/main Sources
Hit http://security.ubuntu.com saucy-security/universe Sources
Hit http://security.ubuntu.com saucy-security/main amd64 Packages
Hit http://security.ubuntu.com saucy-security/universe amd64 Packages
Hit http://security.ubuntu.com saucy-security/main Translation-en
Hit http://security.ubuntu.com saucy-security/universe Translation-en
Get:3 http://nova.clouds.archive.ubuntu.com saucy/main Sources [1,011 kB]
Ign http://security.ubuntu.com saucy-security/main Translation-en_US
Ign http://security.ubuntu.com saucy-security/universe Translation-en_US
Get:4 http://nova.clouds.archive.ubuntu.com saucy/universe Sources [6,108 kB]
Get:5 http://nova.clouds.archive.ubuntu.com saucy/main amd64 Packages [1,239 kB]
Get:6 http://nova.clouds.archive.ubuntu.com saucy/universe amd64 Packages [5,642 kB]
Hit http://nova.clouds.archive.ubuntu.com saucy/main Translation-en
Get:7 http://nova.clouds.archive.ubuntu.com saucy/universe Translation-en [3,884 kB]
Hit http://nova.clouds.archive.ubuntu.com saucy-updates/main Sources
Hit http://nova.clouds.archive.ubuntu.com saucy-updates/universe Sources
Hit http://nova.clouds.archive.ubuntu.com saucy-updates/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com saucy-updates/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com saucy-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com saucy-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com saucy/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com saucy/universe Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com saucy-updates/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com saucy-updates/universe Translation-en_US
Fetched 17.9 MB in 8s (2,081 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 avahi-daemon avahi-utils bind9 bind9utils build-essential curl daemontools distro-info dnsutils firefox freeipmi-tools ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make postgresql-9.1 python-amqplib python-avahi python-bzrlib python-celery python-convoy python-cssselect python-curtin python-dbus python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-formencode python-httplib2 python-jinja2 python-lockfile python-lxml python-netaddr python-netifaces python-oauth python-oops python-oop...

Revision history for this message
MAAS Lander (maas-lander) wrote :
Download full text (23.7 KiB)

The attempt to merge lp:~gmb/maas/fix-bug-1184589 into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com saucy-security InRelease
Hit http://security.ubuntu.com saucy-security Release.gpg
Hit http://security.ubuntu.com saucy-security Release
Ign http://nova.clouds.archive.ubuntu.com saucy InRelease
Ign http://nova.clouds.archive.ubuntu.com saucy-updates InRelease
Get:1 http://nova.clouds.archive.ubuntu.com saucy Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com saucy-updates Release.gpg
Get:2 http://nova.clouds.archive.ubuntu.com saucy Release [49.6 kB]
Hit http://nova.clouds.archive.ubuntu.com saucy-updates Release
Hit http://security.ubuntu.com saucy-security/main Sources
Hit http://security.ubuntu.com saucy-security/universe Sources
Hit http://security.ubuntu.com saucy-security/main amd64 Packages
Hit http://security.ubuntu.com saucy-security/universe amd64 Packages
Hit http://security.ubuntu.com saucy-security/main Translation-en
Hit http://security.ubuntu.com saucy-security/universe Translation-en
Get:3 http://nova.clouds.archive.ubuntu.com saucy/main Sources [1,011 kB]
Ign http://security.ubuntu.com saucy-security/main Translation-en_US
Ign http://security.ubuntu.com saucy-security/universe Translation-en_US
Get:4 http://nova.clouds.archive.ubuntu.com saucy/universe Sources [6,108 kB]
Get:5 http://nova.clouds.archive.ubuntu.com saucy/main amd64 Packages [1,239 kB]
Get:6 http://nova.clouds.archive.ubuntu.com saucy/universe amd64 Packages [5,642 kB]
Hit http://nova.clouds.archive.ubuntu.com saucy/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com saucy/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com saucy-updates/main Sources
Hit http://nova.clouds.archive.ubuntu.com saucy-updates/universe Sources
Hit http://nova.clouds.archive.ubuntu.com saucy-updates/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com saucy-updates/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com saucy-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com saucy-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com saucy/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com saucy/universe Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com saucy-updates/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com saucy-updates/universe Translation-en_US
Fetched 14.1 MB in 7s (1,983 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 avahi-daemon avahi-utils bind9 bind9utils build-essential curl daemontools distro-info dnsutils firefox freeipmi-tools ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make postgresql-9.1 python-amqplib python-avahi python-bzrlib python-celery python-convoy python-cssselect python-curtin python-dbus python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-formencode python-httplib2 python-jinja2 python-lockfile python-lxml python-netaddr python-netifaces python-oauth python-oops python-oops-amqp python...

Revision history for this message
MAAS Lander (maas-lander) wrote :
Download full text (19.0 KiB)

The attempt to merge lp:~gmb/maas/fix-bug-1184589 into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com saucy-security InRelease
Ign http://nova.clouds.archive.ubuntu.com saucy InRelease
Hit http://security.ubuntu.com saucy-security Release.gpg
Hit http://security.ubuntu.com saucy-security Release
Ign http://nova.clouds.archive.ubuntu.com saucy-updates InRelease
Get:1 http://nova.clouds.archive.ubuntu.com saucy Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com saucy-updates Release.gpg
Get:2 http://nova.clouds.archive.ubuntu.com saucy Release [49.6 kB]
Hit http://nova.clouds.archive.ubuntu.com saucy-updates Release
Hit http://security.ubuntu.com saucy-security/main Sources
Hit http://security.ubuntu.com saucy-security/universe Sources
Hit http://security.ubuntu.com saucy-security/main amd64 Packages
Hit http://security.ubuntu.com saucy-security/universe amd64 Packages
Hit http://security.ubuntu.com saucy-security/main Translation-en
Hit http://security.ubuntu.com saucy-security/universe Translation-en
Get:3 http://nova.clouds.archive.ubuntu.com saucy/main Sources [1,011 kB]
Ign http://security.ubuntu.com saucy-security/main Translation-en_US
Ign http://security.ubuntu.com saucy-security/universe Translation-en_US
Get:4 http://nova.clouds.archive.ubuntu.com saucy/universe Sources [6,108 kB]
Get:5 http://nova.clouds.archive.ubuntu.com saucy/main amd64 Packages [1,239 kB]
Get:6 http://nova.clouds.archive.ubuntu.com saucy/universe amd64 Packages [5,642 kB]
Hit http://nova.clouds.archive.ubuntu.com saucy/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com saucy/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com saucy-updates/main Sources
Hit http://nova.clouds.archive.ubuntu.com saucy-updates/universe Sources
Hit http://nova.clouds.archive.ubuntu.com saucy-updates/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com saucy-updates/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com saucy-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com saucy-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com saucy/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com saucy/universe Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com saucy-updates/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com saucy-updates/universe Translation-en_US
Fetched 14.0 MB in 7s (1,869 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 avahi-daemon avahi-utils bind9 bind9utils build-essential curl daemontools distro-info dnsutils firefox freeipmi-tools ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make postgresql-9.1 python-amqplib python-avahi python-bzrlib python-celery python-convoy python-cssselect python-curtin python-dbus python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-formencode python-httplib2 python-jinja2 python-lockfile python-lxml python-netaddr python-netifaces python-oauth python-oops python-oops-amqp python...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/models/tests/test_nodegroup.py'
2--- src/maasserver/models/tests/test_nodegroup.py 2013-10-07 09:12:40 +0000
3+++ src/maasserver/models/tests/test_nodegroup.py 2013-10-15 07:08:29 +0000
4@@ -390,7 +390,7 @@
5 self.assertNotEqual(nodegroup1.dhcp_key, nodegroup2.dhcp_key)
6
7 def test_import_boot_images_calls_script_with_proxy(self):
8- recorder = self.patch(tasks, 'check_call', Mock())
9+ recorder = self.patch(tasks, 'call_and_check')
10 proxy = factory.make_name('proxy')
11 Config.objects.set_config('http_proxy', proxy)
12 nodegroup = factory.make_node_group()
13
14=== modified file 'src/maasserver/tests/test_api_nodegroup.py'
15--- src/maasserver/tests/test_api_nodegroup.py 2013-10-07 09:12:40 +0000
16+++ src/maasserver/tests/test_api_nodegroup.py 2013-10-15 07:08:29 +0000
17@@ -547,7 +547,7 @@
18 self.assertItemsEqual([node.system_id], parsed_result)
19
20 def test_nodegroup_import_boot_images_calls_script(self):
21- recorder = self.patch(tasks, 'check_call')
22+ recorder = self.patch(tasks, 'call_and_check')
23 proxy = factory.getRandomString()
24 Config.objects.set_config('http_proxy', proxy)
25 nodegroup = factory.make_node_group()
26
27=== modified file 'src/maasserver/tests/test_dhcp.py'
28--- src/maasserver/tests/test_dhcp.py 2013-10-07 09:12:40 +0000
29+++ src/maasserver/tests/test_dhcp.py 2013-10-15 07:08:29 +0000
30@@ -114,7 +114,7 @@
31
32 def test_configure_dhcp_restart_dhcp_server(self):
33 self.patch(tasks, "sudo_write_file")
34- mocked_check_call = self.patch(tasks, "check_call")
35+ mocked_check_call = self.patch(tasks, "call_and_check")
36 self.patch(settings, "DHCP_CONNECT", True)
37 nodegroup = factory.make_node_group(status=NODEGROUP_STATUS.ACCEPTED)
38 configure_dhcp(nodegroup)
39
40=== modified file 'src/provisioningserver/dns/config.py'
41--- src/provisioningserver/dns/config.py 2013-10-07 09:12:40 +0000
42+++ src/provisioningserver/dns/config.py 2013-10-15 07:08:29 +0000
43@@ -33,15 +33,13 @@
44 )
45 import os.path
46 import re
47-from subprocess import (
48- check_call,
49- check_output,
50- )
51
52 from celery.conf import conf
53 from provisioningserver.dns.utils import generated_hostname
54 from provisioningserver.utils import (
55 atomic_write,
56+ call_and_check,
57+ call_capture_and_check,
58 incremental_write,
59 locate_config,
60 )
61@@ -100,7 +98,7 @@
62 # Generate the configuration:
63 # - 256 bits is the recommended size for the key nowadays.
64 # - Use urandom to avoid blocking on the random generator.
65- rndc_content = check_output(
66+ rndc_content = call_capture_and_check(
67 ['rndc-confgen', '-b', '256', '-r', '/dev/urandom',
68 '-k', key_name, '-p', unicode(port).encode("ascii")])
69 named_comment = extract_suggested_named_conf(rndc_content)
70@@ -149,7 +147,7 @@
71 rndc_cmd = ['rndc', '-c', rndc_conf]
72 rndc_cmd.extend(arguments)
73 with open(os.devnull, "ab") as devnull:
74- check_call(rndc_cmd, stdout=devnull)
75+ call_and_check(rndc_cmd, stdout=devnull)
76
77
78 # Location of DNS templates, relative to the configuration directory.
79
80=== modified file 'src/provisioningserver/dns/tests/test_config.py'
81--- src/provisioningserver/dns/tests/test_config.py 2013-10-07 09:12:40 +0000
82+++ src/provisioningserver/dns/tests/test_config.py 2013-10-15 07:08:29 +0000
83@@ -21,6 +21,7 @@
84 import errno
85 import os.path
86 import random
87+from subprocess import CalledProcessError
88 from textwrap import dedent
89
90 from celery.conf import conf
91@@ -56,7 +57,10 @@
92 uncomment_named_conf,
93 )
94 from provisioningserver.dns.utils import generated_hostname
95-from provisioningserver.utils import locate_config
96+from provisioningserver.utils import (
97+ ExternalProcessError,
98+ locate_config,
99+ )
100 import tempita
101 from testtools.matchers import (
102 Contains,
103@@ -106,7 +110,7 @@
104 def test_execute_rndc_command_executes_command(self):
105 recorder = FakeMethod()
106 fake_dir = factory.getRandomString()
107- self.patch(config, 'check_call', recorder)
108+ self.patch(config, 'call_and_check', recorder)
109 self.patch(conf, 'DNS_CONFIG_DIR', fake_dir)
110 command = factory.getRandomString()
111 execute_rndc_command([command])
112
113=== modified file 'src/provisioningserver/import_images/config.py'
114--- src/provisioningserver/import_images/config.py 2013-10-11 14:57:17 +0000
115+++ src/provisioningserver/import_images/config.py 2013-10-15 07:08:29 +0000
116@@ -21,9 +21,11 @@
117 from os import rename
118 import os.path
119 from pipes import quote
120-from subprocess import check_output
121
122-from provisioningserver.utils import filter_dict
123+from provisioningserver.utils import (
124+ call_capture_and_check,
125+ filter_dict,
126+ )
127
128 # Legacy shell-style config file for the ephemerals config.
129 EPHEMERALS_LEGACY_CONFIG = '/etc/maas/import_ephemerals'
130@@ -64,7 +66,7 @@
131 export_vars = 'export ' + ' '.join(quote(var) for var in options)
132 dump_env = 'env -0'
133 shell_code = '; '.join([source_config, export_vars, dump_env])
134- output = check_output(['bash', '-c', shell_code])
135+ output = call_capture_and_check(['bash', '-c', shell_code])
136 # Assume UTF-8 encoding. If the system uses something else but the
137 # variables are all ASCII, that's probably fine too.
138 output = output.decode('utf-8')
139
140=== modified file 'src/provisioningserver/import_images/ephemerals_script.py'
141--- src/provisioningserver/import_images/ephemerals_script.py 2013-10-11 14:02:22 +0000
142+++ src/provisioningserver/import_images/ephemerals_script.py 2013-10-15 07:08:29 +0000
143@@ -28,7 +28,6 @@
144 import os.path
145 import re
146 import shutil
147-import subprocess
148 import tempfile
149
150 from provisioningserver.config import Config
151@@ -47,6 +46,7 @@
152 )
153 from provisioningserver.pxe.install_image import install_image
154 from provisioningserver.utils import (
155+ call_and_check,
156 ensure_dir,
157 tempdir,
158 )
159@@ -93,7 +93,7 @@
160
161 Here only so tests can stub it out.
162 """
163- subprocess.check_call(["uec2roottar"] + list(args))
164+ call_and_check(["uec2roottar"] + list(args))
165
166
167 def extract_image_tarball(tarball, target_dir, temp_location=None):
168@@ -114,7 +114,7 @@
169 with tempdir(location=temp_location) as tmp:
170 # Unpack tarball. The -S flag is for sparse files; the disk image
171 # may have holes.
172- subprocess.check_call(["tar", "-Sxzf", tarball, "-C", tmp])
173+ call_and_check(["tar", "-Sxzf", tarball, "-C", tmp])
174
175 copy_file_by_glob(tmp, '*-vmlinuz*', target_dir, 'linux')
176 copy_file_by_glob(tmp, '*-initrd*', target_dir, 'initrd.gz')
177
178=== modified file 'src/provisioningserver/import_images/tests/test_config.py'
179--- src/provisioningserver/import_images/tests/test_config.py 2013-10-11 13:59:07 +0000
180+++ src/provisioningserver/import_images/tests/test_config.py 2013-10-15 07:08:29 +0000
181@@ -13,7 +13,6 @@
182 __all__ = []
183
184 import os.path
185-from subprocess import CalledProcessError
186
187 from maastesting.factory import factory
188 from provisioningserver.import_images import config as config_module
189@@ -23,6 +22,7 @@
190 retire_legacy_config,
191 )
192 from provisioningserver.testing.testcase import PservTestCase
193+from provisioningserver.utils import ExternalProcessError
194 from testtools.matchers import (
195 FileContains,
196 FileExists,
197@@ -142,7 +142,7 @@
198 make_legacy_config(self, {make_var_name(): 'x ; exit 1'})
199
200 self.assertRaises(
201- CalledProcessError,
202+ ExternalProcessError,
203 parse_legacy_config, dict([make_option_and_value()]))
204
205
206
207=== modified file 'src/provisioningserver/import_images/tests/test_ephemerals_script.py'
208--- src/provisioningserver/import_images/tests/test_ephemerals_script.py 2013-10-11 13:46:00 +0000
209+++ src/provisioningserver/import_images/tests/test_ephemerals_script.py 2013-10-15 07:08:29 +0000
210@@ -46,7 +46,10 @@
211 )
212 from provisioningserver.testing.config import ConfigFixture
213 from provisioningserver.testing.testcase import PservTestCase
214-from provisioningserver.utils import read_text_file
215+from provisioningserver.utils import (
216+ ExternalProcessError,
217+ read_text_file,
218+ )
219 from testtools.matchers import (
220 FileContains,
221 StartsWith,
222@@ -222,10 +225,8 @@
223 self.assertItemsEqual([], listdir(temp_location))
224
225 def test_cleans_up_after_failure(self):
226- class DeliberateFailure(RuntimeError):
227- pass
228-
229- self.patch(subprocess, 'check_call').side_effect = DeliberateFailure()
230+ self.patch(subprocess, 'check_call').side_effect = (
231+ ExternalProcessError(-1, "some_command"))
232 fake_image = factory.make_name('image')
233 self.patch(ephemerals_script, 'copy_file_by_glob').return_value = (
234 fake_image)
235@@ -234,7 +235,7 @@
236 temp_location = self.make_dir()
237
238 self.assertRaises(
239- DeliberateFailure,
240+ ExternalProcessError,
241 extract_image_tarball, tarball, target_dir, temp_location)
242
243 self.assertItemsEqual([], listdir(temp_location))
244
245=== modified file 'src/provisioningserver/import_images/tgt.py'
246--- src/provisioningserver/import_images/tgt.py 2013-10-08 09:49:25 +0000
247+++ src/provisioningserver/import_images/tgt.py 2013-10-15 07:08:29 +0000
248@@ -27,7 +27,6 @@
249 import os.path
250 import re
251 import shutil
252-import subprocess
253 from textwrap import dedent
254
255 from provisioningserver.kernel_opts import (
256@@ -35,6 +34,8 @@
257 prefix_target_name,
258 )
259 from provisioningserver.utils import (
260+ call_and_check,
261+ call_capture_and_check,
262 ensure_dir,
263 write_text_file,
264 )
265@@ -91,7 +92,7 @@
266
267 def tgt_admin_delete(name):
268 """Delete a target using `tgt-admin`."""
269- subprocess.check_call(TGT_ADMIN + ["--delete", prefix_target_name(name)])
270+ call_and_check(TGT_ADMIN + ["--delete", prefix_target_name(name)])
271
272
273 class TargetNotCreated(RuntimeError):
274@@ -104,7 +105,7 @@
275 :param full_name: Full target name, including `ISCSI_TARGET_NAME_PREFIX`.
276 :return: bool.
277 """
278- status = subprocess.check_output(TGT_ADMIN + ["--show"])
279+ status = call_capture_and_check(TGT_ADMIN + ["--show"])
280 regex = b'^Target [0-9]+: %s\\s*$' % re.escape(full_name).encode('ascii')
281 match = re.search(regex, status, flags=re.MULTILINE)
282 return match is not None
283@@ -116,7 +117,7 @@
284 Actually we use this to add new targets.
285 """
286 full_name = prefix_target_name(target_name)
287- subprocess.check_call(TGT_ADMIN + ["--update", full_name])
288+ call_and_check(TGT_ADMIN + ["--update", full_name])
289 # Check that the target was really created.
290 # Reportedly tgt-admin tends to return 0 even when it fails, so check
291 # actively.
292
293=== modified file 'src/provisioningserver/omshell.py'
294--- src/provisioningserver/omshell.py 2013-10-07 09:12:40 +0000
295+++ src/provisioningserver/omshell.py 2013-10-15 07:08:29 +0000
296@@ -22,14 +22,14 @@
297 import os
298 import re
299 from subprocess import (
300- CalledProcessError,
301- check_output,
302 PIPE,
303 Popen,
304 )
305 from textwrap import dedent
306
307 from provisioningserver.utils import (
308+ call_capture_and_check,
309+ ExternalProcessError,
310 parse_key_value_file,
311 tempdir,
312 )
313@@ -42,7 +42,7 @@
314 path = os.environ.get("PATH", "").split(os.pathsep)
315 path.append("/usr/sbin")
316 env = dict(os.environ, PATH=os.pathsep.join(path))
317- return check_output(
318+ return call_capture_and_check(
319 ['dnssec-keygen', '-r', '/dev/urandom', '-a', 'HMAC-MD5',
320 '-b', '512', '-n', 'HOST', '-K', tmpdir, '-q', 'omapi_key'],
321 env=env)
322@@ -129,7 +129,7 @@
323 proc = Popen(command, stdin=PIPE, stdout=PIPE)
324 stdout, stderr = proc.communicate(stdin)
325 if proc.poll() != 0:
326- raise CalledProcessError(proc.returncode, command, stdout)
327+ raise ExternalProcessError(proc.returncode, command, stdout)
328 return proc.returncode, stdout
329
330 def create(self, ip_address, mac_address):
331@@ -162,7 +162,7 @@
332 # Host map already existed. Treat as success.
333 pass
334 else:
335- raise CalledProcessError(returncode, "omshell", output)
336+ raise ExternalProcessError(returncode, "omshell", output)
337
338 def remove(self, ip_address):
339 # The "name" is not a host name; it's an identifier used within
340@@ -189,4 +189,4 @@
341 except IndexError:
342 last_line = ""
343 if last_line != "obj: <null>":
344- raise CalledProcessError(returncode, "omshell", output)
345+ raise ExternalProcessError(returncode, "omshell", output)
346
347=== modified file 'src/provisioningserver/tasks.py'
348--- src/provisioningserver/tasks.py 2013-10-07 09:12:40 +0000
349+++ src/provisioningserver/tasks.py 2013-10-15 07:08:29 +0000
350@@ -26,10 +26,7 @@
351 ]
352
353 import os
354-from subprocess import (
355- CalledProcessError,
356- check_call,
357- )
358+from subprocess import CalledProcessError
359
360 from celery.app import app_or_default
361 from celery.task import task
362@@ -53,7 +50,11 @@
363 PowerAction,
364 PowerActionFail,
365 )
366-from provisioningserver.utils import sudo_write_file
367+from provisioningserver.utils import (
368+ call_and_check,
369+ ExternalProcessError,
370+ sudo_write_file,
371+ )
372
373 # For each item passed to refresh_secrets, a refresh function to give it to.
374 refresh_functions = {
375@@ -327,7 +328,7 @@
376 @task
377 def restart_dhcp_server():
378 """Restart the DHCP server."""
379- check_call(['sudo', '-n', 'service', 'maas-dhcp-server', 'restart'])
380+ call_and_check(['sudo', '-n', 'service', 'maas-dhcp-server', 'restart'])
381
382
383 # =====================================================================
384@@ -388,4 +389,4 @@
385 env['PORTS_ARCHIVE'] = ports_archive
386 if cloud_images_archive is not None:
387 env['CLOUD_IMAGES_ARCHIVE'] = cloud_images_archive
388- check_call(['sudo', '-n', '-E', 'maas-import-pxe-files'], env=env)
389+ call_and_check(['sudo', '-n', '-E', 'maas-import-pxe-files'], env=env)
390
391=== modified file 'src/provisioningserver/tests/test_omshell.py'
392--- src/provisioningserver/tests/test_omshell.py 2013-10-07 10:51:06 +0000
393+++ src/provisioningserver/tests/test_omshell.py 2013-10-15 07:08:29 +0000
394@@ -16,7 +16,7 @@
395
396 from itertools import product
397 import os
398-from subprocess import CalledProcessError
399+import subprocess
400 import tempfile
401 from textwrap import dedent
402
403@@ -24,13 +24,15 @@
404 from maastesting.fakemethod import FakeMethod
405 from maastesting.fixtures import TempDirectory
406 from maastesting.testcase import MAASTestCase
407-from mock import Mock
408+from mock import Mock, ANY
409 from provisioningserver import omshell
410 import provisioningserver.omshell
411 from provisioningserver.omshell import (
412+ call_dnssec_keygen,
413 generate_omapi_key,
414 Omshell,
415 )
416+from provisioningserver.utils import ExternalProcessError
417 from testtools.matchers import (
418 EndsWith,
419 MatchesStructure,
420@@ -101,7 +103,7 @@
421 shell._run = recorder
422
423 exc = self.assertRaises(
424- CalledProcessError, shell.create, ip_address, mac_address)
425+ ExternalProcessError, shell.create, ip_address, mac_address)
426 self.assertEqual(random_output, exc.output)
427
428 def test_create_succeeds_when_host_map_already_exists(self):
429@@ -178,7 +180,7 @@
430 shell._run = recorder
431
432 exc = self.assertRaises(
433- CalledProcessError, shell.remove, ip_address)
434+ subprocess.CalledProcessError, shell.remove, ip_address)
435 self.assertEqual(random_output, exc.output)
436
437
438@@ -249,3 +251,18 @@
439
440 # generate_omapi_key() does not return a key known to be bad.
441 self.assertNotIn(generate_omapi_key(), bad_keys)
442+
443+
444+class TestCallDnsSecKeygen(MAASTestCase):
445+ """Tests for omshell.call_dnssec_keygen."""
446+
447+ def test_runs_external_script(self):
448+ check_output = self.patch(subprocess, 'check_output')
449+ target_dir = self.make_dir()
450+ path = os.environ.get("PATH", "").split(os.pathsep)
451+ path.append("/usr/sbin")
452+ call_dnssec_keygen(target_dir)
453+ check_output.assert_called_once_with([
454+ 'dnssec-keygen', '-r', '/dev/urandom', '-a', 'HMAC-MD5',
455+ '-b', '512', '-n', 'HOST', '-K', target_dir, '-q', 'omapi_key'
456+ ], env=ANY)
457
458=== modified file 'src/provisioningserver/tests/test_tasks.py'
459--- src/provisioningserver/tests/test_tasks.py 2013-10-07 09:12:40 +0000
460+++ src/provisioningserver/tests/test_tasks.py 2013-10-15 07:08:29 +0000
461@@ -272,7 +272,7 @@
462
463 def test_restart_dhcp_server_sends_command(self):
464 recorder = FakeMethod()
465- self.patch(tasks, 'check_call', recorder)
466+ self.patch(tasks, 'call_and_check', recorder)
467 restart_dhcp_server()
468 self.assertEqual(
469 (1, (['sudo', '-n', 'service', 'maas-dhcp-server', 'restart'],)),
470@@ -417,15 +417,15 @@
471 # If we simulate RNDC_COMMAND_MAX_RETRY + 1 failures, the
472 # task fails.
473 number_of_failures = RNDC_COMMAND_MAX_RETRY + 1
474- raised_exception = CalledProcessError(
475- factory.make_name('exception'), random.randint(100, 200))
476+ raised_exception = utils.ExternalProcessError(
477+ random.randint(100, 200), factory.make_name('exception'))
478 simulate_failures = MultiFakeMethod(
479 [FakeMethod(failure=raised_exception)] * number_of_failures +
480 [FakeMethod()])
481 self.patch(tasks, 'execute_rndc_command', simulate_failures)
482 command = factory.getRandomString()
483 self.assertRaises(
484- CalledProcessError, rndc_command.delay, command, retry=True)
485+ utils.ExternalProcessError, rndc_command.delay, command, retry=True)
486
487 def test_rndc_command_attached_to_dns_worker_queue(self):
488 self.assertEqual(rndc_command.queue, celery_config.WORKER_QUEUE_DNS)
489@@ -544,20 +544,20 @@
490 return 'http://%s.example.com/%s' % (name, factory.make_name('path'))
491
492 def test_import_boot_images(self):
493- recorder = self.patch(tasks, 'check_call', Mock())
494+ recorder = self.patch(tasks, 'call_and_check')
495 import_boot_images()
496 recorder.assert_called_once_with(
497 ['sudo', '-n', '-E', 'maas-import-pxe-files'], env=ANY)
498 self.assertIsInstance(import_boot_images, Task)
499
500 def test_import_boot_images_preserves_environment(self):
501- recorder = self.patch(tasks, 'check_call', Mock())
502+ recorder = self.patch(tasks, 'call_and_check')
503 import_boot_images()
504 recorder.assert_called_once_with(
505 ['sudo', '-n', '-E', 'maas-import-pxe-files'], env=os.environ)
506
507 def test_import_boot_images_sets_proxy(self):
508- recorder = self.patch(tasks, 'check_call', Mock())
509+ recorder = self.patch(tasks, 'call_and_check')
510 proxy = factory.getRandomString()
511 import_boot_images(http_proxy=proxy)
512 expected_env = dict(os.environ, http_proxy=proxy, https_proxy=proxy)
513@@ -565,7 +565,7 @@
514 ['sudo', '-n', '-E', 'maas-import-pxe-files'], env=expected_env)
515
516 def test_import_boot_images_sets_archive_locations(self):
517- self.patch(tasks, 'check_call')
518+ self.patch(tasks, 'call_and_check')
519 archives = {
520 'main_archive': self.make_archive_url('main'),
521 'ports_archive': self.make_archive_url('ports'),
522@@ -575,7 +575,7 @@
523 parameter.upper(): value
524 for parameter, value in archives.items()}
525 import_boot_images(**archives)
526- env = tasks.check_call.call_args[1]['env']
527+ env = tasks.call_and_check.call_args[1]['env']
528 archive_settings = {
529 variable: value
530 for variable, value in env.iteritems()
531
532=== modified file 'src/provisioningserver/tests/test_utils.py'
533--- src/provisioningserver/tests/test_utils.py 2013-10-07 09:12:40 +0000
534+++ src/provisioningserver/tests/test_utils.py 2013-10-15 07:08:29 +0000
535@@ -1,3 +1,4 @@
536+# -*- coding: UTF-8 -*-
537 # Copyright 2012-2013 Canonical Ltd. This software is licensed under the
538 # GNU Affero General Public License version 3 (see the file LICENSE).
539
540@@ -24,6 +25,7 @@
541 from shutil import rmtree
542 import stat
543 import StringIO
544+import subprocess
545 from subprocess import (
546 CalledProcessError,
547 PIPE,
548@@ -59,8 +61,11 @@
549 ActionScript,
550 atomic_write,
551 AtomicWriteScript,
552+ call_and_check,
553+ call_capture_and_check,
554 classify,
555 ensure_dir,
556+ ExternalProcessError,
557 filter_dict,
558 get_all_interface_addresses,
559 get_mtime,
560@@ -1175,3 +1180,68 @@
561 self.assertSequenceEqual(
562 (['two'], ['one', 'three']),
563 classify(is_even, subjects))
564+
565+
566+class TestSubprocessWrappers(MAASTestCase):
567+ """Tests for the subprocess.* wrapper functions."""
568+
569+ def test_call_and_check_returns_returncode(self):
570+ self.patch(subprocess, 'check_call', FakeMethod(0))
571+ self.assertEqual(0, call_and_check('some_command'))
572+
573+ def test_call_and_check_raises_ExternalProcessError_on_failure(self):
574+ self.patch(subprocess, 'check_call').side_effect = (
575+ CalledProcessError('-1', 'some_command'))
576+ error = self.assertRaises(ExternalProcessError, call_and_check, "some command")
577+ self.assertEqual('-1', error.returncode)
578+ self.assertEqual('some_command', error.cmd)
579+
580+ def test_call_capture_and_check_returns_returncode(self):
581+ self.patch(subprocess, 'check_output', FakeMethod("Some output"))
582+ self.assertEqual("Some output", call_capture_and_check('some_command'))
583+
584+ def test_call_capture_and_check_raises_ExternalProcessError_on_failure(self):
585+ self.patch(subprocess, 'check_output').side_effect = (
586+ CalledProcessError('-1', 'some_command', "Some output"))
587+ error = self.assertRaises(ExternalProcessError, call_capture_and_check, "some command")
588+ self.assertEqual('-1', error.returncode)
589+ self.assertEqual('some_command', error.cmd)
590+ self.assertEqual("Some output", error.output)
591+
592+
593+class TestExternalProcessError(MAASTestCase):
594+ """Tests for the ExternalProcessError class."""
595+
596+ def test_to_unicode_converts_to_unicode(self):
597+ byte_string = b"This string will be converted. \xe5\xb2\x81\xe5."
598+ converted_string = ExternalProcessError._to_unicode(byte_string)
599+ self.assertIsInstance(converted_string, unicode)
600+ self.assertEqual(byte_string.decode('ascii', 'replace'), converted_string)
601+
602+ def test_to_ascii_converts_to_bytes(self):
603+ unicode_string = u"Thîs nøn-åßçií s†ring will be cönvërted"
604+ expected_ascii_string = b"Th?s n?n-???i? s?ring will be c?nv?rted"
605+ converted_string = ExternalProcessError._to_ascii(unicode_string)
606+ self.assertIsInstance(converted_string, bytes)
607+ self.assertEqual(expected_ascii_string, converted_string)
608+
609+ def test__str__returns_bytes(self):
610+ error = ExternalProcessError(returncode=-1, cmd="foo-bar")
611+ self.assertIsInstance(error.__str__(), bytes)
612+
613+ def test__unicode__returns_unicode(self):
614+ error = ExternalProcessError(returncode=-1, cmd="foo-bar")
615+ self.assertIsInstance(error.__unicode__(), unicode)
616+
617+ def test__str__contains_output(self):
618+ output = u"Hëré's søme øu†pût"
619+ ascii_output = "H?r?'s s?me ?u?p?t"
620+ error = ExternalProcessError(
621+ returncode=-1, cmd="foo-bar", output=output)
622+ self.assertIn(ascii_output, error.__str__())
623+
624+ def test__unicode__contains_output(self):
625+ output = "Hëré's søme øu†pût"
626+ error = ExternalProcessError(
627+ returncode=-1, cmd="foo-bar", output=output)
628+ self.assertIn(output, error.__unicode__())
629
630=== modified file 'src/provisioningserver/utils.py'
631--- src/provisioningserver/utils.py 2013-10-07 09:12:40 +0000
632+++ src/provisioningserver/utils.py 2013-10-15 07:08:29 +0000
633@@ -42,6 +42,7 @@
634 from pipes import quote
635 from shutil import rmtree
636 import signal
637+import subprocess
638 from subprocess import (
639 CalledProcessError,
640 PIPE,
641@@ -58,6 +59,69 @@
642 from twisted.internet.defer import maybeDeferred
643
644
645+class ExternalProcessError(CalledProcessError):
646+ """Raised when there's a problem calling an external command.
647+
648+ Unlike CalledProcessError, ExternalProcessError.__str__() contains
649+ the output of the failed external process, if available.
650+ """
651+ @staticmethod
652+ def _to_unicode(string):
653+ if isinstance(string, bytes):
654+ return string.decode("ascii", "replace")
655+ else:
656+ return unicode(string)
657+
658+ @staticmethod
659+ def _to_ascii(string):
660+ if isinstance(string, unicode):
661+ return string.encode("ascii", "replace")
662+ else:
663+ string = bytes(string)
664+ try:
665+ string.decode("ascii")
666+ except UnicodeDecodeError:
667+ return "<non-ASCII string"
668+ else:
669+ return string
670+
671+ def __unicode__(self):
672+ cmd = u" ".join(quote(self._to_unicode(part)) for part in self.cmd)
673+ output = self._to_unicode(self.output)
674+ return u"Command `%s` returned non-zero exit status %d:\n%s" % (
675+ cmd, self.returncode, output)
676+
677+ def __str__(self):
678+ cmd = b" ".join(quote(self._to_ascii(part)) for part in self.cmd)
679+ output = self._to_ascii(self.output)
680+ return b"Command `%s` returned non-zero exit status %d:\n%s" % (
681+ cmd, self.returncode, output)
682+
683+
684+def call_and_check(command, *args, **kwargs):
685+ """A wrapper around subprocess.check_call().
686+
687+ When an error occurs, raise an ExternalProcessError.
688+ """
689+ try:
690+ return subprocess.check_call(command, *args, **kwargs)
691+ except subprocess.CalledProcessError as error:
692+ error.__class__ = ExternalProcessError
693+ raise
694+
695+
696+def call_capture_and_check(command, *args, **kwargs):
697+ """A wrapper around subprocess.check_output().
698+
699+ When an error occurs, raise an ExternalProcessError.
700+ """
701+ try:
702+ return subprocess.check_output(command, *args, **kwargs)
703+ except subprocess.CalledProcessError as error:
704+ error.__class__ = ExternalProcessError
705+ raise
706+
707+
708 def locate_config(*path):
709 """Return the location of a given config file or directory.
710
711@@ -362,7 +426,7 @@
712 proc = Popen(command, stdin=PIPE)
713 stdout, stderr = proc.communicate(raw_contents)
714 if proc.returncode != 0:
715- raise CalledProcessError(proc.returncode, command, stderr)
716+ raise ExternalProcessError(proc.returncode, command, stderr)
717
718
719 class Safe: