Merge lp:~julian-edwards/maas/localboot-amt into lp:~maas-committers/maas/trunk

[Julian Edwards]

Pass a boot_mode option to power templates (which says whether it's a local or a pxe boot) and make the amt template use it. This is to fix a bug with amt where it was always trying to use PXE power-up and resulted in a previously-deployed node failing to power up properly as it tried to PXE.

(This has tested working on my local rig)

[Raphaël Badin (rvb)]

Nicely done! I've tested this on my NUCs as well and it works as expected.

review: Approve

[Julian Edwards (julian-edwards)]

Thanks for the review!

On Tuesday 14 Oct 2014 07:14:39 you wrote:
> Review: Approve
>
> Nicely done! I've tested this on my NUCs as well and it works as expected.

Ta.

> > +# boot_mode tells us whether we're pxe booting or local booting. For
> > local
> > +# booting, the argument to amttool must be empty (NOT 'hd', it doesn't
> > work!). +if [ "$boot_mode" = 'local' ]; then
> > + boot_mode=''
> > +fi
>
> The discrepancy between the passed 'boot_mode' and what the amt command uses
> makes reusing the same variable awkward I think. How about using
> 'boot_mode' as the name of the variable MAAS passes and something else, for
> instance 'amt_boot_mode', for the parameter AMT consumes? I think it would
> make the template more readable.

Sure. I was just trying to keep the template concise.

> > +
> > + # boot_mode is something that tells the template whether this is
> > + # a PXE boot or a local HD boot.
>
> I'd explain that this is something that the power templates can use but that
> it is mostly useful for the power types that don't support having a PXE
> config that says "Boot from local disk." AFAIK, none of the other power
> types need this: they can always PXE boot and they simply use a special PXE
> config when MAAS wants the nodes to boot from local disk.

Actually most power types do support this! IPMI is already using it, but it
uses a fixed config file. It would be nice to drive it from here instead at
some point.

=== modified file 'etc/maas/templates/power/amt.template'
--- etc/maas/templates/power/amt.template	2014-10-01 20:44:21 +0000
+++ etc/maas/templates/power/amt.template	2014-10-14 09:28:38 +0000
@@ -13,6 +13,7 @@
 power_change={{power_change}}
 power_pass={{power_pass}}
 ip_address={{ip_address}}
+boot_mode={{boot_mode}}
 
 # The user specified power_address overrides any automatically determined
 # ip_address.
@@ -20,6 +21,14 @@
   ip_address=$power_address
 fi
 
+# boot_mode tells us whether we're pxe booting or local booting. For local
+# booting, the argument to amttool must be empty (NOT 'hd', it doesn't work!).
+if [ "$boot_mode" = 'local' ]; then
+    amt_boot_mode=''
+else
+    amt_boot_mode=$boot_mode
+fi
+
 
 # Exit with failure message.
 # Parameters: exit code, and error message.
@@ -82,9 +91,9 @@
 }
 
 
-# Power-cycle the machine, and boot it using PXE.
+# Power-cycle the machine, and boot it using the requested boot mode.
 restart() {
-    issue_amt_command powercycle pxe
+    issue_amt_command powercycle "$amt_boot_mode"
 }
 
 
@@ -95,7 +104,7 @@
     for attempts in $(seq 0 10)
     do
         # Issue the AMT command; amttool will prompt for confirmation.
-        yes | issue_amt_command powerup pxe
+        yes | issue_amt_command powerup "$amt_boot_mode"
         if [ "$(query_state)" = 'on' ]
         then
             # Success.  Machine is on.

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2014-10-13 01:40:27 +0000
+++ src/maasserver/models/node.py	2014-10-14 09:28:38 +0000
@@ -1208,6 +1208,14 @@
             if primary_mac is not None:
                 mac = primary_mac.mac_address.get_raw()
                 power_params['mac_address'] = mac
+
+        # boot_mode is something that tells the template whether this is
+        # a PXE boot or a local HD boot.
+        if self.status == NODE_STATUS.DEPLOYED:
+            power_params['boot_mode'] = 'local'
+        else:
+            power_params['boot_mode'] = 'pxe'
+
         return power_params
 
     def get_effective_power_info(self):

=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2014-10-13 01:37:07 +0000
+++ src/maasserver/models/tests/test_node.py	2014-10-14 09:28:38 +0000
@@ -544,6 +544,17 @@
         params = node.get_effective_power_parameters()
         self.assertEqual("qemu://localhost/system", params["power_address"])
 
+    def test_get_effective_power_parameters_sets_local_boot_mode(self):
+        node = factory.make_Node(status=NODE_STATUS.DEPLOYED)
+        params = node.get_effective_power_parameters()
+        self.assertEqual("local", params['boot_mode'])
+
+    def test_get_effective_power_parameters_sets_pxe_boot_mode(self):
+        status = factory.pick_enum(NODE_STATUS, but_not=[NODE_STATUS.DEPLOYED])
+        node = factory.make_Node(status=status)
+        params = node.get_effective_power_parameters()
+        self.assertEqual("pxe", params['boot_mode'])
+
     def test_get_effective_power_info_is_False_for_unset_power_type(self):
         node = factory.make_Node(power_type="")
         self.assertEqual(