MAAS

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

  1. fix-1553841
  2. Merge into trunk
Proposed by Newell Jensen
Status: Merged
Approved by: Newell Jensen
Approved revision: no longer in the source branch.
Merged at revision: 4998
Proposed branch: lp:~newell-jensen/maas/fix-1553841
Merge into: lp:~maas-committers/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) Approve
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.
Revision history for this message
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
Revision history for this message
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.

Revision history for this message
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
Revision history for this message
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 (...

Revision history for this message
Mike Pontillo (mpontillo) wrote :

Looks like you need to "make format".

FAIL: maastesting.tests.test_lint.TestLint.test_that_imports_are_formatted

=(

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/provisioningserver/drivers/power/amt.py'
--- src/provisioningserver/drivers/power/amt.py 2016-04-15 14:54:37 +0000
+++ src/provisioningserver/drivers/power/amt.py 2016-05-05 20:22:05 +0000
@@ -21,9 +21,10 @@
21from provisioningserver.drivers.power import (21from provisioningserver.drivers.power import (
22 is_power_parameter_set,22 is_power_parameter_set,
23 PowerActionError,23 PowerActionError,
24 PowerAuthError,
24 PowerConnError,25 PowerConnError,
25 PowerDriver,26 PowerDriver,
26 PowerFatalError,27 PowerSettingError,
27)28)
28from provisioningserver.utils import (29from provisioningserver.utils import (
29 shell,30 shell,
@@ -31,6 +32,20 @@
31)32)
3233
3334
35AMT_ERRORS = {
36 '401 Unauthorized': {
37 'message': (
38 "Incorrect password. Check BMC configuration and try again."),
39 'exception': PowerAuthError
40 },
41 "500 Can't connect": {
42 'message': (
43 "Could not connect to BMC. "
44 "Check BMC configuration and try again."),
45 'exception': PowerConnError
46 },
47}
48
34REQUIRED_PACKAGES = [["amttool", "amtterm"], ["wsman", "wsmancli"]]49REQUIRED_PACKAGES = [["amttool", "amtterm"], ["wsman", "wsmancli"]]
3550
3651
@@ -176,7 +191,7 @@
176 sleep(1)191 sleep(1)
177192
178 if output is None:193 if output is None:
179 raise PowerFatalError("amttool power querying FAILED.")194 raise PowerActionError("amttool power querying failed.")
180195
181 # Ensure that from this point forward that output is a str.196 # Ensure that from this point forward that output is a str.
182 output = output.decode("utf-8")197 output = output.decode("utf-8")
@@ -228,7 +243,7 @@
228 elif state in ('6', '8', '12', '13'):243 elif state in ('6', '8', '12', '13'):
229 return 'off'244 return 'off'
230 else:245 else:
231 raise PowerFatalError(246 raise PowerActionError(
232 "Got unknown power state from node: %s" % state)247 "Got unknown power state from node: %s" % state)
233248
234 def amttool_restart(self, ip_address, power_pass, amttool_boot_mode):249 def amttool_restart(self, ip_address, power_pass, amttool_boot_mode):
@@ -306,12 +321,16 @@
306 stdout = stdout.decode("utf-8")321 stdout = stdout.decode("utf-8")
307 stderr = stderr.decode("utf-8")322 stderr = stderr.decode("utf-8")
308 if stdout == "" or stdout.isspace():323 if stdout == "" or stdout.isspace():
324 for error, error_info in AMT_ERRORS.items():
325 if error in stderr:
326 raise error_info.get(
327 'exception')(error_info.get('message'))
309 raise PowerConnError(328 raise PowerConnError(
310 "Unable to retrieve AMT version: %s" % stderr)329 "Unable to retrieve AMT version: %s" % stderr)
311 else:330 else:
312 match = re.search("AMT version:\s*([0-9]+)", stdout)331 match = re.search("AMT version:\s*([0-9]+)", stdout)
313 if match is None:332 if match is None:
314 raise PowerFatalError(333 raise PowerActionError(
315 "Unable to extract AMT version from "334 "Unable to extract AMT version from "
316 "amttool output: %s" % stdout)335 "amttool output: %s" % stdout)
317 else:336 else:
@@ -341,7 +360,9 @@
341 elif is_power_parameter_set(ip_address):360 elif is_power_parameter_set(ip_address):
342 return ip_address361 return ip_address
343 else:362 else:
344 raise PowerFatalError('No host provided')363 raise PowerSettingError(
364 "No IP address provided. "
365 "Please update BMC configuration and try again.")
345366
346 def power_on(self, system_id, context):367 def power_on(self, system_id, context):
347 """Power on AMT node."""368 """Power on AMT node."""
348369
=== modified file 'src/provisioningserver/drivers/power/tests/test_amt.py'
--- src/provisioningserver/drivers/power/tests/test_amt.py 2016-04-15 14:54:37 +0000
+++ src/provisioningserver/drivers/power/tests/test_amt.py 2016-05-05 20:22:05 +0000
@@ -26,7 +26,10 @@
26 PowerConnError,26 PowerConnError,
27 PowerFatalError,27 PowerFatalError,
28)28)
29from provisioningserver.drivers.power.amt import AMTPowerDriver29from provisioningserver.drivers.power.amt import (
30 AMT_ERRORS,
31 AMTPowerDriver,
32)
30from provisioningserver.utils.shell import has_command_available33from provisioningserver.utils.shell import has_command_available
31from testtools.matchers import Equals34from testtools.matchers import Equals
3235
@@ -58,7 +61,7 @@
58""").encode('utf-8')61""").encode('utf-8')
5962
6063
61def make_parameters():64def make_context():
62 return {65 return {
63 'system_id': factory.make_name('system_id'),66 'system_id': factory.make_name('system_id'),
64 'power_address': factory.make_name('power_address'),67 'power_address': factory.make_name('power_address'),
@@ -304,7 +307,7 @@
304 _issue_amttool_command_mock.return_value = None307 _issue_amttool_command_mock.return_value = None
305308
306 self.assertRaises(309 self.assertRaises(
307 PowerFatalError, amt_power_driver.amttool_query_state,310 PowerActionError, amt_power_driver.amttool_query_state,
308 ip_address, power_pass)311 ip_address, power_pass)
309312
310 def test_amttool_restart_power_cycles(self):313 def test_amttool_restart_power_cycles(self):
@@ -431,7 +434,7 @@
431 _issue_wsman_command_mock.return_value = WSMAN_OUTPUT % b'error'434 _issue_wsman_command_mock.return_value = WSMAN_OUTPUT % b'error'
432435
433 self.assertRaises(436 self.assertRaises(
434 PowerFatalError, amt_power_driver.wsman_query_state,437 PowerActionError, amt_power_driver.wsman_query_state,
435 ip_address, power_pass)438 ip_address, power_pass)
436 self.assertThat(439 self.assertThat(
437 _issue_wsman_command_mock, MockCalledOnceWith(440 _issue_wsman_command_mock, MockCalledOnceWith(
@@ -571,9 +574,21 @@
571 amt_power_driver = AMTPowerDriver()574 amt_power_driver = AMTPowerDriver()
572 self.patch_popen(return_value=(b'No match here', b''))575 self.patch_popen(return_value=(b'No match here', b''))
573 self.assertRaises(576 self.assertRaises(
574 PowerFatalError, amt_power_driver._get_amt_command,577 PowerActionError, amt_power_driver._get_amt_command,
575 sentinel.ip_address, sentinel.power_pass)578 sentinel.ip_address, sentinel.power_pass)
576579
580 def test__get_amt_command_raises_power_error(self):
581 amt_power_driver = AMTPowerDriver()
582 for error, error_info in AMT_ERRORS.items():
583 popen_mock = self.patch(amt_module, 'Popen')
584 process = popen_mock.return_value
585 process.communicate.return_value = (b'', error.encode('utf-8'))
586 self.assertRaises(
587 error_info.get('exception'),
588 amt_power_driver._get_amt_command,
589 factory.make_ipv4_address(),
590 factory.make_name('power_pass'))
591
577 def test__get_amttool_boot_mode_local_boot(self):592 def test__get_amttool_boot_mode_local_boot(self):
578 amt_power_driver = AMTPowerDriver()593 amt_power_driver = AMTPowerDriver()
579 result = amt_power_driver._get_amttool_boot_mode('local')594 result = amt_power_driver._get_amttool_boot_mode('local')
@@ -605,7 +620,7 @@
605620
606 def test_power_on_powers_on_with_amttool_when_already_on(self):621 def test_power_on_powers_on_with_amttool_when_already_on(self):
607 amt_power_driver = AMTPowerDriver()622 amt_power_driver = AMTPowerDriver()
608 context = make_parameters()623 context = make_context()
609 _get_amt_command_mock = self.patch(624 _get_amt_command_mock = self.patch(
610 amt_power_driver, '_get_amt_command')625 amt_power_driver, '_get_amt_command')
611 _get_amt_command_mock.return_value = 'amttool'626 _get_amt_command_mock.return_value = 'amttool'
@@ -630,7 +645,7 @@
630645
631 def test_power_on_powers_on_with_amttool_when_already_off(self):646 def test_power_on_powers_on_with_amttool_when_already_off(self):
632 amt_power_driver = AMTPowerDriver()647 amt_power_driver = AMTPowerDriver()
633 context = make_parameters()648 context = make_context()
634 _get_amt_command_mock = self.patch(649 _get_amt_command_mock = self.patch(
635 amt_power_driver, '_get_amt_command')650 amt_power_driver, '_get_amt_command')
636 _get_amt_command_mock.return_value = 'amttool'651 _get_amt_command_mock.return_value = 'amttool'
@@ -655,7 +670,7 @@
655670
656 def test_power_on_powers_on_with_wsman_when_already_on(self):671 def test_power_on_powers_on_with_wsman_when_already_on(self):
657 amt_power_driver = AMTPowerDriver()672 amt_power_driver = AMTPowerDriver()
658 context = make_parameters()673 context = make_context()
659 _get_amt_command_mock = self.patch(674 _get_amt_command_mock = self.patch(
660 amt_power_driver, '_get_amt_command')675 amt_power_driver, '_get_amt_command')
661 _get_amt_command_mock.return_value = 'wsman'676 _get_amt_command_mock.return_value = 'wsman'
@@ -679,7 +694,7 @@
679694
680 def test_power_on_powers_on_with_wsman_when_already_off(self):695 def test_power_on_powers_on_with_wsman_when_already_off(self):
681 amt_power_driver = AMTPowerDriver()696 amt_power_driver = AMTPowerDriver()
682 context = make_parameters()697 context = make_context()
683 _get_amt_command_mock = self.patch(698 _get_amt_command_mock = self.patch(
684 amt_power_driver, '_get_amt_command')699 amt_power_driver, '_get_amt_command')
685 _get_amt_command_mock.return_value = 'wsman'700 _get_amt_command_mock.return_value = 'wsman'
@@ -703,7 +718,7 @@
703718
704 def test_power_off_powers_off_with_amttool(self):719 def test_power_off_powers_off_with_amttool(self):
705 amt_power_driver = AMTPowerDriver()720 amt_power_driver = AMTPowerDriver()
706 context = make_parameters()721 context = make_context()
707 _get_amt_command_mock = self.patch(722 _get_amt_command_mock = self.patch(
708 amt_power_driver, '_get_amt_command')723 amt_power_driver, '_get_amt_command')
709 _get_amt_command_mock.return_value = 'amttool'724 _get_amt_command_mock.return_value = 'amttool'
@@ -726,7 +741,7 @@
726741
727 def test_power_off_powers_off_with_wsman(self):742 def test_power_off_powers_off_with_wsman(self):
728 amt_power_driver = AMTPowerDriver()743 amt_power_driver = AMTPowerDriver()
729 context = make_parameters()744 context = make_context()
730 _get_amt_command_mock = self.patch(745 _get_amt_command_mock = self.patch(
731 amt_power_driver, '_get_amt_command')746 amt_power_driver, '_get_amt_command')
732 _get_amt_command_mock.return_value = 'wsman'747 _get_amt_command_mock.return_value = 'wsman'
@@ -749,7 +764,7 @@
749764
750 def test_power_query_queries_with_amttool(self):765 def test_power_query_queries_with_amttool(self):
751 amt_power_driver = AMTPowerDriver()766 amt_power_driver = AMTPowerDriver()
752 context = make_parameters()767 context = make_context()
753 _get_amt_command_mock = self.patch(768 _get_amt_command_mock = self.patch(
754 amt_power_driver, '_get_amt_command')769 amt_power_driver, '_get_amt_command')
755 _get_amt_command_mock.return_value = 'amttool'770 _get_amt_command_mock.return_value = 'amttool'
@@ -769,7 +784,7 @@
769784
770 def test_power_query_queries_with_wsman(self):785 def test_power_query_queries_with_wsman(self):
771 amt_power_driver = AMTPowerDriver()786 amt_power_driver = AMTPowerDriver()
772 context = make_parameters()787 context = make_context()
773 _get_amt_command_mock = self.patch(788 _get_amt_command_mock = self.patch(
774 amt_power_driver, '_get_amt_command')789 amt_power_driver, '_get_amt_command')
775 _get_amt_command_mock.return_value = 'wsman'790 _get_amt_command_mock.return_value = 'wsman'