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

Proposed by Jeroen T. Vermeulen
Status: Merged
Approved by: Jeroen T. Vermeulen
Approved revision: 2385
Merged at revision: 2387
Proposed branch: lp:~jtv/maas/bug-1324237
Merge into: lp:maas
Diff against target: 297 lines (+64/-70)
7 files modified
src/provisioningserver/boot/utils.py (+3/-5)
src/provisioningserver/dns/config.py (+2/-4)
src/provisioningserver/import_images/tests/test_boot_resources.py (+1/-1)
src/provisioningserver/omshell.py (+3/-3)
src/provisioningserver/tests/test_omshell.py (+3/-2)
src/provisioningserver/utils/__init__.py (+15/-21)
src/provisioningserver/utils/tests/test_utils.py (+37/-34)
To merge this branch: bzr merge lp:~jtv/maas/bug-1324237
Reviewer Review Type Date Requested Status
Julian Edwards (community) Approve
Review via email: mp+221469@code.launchpad.net

Commit message

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.

Description of the change

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

To post a comment you must log in.
Revision history for this message
Julian Edwards (julian-edwards) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/provisioningserver/boot/utils.py'
2--- src/provisioningserver/boot/utils.py 2014-03-28 04:06:27 +0000
3+++ src/provisioningserver/boot/utils.py 2014-05-29 23:37:13 +0000
4@@ -24,11 +24,10 @@
5 from platform import linux_distribution
6 import re
7 import StringIO
8-import subprocess
9 import urllib2
10
11 from provisioningserver.utils import (
12- call_capture_and_check,
13+ call_and_check,
14 tempdir,
15 )
16
17@@ -71,14 +70,13 @@
18 with open(data_out, 'wb') as stream:
19 stream.write(data_file)
20
21- args = [
22+ call_and_check([
23 "gpgv",
24 "--keyring",
25 "/etc/apt/trusted.gpg",
26 sig_out,
27 data_out
28- ]
29- call_capture_and_check(args, stderr=subprocess.STDOUT)
30+ ])
31
32
33 def decompress_packages(packages):
34
35=== modified file 'src/provisioningserver/dns/config.py'
36--- src/provisioningserver/dns/config.py 2014-02-14 10:47:19 +0000
37+++ src/provisioningserver/dns/config.py 2014-05-29 23:37:13 +0000
38@@ -39,7 +39,6 @@
39 from provisioningserver.utils import (
40 atomic_write,
41 call_and_check,
42- call_capture_and_check,
43 incremental_write,
44 locate_config,
45 )
46@@ -102,7 +101,7 @@
47 # Generate the configuration:
48 # - 256 bits is the recommended size for the key nowadays.
49 # - Use urandom to avoid blocking on the random generator.
50- rndc_content = call_capture_and_check(
51+ rndc_content = call_and_check(
52 ['rndc-confgen', '-b', '256', '-r', '/dev/urandom',
53 '-k', key_name, '-p', unicode(port).encode("ascii")])
54 named_comment = extract_suggested_named_conf(rndc_content)
55@@ -149,8 +148,7 @@
56 rndc_conf = get_rndc_conf_path()
57 rndc_cmd = ['rndc', '-c', rndc_conf]
58 rndc_cmd.extend(arguments)
59- with open(os.devnull, "ab") as devnull:
60- call_and_check(rndc_cmd, stdout=devnull)
61+ call_and_check(rndc_cmd)
62
63
64 # Location of DNS templates, relative to the configuration directory.
65
66=== modified file 'src/provisioningserver/import_images/tests/test_boot_resources.py'
67--- src/provisioningserver/import_images/tests/test_boot_resources.py 2014-05-27 08:21:33 +0000
68+++ src/provisioningserver/import_images/tests/test_boot_resources.py 2014-05-29 23:37:13 +0000
69@@ -272,7 +272,7 @@
70 # at a low level, so that we exercise all the function calls that a
71 # unit test might not put to the test.
72 self.patch_logger()
73- self.patch(boot_resources, 'call_and_check').return_code = 0
74+ self.patch(boot_resources, 'call_and_check')
75
76 # We'll go through installation of a PXE boot loader here, but skip
77 # all other boot loader types. Testing them all is a job for proper
78
79=== modified file 'src/provisioningserver/omshell.py'
80--- src/provisioningserver/omshell.py 2014-02-27 03:30:05 +0000
81+++ src/provisioningserver/omshell.py 2014-05-29 23:37:13 +0000
82@@ -1,4 +1,4 @@
83-# Copyright 2012 Canonical Ltd. This software is licensed under the
84+# Copyright 2012-2014 Canonical Ltd. This software is licensed under the
85 # GNU Affero General Public License version 3 (see the file LICENSE).
86
87 """Python wrapper around the `omshell` utility which amends objects
88@@ -28,7 +28,7 @@
89 from textwrap import dedent
90
91 from provisioningserver.utils import (
92- call_capture_and_check,
93+ call_and_check,
94 ExternalProcessError,
95 parse_key_value_file,
96 tempdir,
97@@ -42,7 +42,7 @@
98 path = os.environ.get("PATH", "").split(os.pathsep)
99 path.append("/usr/sbin")
100 env = dict(os.environ, PATH=os.pathsep.join(path))
101- return call_capture_and_check(
102+ return call_and_check(
103 ['dnssec-keygen', '-r', '/dev/urandom', '-a', 'HMAC-MD5',
104 '-b', '512', '-n', 'HOST', '-K', tmpdir, '-q', 'omapi_key'],
105 env=env)
106
107=== modified file 'src/provisioningserver/tests/test_omshell.py'
108--- src/provisioningserver/tests/test_omshell.py 2014-02-27 03:29:12 +0000
109+++ src/provisioningserver/tests/test_omshell.py 2014-05-29 23:37:13 +0000
110@@ -271,12 +271,13 @@
111 """Tests for omshell.call_dnssec_keygen."""
112
113 def test_runs_external_script(self):
114- check_output = self.patch(subprocess, 'check_output')
115+ call_and_check = self.patch(
116+ provisioningserver.omshell, 'call_and_check')
117 target_dir = self.make_dir()
118 path = os.environ.get("PATH", "").split(os.pathsep)
119 path.append("/usr/sbin")
120 call_dnssec_keygen(target_dir)
121- check_output.assert_called_once_with(
122+ call_and_check.assert_called_once_with(
123 ['dnssec-keygen', '-r', '/dev/urandom', '-a', 'HMAC-MD5',
124 '-b', '512', '-n', 'HOST', '-K', target_dir, '-q', 'omapi_key'],
125 env=ANY)
126
127=== modified file 'src/provisioningserver/utils/__init__.py'
128--- src/provisioningserver/utils/__init__.py 2014-05-05 23:55:46 +0000
129+++ src/provisioningserver/utils/__init__.py 2014-05-29 23:37:13 +0000
130@@ -15,14 +15,15 @@
131 __all__ = [
132 "ActionScript",
133 "atomic_write",
134+ "call_and_check",
135 "deferred",
136+ "ensure_dir",
137 "filter_dict",
138 "find_ip_via_arp",
139 "import_settings",
140 "incremental_write",
141 "locate_config",
142 "MainScript",
143- "ensure_dir",
144 "parse_key_value_file",
145 "read_text_file",
146 "ShellTemplate",
147@@ -115,25 +116,18 @@
148 def call_and_check(command, *args, **kwargs):
149 """A wrapper around subprocess.check_call().
150
151- When an error occurs, raise an ExternalProcessError.
152- """
153- try:
154- return subprocess.check_call(command, *args, **kwargs)
155- except subprocess.CalledProcessError as error:
156- error.__class__ = ExternalProcessError
157- raise
158-
159-
160-def call_capture_and_check(command, *args, **kwargs):
161- """A wrapper around subprocess.check_output().
162-
163- When an error occurs, raise an ExternalProcessError.
164- """
165- try:
166- return subprocess.check_output(command, *args, **kwargs)
167- except subprocess.CalledProcessError as error:
168- error.__class__ = ExternalProcessError
169- raise
170+ :param command: Command line, as a list of strings.
171+ :return: The command's output from standard output.
172+ :raise ExternalProcessError: If the command returns nonzero.
173+ """
174+ process = subprocess.Popen(
175+ command, *args, stdout=subprocess.PIPE, stderr=subprocess.PIPE,
176+ **kwargs)
177+ stdout, stderr = process.communicate()
178+ stderr = stderr.strip()
179+ if process.returncode != 0:
180+ raise ExternalProcessError(process.returncode, command, output=stderr)
181+ return stdout
182
183
184 def locate_config(*path):
185@@ -822,7 +816,7 @@
186 :param mac: The mac address, e.g. '1c:6f:65:d5:56:98'.
187 """
188
189- output = call_capture_and_check(['arp', '-n']).split('\n')
190+ output = call_and_check(['arp', '-n']).split('\n')
191
192 for line in sorted(output):
193 columns = line.split()
194
195=== modified file 'src/provisioningserver/utils/tests/test_utils.py'
196--- src/provisioningserver/utils/tests/test_utils.py 2014-05-19 12:40:56 +0000
197+++ src/provisioningserver/utils/tests/test_utils.py 2014-05-29 23:37:13 +0000
198@@ -68,7 +68,6 @@
199 atomic_write,
200 AtomicWriteScript,
201 call_and_check,
202- call_capture_and_check,
203 classify,
204 ensure_dir,
205 escape_py_literal,
206@@ -1204,33 +1203,39 @@
207 classify(is_even, subjects))
208
209
210-class TestSubprocessWrappers(MAASTestCase):
211- """Tests for the subprocess.* wrapper functions."""
212-
213- def test_call_and_check_returns_returncode(self):
214- self.patch(subprocess, 'check_call', FakeMethod(0))
215- self.assertEqual(0, call_and_check('some_command'))
216-
217- def test_call_and_check_raises_ExternalProcessError_on_failure(self):
218- self.patch(subprocess, 'check_call').side_effect = (
219- CalledProcessError('-1', 'some_command'))
220- error = self.assertRaises(
221- ExternalProcessError, call_and_check, "some command")
222- self.assertEqual('-1', error.returncode)
223- self.assertEqual('some_command', error.cmd)
224-
225- def test_call_capture_and_check_returns_returncode(self):
226- self.patch(subprocess, 'check_output', FakeMethod("Some output"))
227- self.assertEqual("Some output", call_capture_and_check('some_command'))
228-
229- def test_call_capture_and_check_raises_ExternalProcessError_on_fail(self):
230- self.patch(subprocess, 'check_output').side_effect = (
231- CalledProcessError('-1', 'some_command', "Some output"))
232- error = self.assertRaises(
233- ExternalProcessError, call_capture_and_check, "some command")
234- self.assertEqual('-1', error.returncode)
235- self.assertEqual('some_command', error.cmd)
236- self.assertEqual("Some output", error.output)
237+class TestCallAndCheck(MAASTestCase):
238+ """Tests `call_and_check`."""
239+
240+ def patch_popen(self, returncode=0, stderr=''):
241+ """Replace `subprocess.Popen` with a mock."""
242+ popen = self.patch(subprocess, 'Popen')
243+ process = popen.return_value
244+ process.communicate.return_value = (None, stderr)
245+ process.returncode = returncode
246+ return process
247+
248+ def test__returns_standard_output(self):
249+ output = factory.getRandomString()
250+ self.assertEqual(output, call_and_check(['/bin/echo', '-n', output]))
251+
252+ def test__raises_ExternalProcessError_on_failure(self):
253+ command = factory.make_name('command')
254+ message = factory.getRandomString()
255+ self.patch_popen(returncode=1, stderr=message)
256+ error = self.assertRaises(
257+ ExternalProcessError, call_and_check, command)
258+ self.assertEqual(1, error.returncode)
259+ self.assertEqual(command, error.cmd)
260+ self.assertEqual(message, error.output)
261+
262+ def test__reports_stderr_on_failure(self):
263+ nonfile = os.path.join(self.make_dir(), factory.make_name('nonesuch'))
264+ error = self.assertRaises(
265+ ExternalProcessError,
266+ call_and_check, ['/bin/cat', nonfile], env={'LC_ALL': 'C'})
267+ self.assertEqual(
268+ "/bin/cat: %s: No such file or directory" % nonfile,
269+ error.output)
270
271
272 class TestExternalProcessError(MAASTestCase):
273@@ -1303,8 +1308,8 @@
274 class TestFindIPViaARP(MAASTestCase):
275
276 def patch_call(self, output):
277- """Replace `call_capture_and_check` with one that returns `output`."""
278- fake = self.patch(provisioningserver.utils, 'call_capture_and_check')
279+ """Replace `call_and_check` with one that returns `output`."""
280+ fake = self.patch(provisioningserver.utils, 'call_and_check')
281 fake.return_value = output
282 return fake
283
284@@ -1321,11 +1326,9 @@
285 192.168.0.1 ether 90:f6:52:f6:17:92 C eth0
286 """
287
288- call_capture_and_check = self.patch_call(sample)
289+ call_and_check = self.patch_call(sample)
290 ip_address_observed = find_ip_via_arp("90:f6:52:f6:17:92")
291- self.assertThat(
292- call_capture_and_check,
293- MockCalledOnceWith(['arp', '-n']))
294+ self.assertThat(call_and_check, MockCalledOnceWith(['arp', '-n']))
295 self.assertEqual("192.168.0.1", ip_address_observed)
296
297 def test__returns_consistent_output(self):