Merge lp:~allenap/maas/remove-ipmi-optimisation into lp:~maas-committers/maas/trunk

[Gavin Panella]

Formally revert to old IPMI power control behaviour: do not check the current state before issuing a power command.

[Raphaël Badin (rvb)]

Looks good to me.

[0]

30 +# This script used to get the current power state, then only send a power
31 +# command if a change was needed. See bug 1171418 for an explanation.

It sounds a bit weird to only say what *used to* happen here. I think a short line explaining that we do not check the current power state and why would be welcome.

review: Approve

[Raphaël Badin (rvb)]

I've added Andres as a reviewer for this as he's the one who know this IPMI stuff best.

[Raphaël Badin (rvb)]

I'm building a package from this branch to test it in the lab.

[Raphaël Badin (rvb)]

> I'm building a package from this branch to test it in the lab.

Tests passed: http://10.189.74.2:8080/view/MAAS/job/raring-adt-maas-manual/31/ARCH=amd64,label=lenovo-RD230-01/console

[Raphaël Badin (rvb)]

After discussing the matter with the team we decided to land this. But we will ping Andres so he could have a look at it. If he thinks we should change this, we will do it in a follow-up branch.

[Andres Rodriguez (andreserl)]

So I have tested this in hardware and have not been able to reproduce.

What I believe is happening here, however, is that the integration tests run too fast in comparison to how fast the BMC actually reacts to the power changes. For example:

Say we tell a node to 'stop' and immediately after to 'start'. Say this is happening in 2 microseconds. However, the BMC takes 5 seconds to actually turn off the node. So by the time the 'start' command is executed with the ipmi.template, the --stat shows that the node is on because the BMC hasn't turn it off yet (say it's in second 2 out of 5 the BMC takes to turn off the node), hence, causing this causes the script to eventually not turn the node on.

I do believe that the fix here is an improvement that is necessary to ensure that a node gets turned off or turned on regardless of its current power status. However, this does *not* break MAAS, nor makes it unusable. I have tested this several ways in my local Hardware Environment and have not been able to reproduce this.

However, I'm happy to fix this in Raring though! I'll SRU as soon as raring is out.

=== modified file 'src/provisioningserver/power/templates/ipmi.template'
--- src/provisioningserver/power/templates/ipmi.template	2013-04-18 19:30:26 +0000
+++ src/provisioningserver/power/templates/ipmi.template	2013-04-22 16:26:27 +0000
@@ -45,7 +45,7 @@
     driver_option=""
     if [ -n "$power_driver" ]
         then
-          driver_option="--driver-type=$power_driver"
+          driver_option="--driver-type=${power_driver}"
     fi
 
     echo workaround |\
@@ -54,16 +54,8 @@
     ${ipmipower} ${driver_option} -h ${power_address} -u ${power_user} -p ${power_pass} "$@"
 }
 
-
-# Get the given system's power state: 'on' or 'off'.
-get_power_state() {
-    ipmi_state=$(issue_ipmi_command --stat)
-    formulate_power_state ${ipmi_state}
-}
-
-
-if [ "$(get_power_state)" != "${power_change}" ]
-then
-    power_command=$(formulate_power_command ${power_change})
-    issue_ipmi_command ${power_command}
-fi
+# This script deliberately does not check the current power state
+# before issuing the requested power command. See bug 1171418 for an
+# explanation.
+power_command=$(formulate_power_command ${power_change})
+issue_ipmi_command ${power_command}

=== modified file 'src/provisioningserver/power/tests/test_poweraction.py'
--- src/provisioningserver/power/tests/test_poweraction.py	2013-03-15 11:47:04 +0000
+++ src/provisioningserver/power/tests/test_poweraction.py	2013-04-22 16:26:27 +0000
@@ -180,16 +180,6 @@
         output = action.run_shell(script)
         self.assertIn("Got unknown power state from fence_cdu", output)
 
-    def test_ipmi_checks_state(self):
-        action = PowerAction(POWER_TYPE.IPMI)
-        script = action.render_template(
-            action.get_template(), power_change='on',
-            power_address='mystystem', power_user='me', power_pass='me',
-            ipmipower='echo', ipmi_chassis_config='echo', config_dir='dir',
-            ipmi_config='file.conf', power_driver='LAN')
-        output = action.run_shell(script)
-        self.assertIn("Got unknown power state from ipmipower", output)
-
     def configure_power_config_dir(self, path=None):
         """Configure POWER_CONFIG_DIR to `path`."""
         self.patch(