Merge ~andreserl/maas:ipmi_boot_type into maas:master

[Andres Rodriguez]

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

[MAAS Lander (maas-lander)]

UNIT TESTS
-b ipmi_boot_type lp:~andreserl/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/1993/console
COMMIT: 0bb7de05ed77b2182abae5ee69e13bab0a0ec073

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b ipmi_boot_type lp:~andreserl/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/1997/console
COMMIT: 8877e0c834ee58bc43e9dc2a3fca09fc72d2c18b

review: Needs Fixing

[Newell Jensen (newell-jensen)]

Need to set a commit message. See inline comments as well. I won't block you on these small changes but they should be pushed before landing.

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b ipmi_boot_type lp:~andreserl/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/1999/console
COMMIT: ff60bc9411fb84e1ea79c058a1364a319cd248f9

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b ipmi_boot_type lp:~andreserl/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/2001/console
COMMIT: 8fc8ae253d4782ee28acf0852ad7efddd9782253

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b ipmi_boot_type lp:~andreserl/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/2002/console
COMMIT: f83ab51731b04997ace525fa43ff0f249b764ca3

review: Needs Fixing

[Newell Jensen (newell-jensen)]

Inline comment below and it looks like there are unit tests for maas_ipmi_autodetect but I don't see any unit test additions for the changes you have introduced. Was there a reason not to add unit tests for them?

review: Needs Information

[MAAS Lander (maas-lander)]

UNIT TESTS
-b ipmi_boot_type lp:~andreserl/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/2003/console
COMMIT: e9ffd0688e6255063b54d79731adde5aa02ffa2b

review: Needs Fixing

[Andres Rodriguez (andreserl)]

The unittests for the BMC are targeted to test the BMC related functions. This change is not BMC related.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b ipmi_boot_type lp:~andreserl/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 8a1cb40c712671dc1ca7dffe7168d1093d2e9fc1

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b ipmi_boot_type lp:~andreserl/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: f5180d0018413904736453dbfe4a7f81d65f98bc

review: Approve

[Newell Jensen (newell-jensen)]

+1 based on your comment about unit tests.

review: Approve

[Andres Rodriguez (andreserl)]

I've added a unittest anyway :). Thanks for the review.

On Wed, Mar 21, 2018 at 2:25 PM, Newell Jensen <email address hidden>
wrote:

> Review: Approve
>
> +1 based on your comment about unit tests.
> --
> https://code.launchpad.net/~andreserl/maas/+git/maas/+merge/341740
> You are reviewing the proposed merge of ~andreserl/maas:ipmi_boot_type
> into maas:master.
>

--
Andres Rodriguez
Engineering Manager, MAAS
Canonical USA, Inc.

[MAAS Lander (maas-lander)]

UNABLE TO START LANDING

STATUS: MISSING COMMIT MESSAGE

diff --git a/src/maasserver/api/tests/test_enlistment.py b/src/maasserver/api/tests/test_enlistment.py
index 370cb1b..5b636fb 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'] = ''
         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 cd53215..a2caa8e 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)
 
@@ -316,6 +318,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 ''
+
+
 def main():
     import argparse
 
@@ -374,12 +383,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 31ef8c3..37f52b6 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,
@@ -718,3 +720,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 = ''
+        # 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 ed1e1ab..84835e1 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,20 @@ IPMI_DRIVER_CHOICES = [
     ]
 
 
+class IPMI_BOOT_TYPE:
+    # DEFAULT used to provide backwards compatibility
+    DEFAULT = ''
+    LEGACY = 'PC-COMPATIBLE'
+    EFI = 'EFI'
+
+
+IPMI_BOOT_TYPE_CHOICES = [
+    [IPMI_BOOT_TYPE.DEFAULT, "Automatic"],
+    [IPMI_BOOT_TYPE.LEGACY, "Legacy boot"],
+    [IPMI_BOOT_TYPE.EFI, "EFI boot"]
+    ]
+
+
 class IPMIPowerDriver(PowerDriver):
 
     name = 'ipmi'
@@ -168,6 +190,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 +212,14 @@ 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.get_env_with_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 == '':
+                tmp_config.write(IPMI_CONFIG)
+            else:
+                tmp_config.write(IPMI_CONFIG_WITH_BOOT_TYPE % 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 +264,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 +301,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 816d52e..6e7a4b9 100644
--- a/src/provisioningserver/drivers/power/tests/test_ipmi.py
+++ b/src/provisioningserver/drivers/power/tests/test_ipmi.py
@@ -25,7 +25,9 @@ from provisioningserver.drivers.power import (
     PowerError,
 )
 from provisioningserver.drivers.power.ipmi import (
+    IPMI_BOOT_TYPE,
     IPMI_CONFIG,
+    IPMI_CONFIG_WITH_BOOT_TYPE,
     IPMI_ERRORS,
     IPMIPowerDriver,
 )
@@ -107,7 +109,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 +328,57 @@ 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.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 bb03e31..8977210 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 ae1017c..e5ce942 100644
--- a/src/provisioningserver/refresh/tests/test_maas_api_helper.py
+++ b/src/provisioningserver/refresh/tests/test_maas_api_helper.py
@@ -354,6 +354,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.