From d7a5b3469cfb5c16e122451e9cd8594d7a7671a1 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Tue, 22 Jun 2021 19:12:02 +0200 Subject: [PATCH] Fix ramdisk boot option handling The current ramdisk deploy code expects a user to set the boot_option capability to "ramdisk". Not only is it redundant, it's also not documented, so chances are high nobody has ever done that. As a side effect of e6bb99cd8ff94a24c3f2108e649c7ca39189d220 boot interfaces no longer validate kernel/ramdisk/image if boot option is "local". Unless a user explicitly sets boot option to "ramdisk", the validation will be skipped for the ramdisk deploy. This patch follows the pattern of the anaconda deploy and makes get_boot_option always return "ramdisk" for the ramdisk deploy. In the future we need to refactor this code so that the deploy interface provides the boot option it works with, but that is a lot of changes. Change-Id: I25c28df048c706f0c5b013b4d252f09d5a7e57bd --- ironic/drivers/modules/deploy_utils.py | 10 ++++++++++ ironic/drivers/modules/pxe.py | 13 ------------- ironic/tests/unit/drivers/modules/test_pxe.py | 18 ------------------ 3 files changed, 10 insertions(+), 31 deletions(-) diff --git a/ironic/drivers/modules/deploy_utils.py b/ironic/drivers/modules/deploy_utils.py index 3575a7732c..dc57c85a42 100644 --- a/ironic/drivers/modules/deploy_utils.py +++ b/ironic/drivers/modules/deploy_utils.py @@ -588,11 +588,21 @@ def get_boot_option(node): return 'local' if is_anaconda_deploy(node): return 'kickstart' + if is_ramdisk_deploy(node): + return 'ramdisk' capabilities = utils.parse_instance_info_capabilities(node) return capabilities.get('boot_option', CONF.deploy.default_boot_option).lower() +# FIXME(dtantsur): relying on deploy interface name is an anti-pattern. +# Refactor the code so that the deploy interface itself provides the only boot +# option it supports. + +def is_ramdisk_deploy(node): + return node.deploy_interface == 'ramdisk' + + def is_anaconda_deploy(node): """Determine if Anaconda deploy interface is in use for the deployment. diff --git a/ironic/drivers/modules/pxe.py b/ironic/drivers/modules/pxe.py index 07b54acae5..a91a33e5bb 100644 --- a/ironic/drivers/modules/pxe.py +++ b/ironic/drivers/modules/pxe.py @@ -90,19 +90,6 @@ class PXERamdiskDeploy(agent_base.AgentBaseMixin, agent_base.HeartbeatMixin, @task_manager.require_exclusive_lock def prepare(self, task): node = task.node - # Log a warning if the boot_option is wrong... and - # otherwise reset it. - boot_option = deploy_utils.get_boot_option(node) - if boot_option != 'ramdisk': - LOG.warning('Incorrect "boot_option" set for node %(node)s ' - 'and will be overridden to "ramdisk" as to ' - 'match the deploy interface. Found: %(boot_opt)s.', - {'node': node.uuid, - 'boot_opt': boot_option}) - i_info = task.node.instance_info - i_info.update({'capabilities': {'boot_option': 'ramdisk'}}) - node.instance_info = i_info - node.save() deploy_utils.populate_storage_driver_internal_info(task) if node.provision_state == states.DEPLOYING: diff --git a/ironic/tests/unit/drivers/modules/test_pxe.py b/ironic/tests/unit/drivers/modules/test_pxe.py index 0ad0249b05..7498f20628 100644 --- a/ironic/tests/unit/drivers/modules/test_pxe.py +++ b/ironic/tests/unit/drivers/modules/test_pxe.py @@ -941,8 +941,6 @@ class PXERamdiskDeployTestCase(db_base.DbTestCase): with task_manager.acquire(self.context, node.uuid) as task: task.driver.deploy.prepare(task) self.assertFalse(mock_prepare_instance.called) - self.assertEqual({'boot_option': 'ramdisk'}, - task.node.instance_info['capabilities']) @mock.patch.object(pxe.PXEBoot, 'prepare_instance', autospec=True) def test_prepare_active(self, mock_prepare_instance): @@ -962,22 +960,6 @@ class PXERamdiskDeployTestCase(db_base.DbTestCase): task.driver.deploy.prepare(task) mock_prepare_instance.assert_called_once_with(mock.ANY, task) - @mock.patch.object(pxe.LOG, 'warning', autospec=True) - @mock.patch.object(pxe.PXEBoot, 'prepare_instance', autospec=True) - def test_prepare_fixes_and_logs_boot_option_warning( - self, mock_prepare_instance, mock_warning): - node = self.node - node.properties['capabilities'] = 'boot_option:ramdisk' - node.provision_state = states.DEPLOYING - node.instance_info = {} - node.save() - with task_manager.acquire(self.context, node.uuid) as task: - task.driver.deploy.prepare(task) - self.assertFalse(mock_prepare_instance.called) - self.assertEqual({'boot_option': 'ramdisk'}, - task.node.instance_info['capabilities']) - self.assertTrue(mock_warning.called) - @mock.patch.object(deploy_utils, 'validate_image_properties', autospec=True) def test_validate(self, mock_validate_img):