IPMITool: Check the boot mode when setting the boot device

This patch is fixing a problem where the ipmitool option "efiboot"
wasn't passed as part of the command to set the boot device for UEFI
nodes causing them to switch back to legacy BIOS.

A FIXME note was left in the code. It seems that ipmitool has a problem
setting the efiboot + persistent options at the same time
(see bug #1611306). It seems multiple options ought to be specified
together, and that works, except for efiboot. Therefore this patch is
using raw bytes to change the boot device when persistent and uefi are
specified, that way it will work for newer and older versions of the
ipmitool utility.

Closes-Bug: #1611306
Change-Id: I11456fb035fe41fe46f1ff04e4d56c9f85b9e19d
(cherry picked from commit 6419cc2118e9d8c613156f6d4aeae5db46aa385d)

3 files changed, 77 insertions, 4 deletions

diff --git a/ironic/drivers/modules/ipmitool.py b/ironic/drivers/modules/ipmitool.py
index 2fb9f84ad..6e4d2379a 100644
--- a/ironic/drivers/modules/ipmitool.py
+++ b/ironic/drivers/modules/ipmitool.py
@@ -55,6 +55,7 @@ from ironic.common import utils
 from ironic.conductor import task_manager
 from ironic.drivers import base
 from ironic.drivers.modules import console_utils
+from ironic.drivers.modules import deploy_utils
 from ironic.drivers import utils as driver_utils
 
 
@@ -134,6 +135,18 @@ ipmitool_command_options = {
 # form regardless of locale.
 IPMITOOL_RETRYABLE_FAILURES = ['insufficient resources for session']
 
+# NOTE(lucasagomes): A mapping for the boot devices and their hexadecimal
+# value. For more information about these values see the "Set System Boot
+# Options Command" section of the link below (page 418)
+# http://www.intel.com/content/www/us/en/servers/ipmi/ipmi-second-gen-interface-spec-v2-rev1-1.html  # noqa
+BOOT_DEVICE_HEXA_MAP = {
+    boot_devices.PXE: '0x04',
+    boot_devices.DISK: '0x08',
+    boot_devices.CDROM: '0x14',
+    boot_devices.BIOS: '0x18',
+    boot_devices.SAFE: '0x0c'
+}
+
 
 def _check_option_support(options):
     """Checks if the specific ipmitool options are supported on host.
@@ -856,8 +869,7 @@ class IPMIManagement(base.ManagementInterface):
                   in :mod:`ironic.common.boot_devices`.
 
         """
-        return [boot_devices.PXE, boot_devices.DISK, boot_devices.CDROM,
-                boot_devices.BIOS, boot_devices.SAFE]
+        return list(BOOT_DEVICE_HEXA_MAP)
 
     @task_manager.require_exclusive_lock
     def set_boot_device(self, task, device, persistent=False):
@@ -896,9 +908,32 @@ class IPMIManagement(base.ManagementInterface):
             # persistent or we do not have admin rights.
             persistent = False
 
-        cmd = "chassis bootdev %s" % device
+        # FIXME(lucasagomes): Older versions of the ipmitool utility
+        # are not able to set the options "efiboot" and "persistent"
+        # at the same time, combining other options seems to work fine,
+        # except efiboot. Newer versions of ipmitool (1.8.17) does fix
+        # this problem but (some) distros still packaging an older version.
+        # To workaround this problem for now we can make use of sending
+        # raw bytes to set the boot device for a node in persistent +
+        # uefi mode, this will work with newer and older versions of the
+        # ipmitool utility. Also see:
+        # https://bugs.launchpad.net/ironic/+bug/1611306
+        boot_mode = deploy_utils.get_boot_mode_for_deploy(task.node)
+        if persistent and boot_mode == 'uefi':
+            raw_cmd = ('0x00 0x08 0x05 0xe0 %s 0x00 0x00 0x00' %
+                       BOOT_DEVICE_HEXA_MAP[device])
+            send_raw(task, raw_cmd)
+            return
+
+        options = []
         if persistent:
-            cmd = cmd + " options=persistent"
+            options.append('persistent')
+        if boot_mode == 'uefi':
+            options.append('efiboot')
+
+        cmd = "chassis bootdev %s" % device
+        if options:
+            cmd = cmd + " options=%s" % ','.join(options)
         driver_info = _parse_driver_info(task.node)
         try:
             out, err = _exec_ipmitool(driver_info, cmd)
diff --git a/ironic/tests/unit/drivers/modules/test_ipmitool.py b/ironic/tests/unit/drivers/modules/test_ipmitool.py
index 834df9686..5d459c278 100644
--- a/ironic/tests/unit/drivers/modules/test_ipmitool.py
+++ b/ironic/tests/unit/drivers/modules/test_ipmitool.py
@@ -40,6 +40,7 @@ from ironic.common import states
 from ironic.common import utils
 from ironic.conductor import task_manager
 from ironic.drivers.modules import console_utils
+from ironic.drivers.modules import deploy_utils
 from ironic.drivers.modules import ipmitool as ipmi
 from ironic.drivers import utils as driver_utils
 from ironic.tests import base
@@ -1794,6 +1795,38 @@ class IPMIToolDriverTestCase(db_base.DbTestCase):
                               self.driver.management.set_boot_device,
                               task, boot_devices.PXE)
 
+    @mock.patch.object(deploy_utils, 'get_boot_mode_for_deploy')
+    @mock.patch.object(ipmi, '_exec_ipmitool', autospec=True)
+    def test_management_interface_set_boot_device_uefi(self, mock_exec,
+                                                       mock_boot_mode):
+        mock_boot_mode.return_value = 'uefi'
+        mock_exec.return_value = [None, None]
+
+        with task_manager.acquire(self.context, self.node.uuid) as task:
+            self.driver.management.set_boot_device(task, boot_devices.PXE)
+
+        mock_calls = [
+            mock.call(self.info, "raw 0x00 0x08 0x03 0x08"),
+            mock.call(self.info, "chassis bootdev pxe options=efiboot")
+        ]
+        mock_exec.assert_has_calls(mock_calls)
+
+    @mock.patch.object(deploy_utils, 'get_boot_mode_for_deploy')
+    @mock.patch.object(ipmi, '_exec_ipmitool', autospec=True)
+    def test_management_interface_set_boot_device_uefi_and_persistent(
+            self, mock_exec, mock_boot_mode):
+        mock_boot_mode.return_value = 'uefi'
+        mock_exec.return_value = [None, None]
+
+        with task_manager.acquire(self.context, self.node.uuid) as task:
+            self.driver.management.set_boot_device(task, boot_devices.PXE,
+                                                   persistent=True)
+        mock_calls = [
+            mock.call(self.info, "raw 0x00 0x08 0x03 0x08"),
+            mock.call(self.info, "raw 0x00 0x08 0x05 0xe0 0x04 0x00 0x00 0x00")
+        ]
+        mock_exec.assert_has_calls(mock_calls)
+
     def test_management_interface_get_supported_boot_devices(self):
         with task_manager.acquire(self.context, self.node.uuid) as task:
             expected = [boot_devices.PXE, boot_devices.DISK,
diff --git a/releasenotes/notes/ipmitool-bootdev-persistent-uefi-b1181a3c82343c8f.yaml b/releasenotes/notes/ipmitool-bootdev-persistent-uefi-b1181a3c82343c8f.yaml
new file mode 100644
index 000000000..a7dc6f7e9
--- /dev/null
+++ b/releasenotes/notes/ipmitool-bootdev-persistent-uefi-b1181a3c82343c8f.yaml
@@ -0,0 +1,5 @@
+---
+fixes:
+  - Fixes a problem where the boot mode (UEFI or BIOS) wasn't checked
+    as part of changing the boot device of a node, making it incorrectly
+    switch from UEFI to Legacy BIOS mode on some hardware models.