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

[Gavin Panella]

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

[Andres Rodriguez (andreserl)]

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

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

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

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

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

Looks good. Thanks Gavin.

review: Approve

=== modified file 'src/provisioningserver/drivers/power/ipmi.py'
--- src/provisioningserver/drivers/power/ipmi.py	2016-04-25 18:06:57 +0000
+++ src/provisioningserver/drivers/power/ipmi.py	2016-05-20 12:40:37 +0000
@@ -5,6 +5,7 @@
 
 __all__ = []
 
+import re
 from subprocess import (
     PIPE,
     Popen,
@@ -198,12 +199,8 @@
             raise PowerError(
                 "Failed to power %s %s: %s" % (
                     power_change, power_address, stdout))
-        if 'on' in stdout:
-            return 'on'
-        elif 'off' in stdout:
-            return 'off'
-        else:
-            return stdout
+        match = re.search(":\s*(on|off)", stdout)
+        return stdout if match is None else match.group(1)
 
     def _issue_ipmi_command(
             self, power_change, power_address=None, power_user=None,

=== modified file 'src/provisioningserver/drivers/power/tests/test_ipmi.py'
--- src/provisioningserver/drivers/power/tests/test_ipmi.py	2016-05-12 19:07:37 +0000
+++ src/provisioningserver/drivers/power/tests/test_ipmi.py	2016-05-20 12:40:37 +0000
@@ -186,6 +186,18 @@
             factory.make_name('command'), factory.make_name('power_change'),
             factory.make_name('power_address'))
 
+    def test__issue_ipmipower_command_does_not_mistake_host_for_status(self):
+        popen_mock = self.patch(ipmi_module, 'Popen')
+        process = popen_mock.return_value
+        # "cameron" contains the string "on", but the machine is off.
+        process.communicate.return_value = (b'cameron: off', b'')
+        process.returncode = 0
+        self.assertThat(
+            IPMIPowerDriver._issue_ipmipower_command(
+                factory.make_name('command'), 'query',
+                factory.make_name('address')),
+            Equals("off"))
+
     def test__issue_ipmi_command_issues_power_on(self):
         context = make_context()
         ipmi_chassis_config_command = make_ipmi_chassis_config_command(