Merge ~andreserl/maas:2.3_ipmi_boot_type into maas:2.3

[Andres Rodriguez]

Backport c70ed068a and 84a9fd27e

LP: #1750622 - Add ability to force the BIOS boot method, and auto discover it during enlistment/commissioning

9dffe2f... by Andres Rodriguez on 2018-04-04

Revert change thatcannot be backported

[Blake Rouse (blake-rouse)]

Looks like a clean backport to me.

review: Approve

diff --git a/src/maasserver/api/tests/test_enlistment.py b/src/maasserver/api/tests/test_enlistment.py
index db951af..74e6218 100644
--- a/src/maasserver/api/tests/test_enlistment.py
+++ b/src/maasserver/api/tests/test_enlistment.py
@@ -115,6 +115,7 @@ class EnlistmentAPITest(APITestCase.ForAnonymousAndUserAndAdmin):
         # Add the default values.
         power_parameters['power_driver'] = 'LAN_2_0'
         power_parameters['mac_address'] = ''
+        power_parameters['power_boot_type'] = 'auto'
         self.assertEqual(http.client.OK, response.status_code)
         [machine] = Machine.objects.filter(hostname=hostname)
         self.assertEqual(power_parameters, machine.power_parameters)
diff --git a/src/metadataserver/user_data/templates/snippets/maas_ipmi_autodetect.py b/src/metadataserver/user_data/templates/snippets/maas_ipmi_autodetect.py
index 49a9424..e2c3ce5 100644
--- a/src/metadataserver/user_data/templates/snippets/maas_ipmi_autodetect.py
+++ b/src/metadataserver/user_data/templates/snippets/maas_ipmi_autodetect.py
@@ -263,16 +263,18 @@ def commit_ipmi_settings(config):
     run_command(('bmc-config', '--commit', '--filename', config))
 
 
-def get_maas_power_settings(user, password, ipaddress, version):
-    return "%s,%s,%s,%s" % (user, password, ipaddress, version)
+def get_maas_power_settings(user, password, ipaddress, version, boot_type):
+    return "%s,%s,%s,%s,%s" % (user, password, ipaddress, version, boot_type)
 
 
-def get_maas_power_settings_json(user, password, ipaddress, version):
+def get_maas_power_settings_json(
+        user, password, ipaddress, version, boot_type):
     power_params = {
         "power_address": ipaddress,
         "power_pass": password,
         "power_user": user,
         "power_driver": version,
+        "power_boot_type": boot_type,
     }
     return json.dumps(power_params)
 
@@ -320,6 +322,13 @@ def bmc_supports_lan2_0():
     return False
 
 
+def get_system_boot_type():
+    """Detect if the system has boot EFI."""
+    if os.path.isdir('/sys/firmware/efi'):
+        return 'efi'
+    return 'auto'
+
+
 def main():
     import argparse
 
@@ -378,12 +387,17 @@ def main():
         IPMI_VERSION = "LAN_2_0"
     else:
         IPMI_VERSION = "LAN"
+
+    IPMI_BOOT_TYPE = get_system_boot_type()
+
     if args.commission_creds:
         print(get_maas_power_settings_json(
-            IPMI_MAAS_USER, IPMI_MAAS_PASSWORD, IPMI_IP_ADDRESS, IPMI_VERSION))
+            IPMI_MAAS_USER, IPMI_MAAS_PASSWORD, IPMI_IP_ADDRESS,
+            IPMI_VERSION, IPMI_BOOT_TYPE))
     else:
         print(get_maas_power_settings(
-            IPMI_MAAS_USER, IPMI_MAAS_PASSWORD, IPMI_IP_ADDRESS, IPMI_VERSION))
+            IPMI_MAAS_USER, IPMI_MAAS_PASSWORD, IPMI_IP_ADDRESS,
+            IPMI_VERSION, IPMI_BOOT_TYPE))
 
 if __name__ == '__main__':
     main()
diff --git a/src/metadataserver/user_data/templates/snippets/tests/test_maas_ipmi_autodetect.py b/src/metadataserver/user_data/templates/snippets/tests/test_maas_ipmi_autodetect.py
index e79740b..9d5fc50 100644
--- a/src/metadataserver/user_data/templates/snippets/tests/test_maas_ipmi_autodetect.py
+++ b/src/metadataserver/user_data/templates/snippets/tests/test_maas_ipmi_autodetect.py
@@ -6,6 +6,7 @@
 __all__ = []
 
 from collections import OrderedDict
+import os.path
 import platform
 import re
 import subprocess
@@ -31,6 +32,7 @@ from snippets.maas_ipmi_autodetect import (
     format_user_key,
     generate_random_password,
     get_ipmi_ip_address,
+    get_system_boot_type,
     IPMIError,
     list_user_numbers,
     make_ipmi_user_settings,
@@ -722,3 +724,24 @@ class TestEnableLanChannel(MAASTestCase):
             subprocess.CalledProcessError, bmc_set_mock)
         self.assertThat(
             bmc_set_mock.call_args_list, HasLength(1))
+
+
+class TestGetSystemBootType(MAASTestCase):
+
+    def test_get_system_boot_type_efi(self):
+        """Test that returns ."""
+        boot_type = 'efi'
+        # path os.path.isdir to return True to simulate
+        # that /sys/firmware/efi exists.
+        self.patch(os.path, "isdir").return_value = True
+        actual_boot_type = get_system_boot_type()
+        self.assertEqual(boot_type, actual_boot_type)
+
+    def test_get_system_boot_type_non_efi(self):
+        """Test """
+        boot_type = 'auto'
+        # path os.path.isdir to return False to simulate
+        # that /sys/firmware/efi doesn't exist.
+        self.patch(os.path, "isdir").return_value = False
+        actual_boot_type = get_system_boot_type()
+        self.assertEqual(boot_type, actual_boot_type)
diff --git a/src/provisioningserver/drivers/power/ipmi.py b/src/provisioningserver/drivers/power/ipmi.py
index 9ef5ba4..99b3e6e 100644
--- a/src/provisioningserver/drivers/power/ipmi.py
+++ b/src/provisioningserver/drivers/power/ipmi.py
@@ -39,6 +39,14 @@ EndSection
 """
 
 
+IPMI_CONFIG_WITH_BOOT_TYPE = """\
+Section Chassis_Boot_Flags
+        Boot_Flags_Persistent                         No
+        BIOS_Boot_Type                                %s
+        Boot_Device                                   PXE
+EndSection
+"""
+
 IPMI_ERRORS = {
     'username invalid': {
         'message': (
@@ -159,6 +167,26 @@ IPMI_DRIVER_CHOICES = [
     ]
 
 
+class IPMI_BOOT_TYPE:
+    # DEFAULT used to provide backwards compatibility
+    DEFAULT = 'auto'
+    LEGACY = 'legacy'
+    EFI = 'efi'
+
+
+IPMI_BOOT_TYPE_CHOICES = [
+    [IPMI_BOOT_TYPE.DEFAULT, "Automatic"],
+    [IPMI_BOOT_TYPE.LEGACY, "Legacy boot"],
+    [IPMI_BOOT_TYPE.EFI, "EFI boot"]
+    ]
+
+
+IPMI_BOOT_TYPE_MAPPING = {
+    IPMI_BOOT_TYPE.EFI: 'EFI',
+    IPMI_BOOT_TYPE.LEGACY: 'PC-COMPATIBLE',
+    }
+
+
 class IPMIPowerDriver(PowerDriver):
 
     name = 'ipmi'
@@ -168,6 +196,11 @@ class IPMIPowerDriver(PowerDriver):
             'power_driver', "Power driver", field_type='choice',
             choices=IPMI_DRIVER_CHOICES, default=IPMI_DRIVER.LAN_2_0,
             required=True),
+        make_setting_field(
+            'power_boot_type', "Power boot type", field_type='choice',
+            choices=IPMI_BOOT_TYPE_CHOICES, default=IPMI_BOOT_TYPE.DEFAULT,
+            required=False
+            ),
         make_setting_field('power_address', "IP address", required=True),
         make_setting_field('power_user', "Power user"),
         make_setting_field(
@@ -185,11 +218,17 @@ class IPMIPowerDriver(PowerDriver):
 
     @staticmethod
     def _issue_ipmi_chassis_config_command(
-            command, power_change, power_address):
+            command, power_change, power_address, power_boot_type=None):
         env = shell.select_c_utf8_locale()
         with NamedTemporaryFile("w+", encoding="utf-8") as tmp_config:
             # Write out the chassis configuration.
-            tmp_config.write(IPMI_CONFIG)
+            if (power_boot_type is None or
+                    power_boot_type == IPMI_BOOT_TYPE.DEFAULT):
+                tmp_config.write(IPMI_CONFIG)
+            else:
+                tmp_config.write(
+                    IPMI_CONFIG_WITH_BOOT_TYPE %
+                    IPMI_BOOT_TYPE_MAPPING[power_boot_type])
             tmp_config.flush()
             # Use it when running the chassis config command.
             # XXX: Not using call_and_check here because we
@@ -234,7 +273,7 @@ class IPMIPowerDriver(PowerDriver):
     def _issue_ipmi_command(
             self, power_change, power_address=None, power_user=None,
             power_pass=None, power_driver=None, power_off_mode=None,
-            mac_address=None, **extra):
+            mac_address=None, power_boot_type=None, **extra):
         """Issue command to ipmipower, for the given system."""
         # This script deliberately does not check the current power state
         # before issuing the requested power command. See bug 1171418 for an
@@ -271,7 +310,8 @@ class IPMIPowerDriver(PowerDriver):
         # Before changing state run the chassis config command.
         if power_change in ("on", "off"):
             self._issue_ipmi_chassis_config_command(
-                ipmi_chassis_config_command, power_change, power_address)
+                ipmi_chassis_config_command, power_change, power_address,
+                power_boot_type)
 
         # Additional arguments for the power command.
         if power_change == 'on':
diff --git a/src/provisioningserver/drivers/power/tests/test_ipmi.py b/src/provisioningserver/drivers/power/tests/test_ipmi.py
index bc5d3fb..2172704 100644
--- a/src/provisioningserver/drivers/power/tests/test_ipmi.py
+++ b/src/provisioningserver/drivers/power/tests/test_ipmi.py
@@ -25,7 +25,10 @@ from provisioningserver.drivers.power import (
     PowerError,
 )
 from provisioningserver.drivers.power.ipmi import (
+    IPMI_BOOT_TYPE,
+    IPMI_BOOT_TYPE_MAPPING,
     IPMI_CONFIG,
+    IPMI_CONFIG_WITH_BOOT_TYPE,
     IPMI_ERRORS,
     IPMIPowerDriver,
 )
@@ -107,7 +110,9 @@ class TestIPMIPowerDriver(MAASTestCase):
         # The IP address is passed to _issue_ipmi_chassis_config_command.
         self.assertThat(
             driver._issue_ipmi_chassis_config_command,
-            MockCalledOnceWith(ANY, power_change, ip_address))
+            MockCalledOnceWith(
+                ANY, power_change, ip_address,
+                power_boot_type=None))
         # The IP address is also within the command passed to
         # _issue_ipmi_chassis_config_command.
         self.assertThat(
@@ -324,3 +329,58 @@ class TestIPMIPowerDriver(MAASTestCase):
 
         self.assertThat(
             _issue_ipmi_command_mock, MockCalledOnceWith('query', **context))
+
+    def test__issue_ipmi_chassis_config_with_power_boot_type(self):
+        context = make_context()
+        driver = IPMIPowerDriver()
+        ip_address = factory.make_ipv4_address()
+        find_ip_via_arp = self.patch(ipmi_module, 'find_ip_via_arp')
+        find_ip_via_arp.return_value = ip_address
+        power_change = random.choice(("on", "off"))
+
+        context['mac_address'] = factory.make_mac_address()
+        context['power_address'] = random.choice((None, "", "   "))
+        context['power_boot_type'] = IPMI_BOOT_TYPE.EFI
+
+        self.patch_autospec(driver, "_issue_ipmi_chassis_config_command")
+        self.patch_autospec(driver, "_issue_ipmipower_command")
+        driver._issue_ipmi_command(power_change, **context)
+
+        # The IP address is passed to _issue_ipmi_chassis_config_command.
+        self.assertThat(
+            driver._issue_ipmi_chassis_config_command,
+            MockCalledOnceWith(
+                ANY, power_change, ip_address,
+                power_boot_type=IPMI_BOOT_TYPE.EFI))
+        # The IP address is also within the command passed to
+        # _issue_ipmi_chassis_config_command.
+        self.assertThat(
+            driver._issue_ipmi_chassis_config_command.call_args[0],
+            Contains(ip_address))
+        # The IP address is passed to _issue_ipmipower_command.
+        self.assertThat(
+            driver._issue_ipmipower_command,
+            MockCalledOnceWith(ANY, power_change, ip_address))
+
+    def test__chassis_config_written_to_temporary_file_with_boot_type(self):
+        boot_type = self.patch(ipmi_module, "power_boot_type")
+        boot_type.return_value = IPMI_BOOT_TYPE.EFI
+        NamedTemporaryFile = self.patch(ipmi_module, "NamedTemporaryFile")
+        tmpfile = NamedTemporaryFile.return_value
+        tmpfile.__enter__.return_value = tmpfile
+        tmpfile.name = factory.make_name("filename")
+
+        IPMIPowerDriver._issue_ipmi_chassis_config_command(
+            ["true"], sentinel.change, sentinel.addr,
+            power_boot_type=IPMI_BOOT_TYPE.EFI)
+
+        self.assertThat(
+            NamedTemporaryFile, MockCalledOnceWith("w+", encoding="utf-8"))
+        self.assertThat(tmpfile.__enter__, MockCalledOnceWith())
+        self.assertThat(
+            tmpfile.write,
+            MockCalledOnceWith(
+                IPMI_CONFIG_WITH_BOOT_TYPE % IPMI_BOOT_TYPE_MAPPING[
+                    IPMI_BOOT_TYPE.EFI]))
+        self.assertThat(tmpfile.flush, MockCalledOnceWith())
+        self.assertThat(tmpfile.__exit__, MockCalledOnceWith(None, None, None))
diff --git a/src/provisioningserver/refresh/maas_api_helper.py b/src/provisioningserver/refresh/maas_api_helper.py
index cfcd6a4..94f9cf0 100644
--- a/src/provisioningserver/refresh/maas_api_helper.py
+++ b/src/provisioningserver/refresh/maas_api_helper.py
@@ -234,7 +234,11 @@ def signal(
 
     if None not in (power_type, power_params):
         params[b'power_type'] = power_type.encode('utf-8')
-        user, power_pass, power_address, driver = power_params.split(",")
+        if power_type == 'moonshot':
+            user, power_pass, power_address, driver = power_params.split(",")
+        else:
+            (user, power_pass, power_address,
+                driver, boot_type) = power_params.split(",")
         # OrderedDict is used to make testing easier.
         power_params = OrderedDict([
             ('power_user', user),
@@ -245,6 +249,7 @@ def signal(
             power_params['power_hwaddress'] = driver
         else:
             power_params['power_driver'] = driver
+            power_params['power_boot_type'] = boot_type
         params[b'power_parameters'] = json.dumps(power_params).encode()
 
     data, headers = encode_multipart_data(
diff --git a/src/provisioningserver/refresh/tests/test_maas_api_helper.py b/src/provisioningserver/refresh/tests/test_maas_api_helper.py
index 72f6341..cdaaa8f 100644
--- a/src/provisioningserver/refresh/tests/test_maas_api_helper.py
+++ b/src/provisioningserver/refresh/tests/test_maas_api_helper.py
@@ -334,6 +334,7 @@ class TestSignal(MAASTestCase):
             ('power_pass', factory.make_name('power_pass')),
             ('power_address', factory.make_url()),
             ('power_driver', factory.make_name('power_driver')),
+            ('power_boot_type', factory.make_name('power_boot_type')),
         ])
 
         # None used for url and creds as we're not actually sending data.