Merge lp:~newell-jensen/maas/fix-1553841 into lp:maas/trunk

Proposed by Newell Jensen on 2016-05-04
Status: Merged
Approved by: Newell Jensen on 2016-05-05
Approved revision: 4988
Merged at revision: 4998
Proposed branch: lp:~newell-jensen/maas/fix-1553841
Merge into: lp:maas/trunk
Diff against target: 221 lines (+54/-18)
2 files modified
src/provisioningserver/drivers/power/amt.py (+26/-5)
src/provisioningserver/drivers/power/tests/test_amt.py (+28/-13)
To merge this branch: bzr merge lp:~newell-jensen/maas/fix-1553841
Reviewer Review Type Date Requested Status
Mike Pontillo (community) 2016-05-04 Approve on 2016-05-05
Review via email: mp+293819@code.launchpad.net

Commit message

Update AMT error messages to instruct the user if they have entered an incorrect BMC configuration (power parameters).

To post a comment you must log in.
Mike Pontillo (mpontillo) wrote :

Code looks fine to me. Just a couple things I'm wondering:

 * Can you update the commit message so that it is more accurate? (it makes it sound like error propagation in general, but the branch seems to just fix up the AMT exceptions so that they are thrown correctly.)

 * Are we confident the AMT utilities will always print these particular strings? (I assume we're using a consistent locale, so that the strings don't show up translated into another language and surprise us. And I was wondering if using just the HTTP status code for matching might be more future-proof... assuming the output isn't too large, and we could match the number in multiple places in the string...?)

review: Needs Information
Newell Jensen (newell-jensen) wrote :

> Code looks fine to me. Just a couple things I'm wondering:
>
> * Can you update the commit message so that it is more accurate? (it makes it
> sound like error propagation in general, but the branch seems to just fix up
> the AMT exceptions so that they are thrown correctly.)
>
> * Are we confident the AMT utilities will always print these particular
> strings? (I assume we're using a consistent locale, so that the strings don't
> show up translated into another language and surprise us.

The subprocesses are executed using select_c_utf8_locale() to set the environment.

 And I was wondering
> if using just the HTTP status code for matching might be more future-proof...
> assuming the output isn't too large, and we could match the number in multiple
> places in the string...?)

I am on the fence about changing what we are matching on. Here are the two errors that amttool gives back:

401 Unauthorized at /usr/bin/amttool line 126.

and

500 Can't connect to 172.24.124.257:16992 at /usr/bin/amttool line 126.

If you think we should match just on the error status code and don't think it will cause any future issues, that is fine with me. These messages are coming back from amttool, so how I have it now seems fine to me.

Mike Pontillo (mpontillo) wrote :

Yeah. I suppose the other option would be to just match the beginning of the string, if it's always the first thing in the output.

OK, I'll approve this branch, but please update the commit message to be more accurate before you land it.

review: Approve
MAAS Lander (maas-lander) wrote :
Download full text (1.1 MiB)

The attempt to merge lp:~newell-jensen/maas/fix-1553841 into lp:maas failed. Below is the output from the failed tests.

Hit:1 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial InRelease
Get:2 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates InRelease [93.3 kB]
Hit:3 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-backports InRelease
Get:4 http://security.ubuntu.com/ubuntu xenial-security InRelease [92.2 kB]
Fetched 185 kB in 0s (389 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
    --no-install-recommends install apache2 archdetect-deb authbind bash bind9 bind9utils build-essential bzr bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libpq-dev make nodejs-legacy npm postgresql pxelinux python3-all python3-apt python3-bson python3-convoy python3-coverage python3-crochet python3-cssselect python3-curtin python3-dev python3-distro-info python3-django python3-django-nose python3-django-piston3 python3-dnspython python3-docutils python3-formencode python3-hivex python3-httplib2 python3-jinja2 python3-jsonschema python3-lxml python3-mock python3-netaddr python3-netifaces python3-oauth python3-oauthlib python3-openssl python3-paramiko python3-petname python3-pexpect python3-psycopg2 python3-pyinotify python3-pyparsing python3-pyvmomi python3-requests python3-seamicroclient python3-setuptools python3-simplestreams python3-sphinx python3-tempita python3-twisted python3-txtftp python3-tz python3-yaml python3-zope.interface python-bson python-crochet python-django python-django-piston python-djorm-ext-pgarray python-formencode python-lxml python-netaddr python-netifaces python-pocket-lint python-psycopg2 python-simplejson python-tempita python-twisted python-yaml socat syslinux-common tgt ubuntu-cloudimage-keyring wget xvfb
Reading package lists...
Building dependency tree...
Reading state information...
apache2 is already the newest version (2.4.18-2ubuntu3).
archdetect-deb is already the newest version (1.117ubuntu2).
authbind is already the newest version (2.1.1+nmu1).
bash is already the newest version (4.3-14ubuntu1).
build-essential is already the newest version (12.1ubuntu2).
bzr is already the newest version (2.7.0-2ubuntu1).
curl is already the newest version (7.47.0-1ubuntu2).
debhelper is already the newest version (9.20160115ubuntu3).
distro-info is already the newest version (0.14build1).
freeipmi-tools is already the newest version (1.4.11-1ubuntu1).
git is already the newest version (1:2.7.4-0ubuntu1).
isc-dhcp-common is already the newest version (4.3.3-5ubuntu12).
libjs-angularjs is already the newest version (1.2.28-1ubuntu2).
libjs-jquery is already the newest version (1.11.3+dfsg-4).
libjs-yui3-full is already the newest version (3.5.1-1ubuntu3).
libjs-yui3-min is already the newest version (3.5.1-1ubuntu3).
libpq-dev is already the newest version (9.5.2-1).
make is already the newest version (4.1-6).
postgresql is already the newest version (...

Mike Pontillo (mpontillo) wrote :

Looks like you need to "make format".

FAIL: maastesting.tests.test_lint.TestLint.test_that_imports_are_formatted

=(

lp:~newell-jensen/maas/fix-1553841 updated on 2016-05-05
4988. By Newell Jensen <email address hidden> on 2016-05-05

Fix lint.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/provisioningserver/drivers/power/amt.py'
2--- src/provisioningserver/drivers/power/amt.py 2016-04-15 14:54:37 +0000
3+++ src/provisioningserver/drivers/power/amt.py 2016-05-05 20:22:05 +0000
4@@ -21,9 +21,10 @@
5 from provisioningserver.drivers.power import (
6 is_power_parameter_set,
7 PowerActionError,
8+ PowerAuthError,
9 PowerConnError,
10 PowerDriver,
11- PowerFatalError,
12+ PowerSettingError,
13 )
14 from provisioningserver.utils import (
15 shell,
16@@ -31,6 +32,20 @@
17 )
18
19
20+AMT_ERRORS = {
21+ '401 Unauthorized': {
22+ 'message': (
23+ "Incorrect password. Check BMC configuration and try again."),
24+ 'exception': PowerAuthError
25+ },
26+ "500 Can't connect": {
27+ 'message': (
28+ "Could not connect to BMC. "
29+ "Check BMC configuration and try again."),
30+ 'exception': PowerConnError
31+ },
32+}
33+
34 REQUIRED_PACKAGES = [["amttool", "amtterm"], ["wsman", "wsmancli"]]
35
36
37@@ -176,7 +191,7 @@
38 sleep(1)
39
40 if output is None:
41- raise PowerFatalError("amttool power querying FAILED.")
42+ raise PowerActionError("amttool power querying failed.")
43
44 # Ensure that from this point forward that output is a str.
45 output = output.decode("utf-8")
46@@ -228,7 +243,7 @@
47 elif state in ('6', '8', '12', '13'):
48 return 'off'
49 else:
50- raise PowerFatalError(
51+ raise PowerActionError(
52 "Got unknown power state from node: %s" % state)
53
54 def amttool_restart(self, ip_address, power_pass, amttool_boot_mode):
55@@ -306,12 +321,16 @@
56 stdout = stdout.decode("utf-8")
57 stderr = stderr.decode("utf-8")
58 if stdout == "" or stdout.isspace():
59+ for error, error_info in AMT_ERRORS.items():
60+ if error in stderr:
61+ raise error_info.get(
62+ 'exception')(error_info.get('message'))
63 raise PowerConnError(
64 "Unable to retrieve AMT version: %s" % stderr)
65 else:
66 match = re.search("AMT version:\s*([0-9]+)", stdout)
67 if match is None:
68- raise PowerFatalError(
69+ raise PowerActionError(
70 "Unable to extract AMT version from "
71 "amttool output: %s" % stdout)
72 else:
73@@ -341,7 +360,9 @@
74 elif is_power_parameter_set(ip_address):
75 return ip_address
76 else:
77- raise PowerFatalError('No host provided')
78+ raise PowerSettingError(
79+ "No IP address provided. "
80+ "Please update BMC configuration and try again.")
81
82 def power_on(self, system_id, context):
83 """Power on AMT node."""
84
85=== modified file 'src/provisioningserver/drivers/power/tests/test_amt.py'
86--- src/provisioningserver/drivers/power/tests/test_amt.py 2016-04-15 14:54:37 +0000
87+++ src/provisioningserver/drivers/power/tests/test_amt.py 2016-05-05 20:22:05 +0000
88@@ -26,7 +26,10 @@
89 PowerConnError,
90 PowerFatalError,
91 )
92-from provisioningserver.drivers.power.amt import AMTPowerDriver
93+from provisioningserver.drivers.power.amt import (
94+ AMT_ERRORS,
95+ AMTPowerDriver,
96+)
97 from provisioningserver.utils.shell import has_command_available
98 from testtools.matchers import Equals
99
100@@ -58,7 +61,7 @@
101 """).encode('utf-8')
102
103
104-def make_parameters():
105+def make_context():
106 return {
107 'system_id': factory.make_name('system_id'),
108 'power_address': factory.make_name('power_address'),
109@@ -304,7 +307,7 @@
110 _issue_amttool_command_mock.return_value = None
111
112 self.assertRaises(
113- PowerFatalError, amt_power_driver.amttool_query_state,
114+ PowerActionError, amt_power_driver.amttool_query_state,
115 ip_address, power_pass)
116
117 def test_amttool_restart_power_cycles(self):
118@@ -431,7 +434,7 @@
119 _issue_wsman_command_mock.return_value = WSMAN_OUTPUT % b'error'
120
121 self.assertRaises(
122- PowerFatalError, amt_power_driver.wsman_query_state,
123+ PowerActionError, amt_power_driver.wsman_query_state,
124 ip_address, power_pass)
125 self.assertThat(
126 _issue_wsman_command_mock, MockCalledOnceWith(
127@@ -571,9 +574,21 @@
128 amt_power_driver = AMTPowerDriver()
129 self.patch_popen(return_value=(b'No match here', b''))
130 self.assertRaises(
131- PowerFatalError, amt_power_driver._get_amt_command,
132+ PowerActionError, amt_power_driver._get_amt_command,
133 sentinel.ip_address, sentinel.power_pass)
134
135+ def test__get_amt_command_raises_power_error(self):
136+ amt_power_driver = AMTPowerDriver()
137+ for error, error_info in AMT_ERRORS.items():
138+ popen_mock = self.patch(amt_module, 'Popen')
139+ process = popen_mock.return_value
140+ process.communicate.return_value = (b'', error.encode('utf-8'))
141+ self.assertRaises(
142+ error_info.get('exception'),
143+ amt_power_driver._get_amt_command,
144+ factory.make_ipv4_address(),
145+ factory.make_name('power_pass'))
146+
147 def test__get_amttool_boot_mode_local_boot(self):
148 amt_power_driver = AMTPowerDriver()
149 result = amt_power_driver._get_amttool_boot_mode('local')
150@@ -605,7 +620,7 @@
151
152 def test_power_on_powers_on_with_amttool_when_already_on(self):
153 amt_power_driver = AMTPowerDriver()
154- context = make_parameters()
155+ context = make_context()
156 _get_amt_command_mock = self.patch(
157 amt_power_driver, '_get_amt_command')
158 _get_amt_command_mock.return_value = 'amttool'
159@@ -630,7 +645,7 @@
160
161 def test_power_on_powers_on_with_amttool_when_already_off(self):
162 amt_power_driver = AMTPowerDriver()
163- context = make_parameters()
164+ context = make_context()
165 _get_amt_command_mock = self.patch(
166 amt_power_driver, '_get_amt_command')
167 _get_amt_command_mock.return_value = 'amttool'
168@@ -655,7 +670,7 @@
169
170 def test_power_on_powers_on_with_wsman_when_already_on(self):
171 amt_power_driver = AMTPowerDriver()
172- context = make_parameters()
173+ context = make_context()
174 _get_amt_command_mock = self.patch(
175 amt_power_driver, '_get_amt_command')
176 _get_amt_command_mock.return_value = 'wsman'
177@@ -679,7 +694,7 @@
178
179 def test_power_on_powers_on_with_wsman_when_already_off(self):
180 amt_power_driver = AMTPowerDriver()
181- context = make_parameters()
182+ context = make_context()
183 _get_amt_command_mock = self.patch(
184 amt_power_driver, '_get_amt_command')
185 _get_amt_command_mock.return_value = 'wsman'
186@@ -703,7 +718,7 @@
187
188 def test_power_off_powers_off_with_amttool(self):
189 amt_power_driver = AMTPowerDriver()
190- context = make_parameters()
191+ context = make_context()
192 _get_amt_command_mock = self.patch(
193 amt_power_driver, '_get_amt_command')
194 _get_amt_command_mock.return_value = 'amttool'
195@@ -726,7 +741,7 @@
196
197 def test_power_off_powers_off_with_wsman(self):
198 amt_power_driver = AMTPowerDriver()
199- context = make_parameters()
200+ context = make_context()
201 _get_amt_command_mock = self.patch(
202 amt_power_driver, '_get_amt_command')
203 _get_amt_command_mock.return_value = 'wsman'
204@@ -749,7 +764,7 @@
205
206 def test_power_query_queries_with_amttool(self):
207 amt_power_driver = AMTPowerDriver()
208- context = make_parameters()
209+ context = make_context()
210 _get_amt_command_mock = self.patch(
211 amt_power_driver, '_get_amt_command')
212 _get_amt_command_mock.return_value = 'amttool'
213@@ -769,7 +784,7 @@
214
215 def test_power_query_queries_with_wsman(self):
216 amt_power_driver = AMTPowerDriver()
217- context = make_parameters()
218+ context = make_context()
219 _get_amt_command_mock = self.patch(
220 amt_power_driver, '_get_amt_command')
221 _get_amt_command_mock.return_value = 'wsman'