From 8b2ec52e42fc49646309ab07145d1fe5e20ce0bb Mon Sep 17 00:00:00 2001 From: Shivanand Tendulker Date: Wed, 20 Jul 2016 23:52:26 -0700 Subject: [PATCH] Fix iLO drivers inconsistent boot mode default value When no boot mode is explicitly passed to iLO drivers, it picks default boot mode based on series of factors like, pending boot mode setting, UEFI boot mode support on the node. This causes confusion to the users as these factors are node specific and beyond user's control. User expects a predictable behavior when no boot mode is explicitly passed. A new configuration parameter '[ilo]/default_boot_mode' has been added to specify default boot mode. It would be used if no boot mode is explicitly passed to iLO drivers. Change-Id: I4efd28985674bedabe42fe786135255698425321 Closes-Bug: #1604002 --- doc/source/drivers/ilo.rst | 5 +++++ etc/ironic/ironic.conf.sample | 9 +++++++++ ironic/conf/ilo.py | 9 +++++++++ ironic/drivers/modules/ilo/common.py | 12 +++++++++++- .../tests/unit/drivers/modules/ilo/test_common.py | 13 +++++++++++++ ...nsistent-default-boot-mode-ef5a7c56372f89f1.yaml | 12 ++++++++++++ 6 files changed, 59 insertions(+), 1 deletion(-) create mode 100644 releasenotes/notes/ilo-inconsistent-default-boot-mode-ef5a7c56372f89f1.yaml diff --git a/doc/source/drivers/ilo.rst b/doc/source/drivers/ilo.rst index 46172b905e..821553886c 100644 --- a/doc/source/drivers/ilo.rst +++ b/doc/source/drivers/ilo.rst @@ -626,6 +626,11 @@ mode (Legacy BIOS or UEFI). * When boot mode capability is not configured: + - If config variable ``default_boot_mode`` in ``[ilo]`` section of + ironic configuration file is set to either 'bios' or 'uefi', then iLO + drivers use that boot mode for provisioning the baremetal ProLiant + servers. + - If the pending boot mode is set on the node then iLO drivers use that boot mode for provisioning the baremetal ProLiant servers. diff --git a/etc/ironic/ironic.conf.sample b/etc/ironic/ironic.conf.sample index df778417d4..360f2da77e 100644 --- a/etc/ironic/ironic.conf.sample +++ b/etc/ironic/ironic.conf.sample @@ -1331,6 +1331,15 @@ # CA certificate file to validate iLO. (string value) #ca_file = +# Default boot mode to be used in provisioning when +# "boot_mode" capability is not provided in +# the"properties/capabilities" of the node. The default is +# "auto" for backward compatibility. When "auto" is specified, +# default boot mode will be selected based on boot mode +# settings on the system. (string value) +# Allowed values: auto, bios, uefi +#default_boot_mode = auto + [inspector] diff --git a/ironic/conf/ilo.py b/ironic/conf/ilo.py index 172b138d99..abda84cd4c 100644 --- a/ironic/conf/ilo.py +++ b/ironic/conf/ilo.py @@ -80,6 +80,15 @@ opts = [ 'operations')), cfg.StrOpt('ca_file', help=_('CA certificate file to validate iLO.')), + cfg.StrOpt('default_boot_mode', + default='auto', + choices=['auto', 'bios', 'uefi'], + help=_('Default boot mode to be used in provisioning when ' + '"boot_mode" capability is not provided in the' + '"properties/capabilities" of the node. The default is ' + '"auto" for backward compatibility. When "auto" is ' + 'specified, default boot mode will be selected based ' + 'on boot mode settings on the system.')), ] diff --git a/ironic/drivers/modules/ilo/common.py b/ironic/drivers/modules/ilo/common.py index b22e32ece8..368a4d44e4 100644 --- a/ironic/drivers/modules/ilo/common.py +++ b/ironic/drivers/modules/ilo/common.py @@ -437,12 +437,22 @@ def update_boot_mode(task): node = task.node boot_mode = deploy_utils.get_boot_mode_for_deploy(node) - if boot_mode is not None: + # No boot mode found. Check if default_boot_mode is defined + if not boot_mode and (CONF.ilo.default_boot_mode in ['bios', 'uefi']): + boot_mode = CONF.ilo.default_boot_mode + instance_info = node.instance_info + instance_info['deploy_boot_mode'] = boot_mode + node.instance_info = instance_info + node.save() + + # Boot mode is computed, setting it for the deploy + if boot_mode: LOG.debug("Node %(uuid)s boot mode is being set to %(boot_mode)s", {'uuid': node.uuid, 'boot_mode': boot_mode}) set_boot_mode(node, boot_mode) return + # Computing boot mode based on boot mode settings on bare metal LOG.debug("Check pending boot mode for node %s.", node.uuid) ilo_object = get_ilo_object(node) diff --git a/ironic/tests/unit/drivers/modules/ilo/test_common.py b/ironic/tests/unit/drivers/modules/ilo/test_common.py index cde0808a39..d749c50845 100644 --- a/ironic/tests/unit/drivers/modules/ilo/test_common.py +++ b/ironic/tests/unit/drivers/modules/ilo/test_common.py @@ -373,9 +373,22 @@ class IloCommonMethodsTestCase(db_base.DbTestCase): ilo_common.update_boot_mode(task) set_boot_mode_mock.assert_called_once_with(task.node, 'bios') + @mock.patch.object(ilo_common, 'set_boot_mode', spec_set=True, + autospec=True) + def test_update_boot_mode_use_def_boot_mode(self, + set_boot_mode_mock): + self.config(default_boot_mode='bios', group='ilo') + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + ilo_common.update_boot_mode(task) + set_boot_mode_mock.assert_called_once_with(task.node, 'bios') + self.assertEqual('bios', + task.node.instance_info['deploy_boot_mode']) + @mock.patch.object(ilo_common, 'get_ilo_object', spec_set=True, autospec=True) def test_update_boot_mode(self, get_ilo_object_mock): + self.config(default_boot_mode="auto", group='ilo') ilo_mock_obj = get_ilo_object_mock.return_value ilo_mock_obj.get_pending_boot_mode.return_value = 'LEGACY' diff --git a/releasenotes/notes/ilo-inconsistent-default-boot-mode-ef5a7c56372f89f1.yaml b/releasenotes/notes/ilo-inconsistent-default-boot-mode-ef5a7c56372f89f1.yaml new file mode 100644 index 0000000000..310053107f --- /dev/null +++ b/releasenotes/notes/ilo-inconsistent-default-boot-mode-ef5a7c56372f89f1.yaml @@ -0,0 +1,12 @@ +--- +fixes: + - When no boot mode is explicitly set on a node using + an iLO driver, ironic automatically picks a boot + mode based on hardware capabilities. This confuses + deployers, as these factors are system specific and + not configurable. In order to ensure predictable + behavior, a new configuration parameter, + ``[ilo]/default_boot_mode``, was added to allow + deployers to explicitly set a default. The default + value of this option keeps behavior consistent for + existing deployments.