Merge lp:~allenap/maas/ipmi-power-confusion--bug-1560830 into lp:maas/trunk

Proposed by Gavin Panella on 2016-05-18
Status: Merged
Approved by: Gavin Panella on 2016-05-20
Approved revision: 5031
Merged at revision: 5034
Proposed branch: lp:~allenap/maas/ipmi-power-confusion--bug-1560830
Merge into: lp:maas/trunk
Diff against target: 49 lines (+15/-6)
2 files modified
src/provisioningserver/drivers/power/ipmi.py (+3/-6)
src/provisioningserver/drivers/power/tests/test_ipmi.py (+12/-0)
To merge this branch: bzr merge lp:~allenap/maas/ipmi-power-confusion--bug-1560830
Reviewer Review Type Date Requested Status
Newell Jensen Approve on 2016-05-20
Andres Rodriguez 2016-05-18 Needs Information on 2016-05-18
Review via email: mp+295018@code.launchpad.net

Commit message

In the IPMI power driver, don't get confused when ipmipower prints status for a host with 'on' or 'off' in its name.

To post a comment you must log in.
Andres Rodriguez (andreserl) wrote :

Hi Gavin,

Newell fixed this for 1.9, but It seems the patch never made it to 2.0, as I don't know if it is actually needed depending on the way how power drivers changed for 2.0, but it may still be there. Please syncup with him, but otherwise look at:

https://bugs.launchpad.net/maas/+bug/1508741

Also, question inline.

review: Needs Information
Gavin Panella (allenap) wrote :

The inline question isn't appearing for me, but I see it in the email:

> Shouldn't it be better that the regex ensures here that 'on' or 'off'
> are after ':', as in match:
>
> ': on' or ': off'.

Looking at 1.9 I can see that's what it does, and if that works, fine.

I went for \b(on|off)$ because it works for output that contains *only*
"on" or "off" as well as output like "hostname: on" and "hostname: off".
None of the existing tests demonstrate the "hostname: $state" form —
they all use simply "on" or "off" — so I was forced to assume that older
versions of ipmipower could have printed only "on" or "off", and I had
to be compatible with those.

I will talk to Newell about this. Whichever way we go, there was a small
process issue because this fix shouldn't have made it into 1.9 without
landing in trunk first.

Thanks for the review!

Christian Reis (kiko) wrote :

This is a funny one. Some questions:

On Wed, May 18, 2016 at 08:39:27AM -0000, Gavin Panella wrote:
> + match = re.search(r'\b(on|off)\b$', stdout)

I'm asking myself about the trailing \b -- is there actually a blank in
the output, or should that actually be "\b?"?

Does the ipmipower output not always include a colon? If there is a
reason not to use the colon, please add a comment.

Will this fail if a hostname is simply called "on"?

Gavin Panella (allenap) wrote :

> This is a funny one. Some questions:
>
> On Wed, May 18, 2016 at 08:39:27AM -0000, Gavin Panella wrote:
> > + match = re.search(r'\b(on|off)\b$', stdout)
>
> I'm asking myself about the trailing \b -- is there actually a blank
> in the output, or should that actually be "\b?"?

The reason for the \b is so it doesn't match things like "karloff" or
"boron", i.e. \b doesn't match blanks, it matches word boundaries (and
consumes no characters).

Meaning the trailing \b is superfluous because the $ match for the end
of the line also implies a word boundary, hence \b(on|off)$ is, I think,
equivalent to the regex I used.

>
> Does the ipmipower output not always include a colon? If there is a
> reason not to use the colon, please add a comment.

I wrote this code given the tests already written, none of which include
hostnames or colons in the example output from ipmipower. If that is not
representative of ipmipower's output now or at any point in the past
then the tests are, well, a bit broken. If someone can categorically say
that ipmipower has only ever used the form 'hostname: status' then I can
update this code and test, as well as all the other tests.

>
> Will this fail if a hostname is simply called "on"?

No; the regex anchors to the end of the line so it won't match "on: off".

Gavin Panella (allenap) wrote :

Okay, my code is a little too defensive here. Worrying about whether different versions of ipmipower might use "$hostname: $status" or just "$status" is not really warranted because we control the version of ipmipower. It might matter if we were to backport this change, but we won't because the fix already exists in 1.9.

I'll update this code to more closely resemble the fix in 1.9. It won't be the same because so much else has changed in the IPMI driver since, but it will be close enough that our future selves won't see a discrepancy.

Newell Jensen (newell-jensen) wrote :

Looks good. Thanks Gavin.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/provisioningserver/drivers/power/ipmi.py'
2--- src/provisioningserver/drivers/power/ipmi.py 2016-04-25 18:06:57 +0000
3+++ src/provisioningserver/drivers/power/ipmi.py 2016-05-20 12:40:37 +0000
4@@ -5,6 +5,7 @@
5
6 __all__ = []
7
8+import re
9 from subprocess import (
10 PIPE,
11 Popen,
12@@ -198,12 +199,8 @@
13 raise PowerError(
14 "Failed to power %s %s: %s" % (
15 power_change, power_address, stdout))
16- if 'on' in stdout:
17- return 'on'
18- elif 'off' in stdout:
19- return 'off'
20- else:
21- return stdout
22+ match = re.search(":\s*(on|off)", stdout)
23+ return stdout if match is None else match.group(1)
24
25 def _issue_ipmi_command(
26 self, power_change, power_address=None, power_user=None,
27
28=== modified file 'src/provisioningserver/drivers/power/tests/test_ipmi.py'
29--- src/provisioningserver/drivers/power/tests/test_ipmi.py 2016-05-12 19:07:37 +0000
30+++ src/provisioningserver/drivers/power/tests/test_ipmi.py 2016-05-20 12:40:37 +0000
31@@ -186,6 +186,18 @@
32 factory.make_name('command'), factory.make_name('power_change'),
33 factory.make_name('power_address'))
34
35+ def test__issue_ipmipower_command_does_not_mistake_host_for_status(self):
36+ popen_mock = self.patch(ipmi_module, 'Popen')
37+ process = popen_mock.return_value
38+ # "cameron" contains the string "on", but the machine is off.
39+ process.communicate.return_value = (b'cameron: off', b'')
40+ process.returncode = 0
41+ self.assertThat(
42+ IPMIPowerDriver._issue_ipmipower_command(
43+ factory.make_name('command'), 'query',
44+ factory.make_name('address')),
45+ Equals("off"))
46+
47 def test__issue_ipmi_command_issues_power_on(self):
48 context = make_context()
49 ipmi_chassis_config_command = make_ipmi_chassis_config_command(