From 954a23482082f3dcfb57cfbb71e5d61f05c7fbce Mon Sep 17 00:00:00 2001 From: Jim Rollenhagen Date: Thu, 1 Feb 2018 09:00:16 -0500 Subject: [PATCH] Remove mode argument from boot.(prepare|clean_up)_ramdisk Ideally, the boot interface shouldn't care if it's booting an image for deploy or rescue. The first step to unwinding this is not passing the mode argument into the boot interface - for now, we infer it from the state. Also stop validating whether the boot interface methods have a mode argument, as this is totally broken anyway (due to the decorator on the method). The rest of the boot interface's knowledge about deploy vs rescue can be eliminated in the future, as we shouldn't rework too much of that during feature freeze. We fix the bug in validation this way for now, for two reasons: * This argument really doesn't belong, and it's only been on master for six days. Get it out before people use it. * Library updates are currently frozen; fixing the decorator isn't an option right now. Change-Id: Icdeb870e9fd9cf836ff7ab97624189c8436adaba Closes-Bug: #1746730 --- ironic/drivers/base.py | 12 ++--------- ironic/drivers/modules/agent.py | 17 ++------------- ironic/drivers/modules/deploy_utils.py | 5 +++++ ironic/drivers/modules/ilo/boot.py | 9 ++------ ironic/drivers/modules/irmc/boot.py | 9 ++------ ironic/drivers/modules/pxe.py | 9 ++++---- .../unit/drivers/modules/ilo/test_boot.py | 15 ++++++------- .../unit/drivers/modules/irmc/test_boot.py | 4 ++-- .../tests/unit/drivers/modules/test_agent.py | 21 +++---------------- ironic/tests/unit/drivers/modules/test_pxe.py | 9 +++++--- 10 files changed, 35 insertions(+), 75 deletions(-) diff --git a/ironic/drivers/base.py b/ironic/drivers/base.py index cfc284e1bc..d6bfe37305 100644 --- a/ironic/drivers/base.py +++ b/ironic/drivers/base.py @@ -433,7 +433,7 @@ class BootInterface(BaseInterface): interface_type = 'boot' @abc.abstractmethod - def prepare_ramdisk(self, task, ramdisk_params, mode='deploy'): + def prepare_ramdisk(self, task, ramdisk_params): """Prepares the boot of Ironic ramdisk. This method prepares the boot of the deploy or rescue ramdisk after @@ -450,25 +450,17 @@ class BootInterface(BaseInterface): Other implementations can make use of ramdisk_params to pass such information. Different implementations of boot interface will have different ways of passing parameters to the ramdisk. - :param mode: Label indicating a deploy or rescue operation - being carried out on the node. Supported values are 'deploy' and - 'rescue'. Defaults to 'deploy', indicating deploy operation is - being carried out. :returns: None """ @abc.abstractmethod - def clean_up_ramdisk(self, task, mode='deploy'): + def clean_up_ramdisk(self, task): """Cleans up the boot of ironic ramdisk. This method cleans up the environment that was setup for booting the deploy or rescue ramdisk. :param task: a task from TaskManager. - :param mode: Label indicating a deploy or rescue operation - was carried out on the node. Supported values are 'deploy' and - 'rescue'. Defaults to 'deploy', indicating deploy operation was - carried out. :returns: None """ diff --git a/ironic/drivers/modules/agent.py b/ironic/drivers/modules/agent.py index bb147bd1b3..511747c98d 100644 --- a/ironic/drivers/modules/agent.py +++ b/ironic/drivers/modules/agent.py @@ -15,7 +15,6 @@ from ironic_lib import metrics_utils from ironic_lib import utils as il_utils from oslo_log import log -from oslo_utils import reflection from oslo_utils import units import six.moves.urllib_parse as urlparse @@ -738,7 +737,7 @@ class AgentRescue(base.RescueInterface): if CONF.agent.manage_agent_boot: ramdisk_opts = deploy_utils.build_agent_options(task.node) # prepare_ramdisk will set the boot device - task.driver.boot.prepare_ramdisk(task, ramdisk_opts, mode='rescue') + task.driver.boot.prepare_ramdisk(task, ramdisk_opts) manager_utils.node_power_action(task, states.POWER_ON) return states.RESCUEWAIT @@ -775,9 +774,6 @@ class AgentRescue(base.RescueInterface): has an invalid value when 'neutron' network is used. :raises: MissingParameterValue if node is missing one or more required parameters - :raises: IncompatibleInterface if 'prepare_ramdisk' and - 'clean_up_ramdisk' of node's boot interface do not support 'mode' - argument. """ node = task.node missing_params = [] @@ -787,15 +783,6 @@ class AgentRescue(base.RescueInterface): task.driver.network.get_rescuing_network_uuid(task) if CONF.agent.manage_agent_boot: - if ('mode' not in reflection.get_signature( - task.driver.boot.prepare_ramdisk).parameters or - 'mode' not in reflection.get_signature( - task.driver.boot.clean_up_ramdisk).parameters): - raise exception.IncompatibleInterface( - interface_type='boot', - interface_impl="of 'prepare_ramdisk' and/or " - "'clean_up_ramdisk' with 'mode' argument", - hardware_type=node.driver) # TODO(stendulker): boot.validate() performs validation of # provisioning related parameters which is not required during # rescue operation. @@ -833,5 +820,5 @@ class AgentRescue(base.RescueInterface): """ manager_utils.remove_node_rescue_password(task.node, save=True) if CONF.agent.manage_agent_boot: - task.driver.boot.clean_up_ramdisk(task, mode='rescue') + task.driver.boot.clean_up_ramdisk(task) task.driver.network.remove_rescuing_network(task) diff --git a/ironic/drivers/modules/deploy_utils.py b/ironic/drivers/modules/deploy_utils.py index 250aa738e3..b3b6bf20fb 100644 --- a/ironic/drivers/modules/deploy_utils.py +++ b/ironic/drivers/modules/deploy_utils.py @@ -124,6 +124,11 @@ def get_ironic_api_url(): return ironic_api +def rescue_or_deploy_mode(node): + return ('rescue' if node.provision_state in RESCUE_LIKE_STATES + else 'deploy') + + def discovery(portal_address, portal_port): """Do iSCSI discovery on portal.""" utils.execute('iscsiadm', diff --git a/ironic/drivers/modules/ilo/boot.py b/ironic/drivers/modules/ilo/boot.py index a4acecc80e..85388fe4d8 100644 --- a/ironic/drivers/modules/ilo/boot.py +++ b/ironic/drivers/modules/ilo/boot.py @@ -603,7 +603,7 @@ class IloVirtualMediaBoot(base.BootInterface): class IloPXEBoot(pxe.PXEBoot): @METRICS.timer('IloPXEBoot.prepare_ramdisk') - def prepare_ramdisk(self, task, ramdisk_params, mode='deploy'): + def prepare_ramdisk(self, task, ramdisk_params): """Prepares the boot of Ironic ramdisk using PXE. This method prepares the boot of the deploy or rescue ramdisk after @@ -612,10 +612,6 @@ class IloPXEBoot(pxe.PXEBoot): :param task: a task from TaskManager. :param ramdisk_params: the parameters to be passed to the ramdisk. - :param mode: Label indicating a deploy or rescue operation - being carried out on the node. Supported values are - 'deploy' and 'rescue'. Defaults to 'deploy', indicating - deploy operation is being carried out. :returns: None :raises: MissingParameterValue, if some information is missing in node's driver_info or instance_info. @@ -629,8 +625,7 @@ class IloPXEBoot(pxe.PXEBoot): if task.node.provision_state in (states.DEPLOYING, states.RESCUING): prepare_node_for_deploy(task) - super(IloPXEBoot, self).prepare_ramdisk(task, ramdisk_params, - mode=mode) + super(IloPXEBoot, self).prepare_ramdisk(task, ramdisk_params) @METRICS.timer('IloPXEBoot.prepare_instance') def prepare_instance(self, task): diff --git a/ironic/drivers/modules/irmc/boot.py b/ironic/drivers/modules/irmc/boot.py index 1c246bf869..3d4cd06598 100644 --- a/ironic/drivers/modules/irmc/boot.py +++ b/ironic/drivers/modules/irmc/boot.py @@ -1040,7 +1040,7 @@ class IRMCPXEBoot(pxe.PXEBoot): """iRMC PXE boot.""" @METRICS.timer('IRMCPXEBoot.prepare_ramdisk') - def prepare_ramdisk(self, task, ramdisk_params, mode='deploy'): + def prepare_ramdisk(self, task, ramdisk_params): """Prepares the boot of Ironic ramdisk using PXE. This method prepares the boot of the deploy kernel/ramdisk after @@ -1051,10 +1051,6 @@ class IRMCPXEBoot(pxe.PXEBoot): :param ramdisk_params: the parameters to be passed to the ramdisk. pxe driver passes these parameters as kernel command-line arguments. - :param mode: Label indicating a deploy or rescue operation - being carried out on the node. Supported values are - 'deploy' and 'rescue'. Defaults to 'deploy', indicating - deploy operation is being carried out. :returns: None :raises: MissingParameterValue, if some information is missing in node's driver_info or instance_info. @@ -1068,8 +1064,7 @@ class IRMCPXEBoot(pxe.PXEBoot): if task.node.provision_state == states.DEPLOYING: irmc_management.backup_bios_config(task) - super(IRMCPXEBoot, self).prepare_ramdisk(task, ramdisk_params, - mode=mode) + super(IRMCPXEBoot, self).prepare_ramdisk(task, ramdisk_params) @METRICS.timer('IRMCPXEBoot.prepare_instance') def prepare_instance(self, task): diff --git a/ironic/drivers/modules/pxe.py b/ironic/drivers/modules/pxe.py index 8a820a6ea4..c43558377f 100644 --- a/ironic/drivers/modules/pxe.py +++ b/ironic/drivers/modules/pxe.py @@ -234,8 +234,7 @@ def _build_pxe_config_options(task, pxe_info, service=False): template. """ node = task.node - mode = ('rescue' if node.provision_state in deploy_utils.RESCUE_LIKE_STATES - else 'deploy') + mode = deploy_utils.rescue_or_deploy_mode(node) if service: pxe_options = {} elif (node.driver_internal_info.get('boot_from_volume') and @@ -469,7 +468,7 @@ class PXEBoot(base.BootInterface): deploy_utils.validate_image_properties(task.context, d_info, props) @METRICS.timer('PXEBoot.prepare_ramdisk') - def prepare_ramdisk(self, task, ramdisk_params, mode='deploy'): + def prepare_ramdisk(self, task, ramdisk_params): """Prepares the boot of Ironic ramdisk using PXE. This method prepares the boot of the deploy or rescue kernel/ramdisk @@ -493,6 +492,7 @@ class PXEBoot(base.BootInterface): operation failed on the node. """ node = task.node + mode = deploy_utils.rescue_or_deploy_mode(node) if CONF.pxe.ipxe_enabled: # NOTE(mjturek): At this point, the ipxe boot script should @@ -536,7 +536,7 @@ class PXEBoot(base.BootInterface): _cache_ramdisk_kernel(task.context, node, pxe_info) @METRICS.timer('PXEBoot.clean_up_ramdisk') - def clean_up_ramdisk(self, task, mode='deploy'): + def clean_up_ramdisk(self, task): """Cleans up the boot of ironic ramdisk. This method cleans up the PXE environment that was setup for booting @@ -552,6 +552,7 @@ class PXEBoot(base.BootInterface): :returns: None """ node = task.node + mode = deploy_utils.rescue_or_deploy_mode(node) try: images_info = _get_image_info(node, mode=mode) except exception.MissingParameterValue as e: diff --git a/ironic/tests/unit/drivers/modules/ilo/test_boot.py b/ironic/tests/unit/drivers/modules/ilo/test_boot.py index 25d97c5a42..1bce8f432c 100644 --- a/ironic/tests/unit/drivers/modules/ilo/test_boot.py +++ b/ironic/tests/unit/drivers/modules/ilo/test_boot.py @@ -1102,33 +1102,30 @@ class IloPXEBootTestCase(db_base.DbTestCase): self.assertFalse(prepare_node_mock.called) pxe_prepare_ramdisk_mock.assert_called_once_with( - mock.ANY, task, None, mode='deploy') + mock.ANY, task, None) @mock.patch.object(ilo_boot, 'prepare_node_for_deploy', spec_set=True, autospec=True) @mock.patch.object(pxe.PXEBoot, 'prepare_ramdisk', spec_set=True, autospec=True) def _test_prepare_ramdisk_needs_node_prep(self, pxe_prepare_ramdisk_mock, - prepare_node_mock, prov_state, - mode): + prepare_node_mock, prov_state): self.node.provision_state = prov_state self.node.save() with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: self.assertIsNone( - task.driver.boot.prepare_ramdisk(task, None, mode=mode)) + task.driver.boot.prepare_ramdisk(task, None)) prepare_node_mock.assert_called_once_with(task) pxe_prepare_ramdisk_mock.assert_called_once_with( - mock.ANY, task, None, mode=mode) + mock.ANY, task, None) def test_prepare_ramdisk_in_deploying(self): - self._test_prepare_ramdisk_needs_node_prep(prov_state=states.DEPLOYING, - mode='deploy') + self._test_prepare_ramdisk_needs_node_prep(prov_state=states.DEPLOYING) def test_prepare_ramdisk_in_rescuing(self): - self._test_prepare_ramdisk_needs_node_prep(prov_state=states.RESCUING, - mode='rescue') + self._test_prepare_ramdisk_needs_node_prep(prov_state=states.RESCUING) @mock.patch.object(deploy_utils, 'is_iscsi_boot', spec_set=True, autospec=True) diff --git a/ironic/tests/unit/drivers/modules/irmc/test_boot.py b/ironic/tests/unit/drivers/modules/irmc/test_boot.py index 6606a23b65..af6fa25213 100644 --- a/ironic/tests/unit/drivers/modules/irmc/test_boot.py +++ b/ironic/tests/unit/drivers/modules/irmc/test_boot.py @@ -1230,7 +1230,7 @@ class IRMCPXEBootTestCase(db_base.DbTestCase): task.driver.boot.prepare_ramdisk(task, {}) mock_backup_bios.assert_called_once_with(task) mock_parent_prepare.assert_called_once_with( - task.driver.boot, task, {}, mode='deploy') + task.driver.boot, task, {}) @mock.patch.object(irmc_management, 'backup_bios_config', spec_set=True, autospec=True) @@ -1245,7 +1245,7 @@ class IRMCPXEBootTestCase(db_base.DbTestCase): task.driver.boot.prepare_ramdisk(task, {}) self.assertFalse(mock_backup_bios.called) mock_parent_prepare.assert_called_once_with( - task.driver.boot, task, {}, mode='deploy') + task.driver.boot, task, {}) @mock.patch.object(irmc_common, 'set_secure_boot_mode', spec_set=True, autospec=True) diff --git a/ironic/tests/unit/drivers/modules/test_agent.py b/ironic/tests/unit/drivers/modules/test_agent.py index 1d86c0487e..29380805e5 100644 --- a/ironic/tests/unit/drivers/modules/test_agent.py +++ b/ironic/tests/unit/drivers/modules/test_agent.py @@ -16,7 +16,6 @@ import types import mock from oslo_config import cfg -from oslo_utils import reflection from ironic.common import dhcp_factory from ironic.common import exception @@ -1281,7 +1280,7 @@ class AgentRescueTestCase(db_base.DbTestCase): mock_add_rescue_net.assert_called_once_with(mock.ANY, task) mock_build_agent_opts.assert_called_once_with(task.node) mock_prepare_ramdisk.assert_called_once_with( - mock.ANY, task, {'ipa-api-url': 'fake-api'}, mode='rescue') + mock.ANY, task, {'ipa-api-url': 'fake-api'}) self.assertEqual(states.RESCUEWAIT, result) @mock.patch.object(flat_network.FlatNetwork, 'add_rescuing_network', @@ -1329,7 +1328,7 @@ class AgentRescueTestCase(db_base.DbTestCase): [mock.call(task, states.POWER_OFF), mock.call(task, states.POWER_ON)]) mock_clean_ramdisk.assert_called_once_with( - mock.ANY, task, mode='rescue') + mock.ANY, task) mock_remove_rescue_net.assert_called_once_with(mock.ANY, task) mock_conf_tenant_net.assert_called_once_with(mock.ANY, task) mock_prepare_instance.assert_called_once_with(mock.ANY, task) @@ -1463,20 +1462,6 @@ class AgentRescueTestCase(db_base.DbTestCase): self.assertFalse(mock_rescuing_net.called) mock_boot_validate.assert_called_once_with(mock.ANY, task) - @mock.patch('ironic.drivers.modules.network.neutron.NeutronNetwork.' - 'get_rescuing_network_uuid', autospec=True) - @mock.patch.object(reflection, 'get_signature', autospec=True) - @mock.patch.object(fake.FakeBoot, 'validate', autospec=True) - def test_agent_rescue_validate_incompat_exc(self, mock_boot_validate, - mock_get_signature, - mock_rescuing_net): - mock_get_signature.return_value.parameters = ['task'] - with task_manager.acquire(self.context, self.node.uuid) as task: - self.assertRaises(exception.IncompatibleInterface, - task.driver.rescue.validate, task) - self.assertFalse(mock_rescuing_net.called) - self.assertFalse(mock_boot_validate.called) - @mock.patch.object(flat_network.FlatNetwork, 'remove_rescuing_network', spec_set=True, autospec=True) @mock.patch.object(fake.FakeBoot, 'clean_up_ramdisk', autospec=True) @@ -1486,7 +1471,7 @@ class AgentRescueTestCase(db_base.DbTestCase): task.driver.rescue.clean_up(task) self.assertNotIn('rescue_password', task.node.instance_info) mock_clean_ramdisk.assert_called_once_with( - mock.ANY, task, mode='rescue') + mock.ANY, task) mock_remove_rescue_net.assert_called_once_with(mock.ANY, task) @mock.patch.object(flat_network.FlatNetwork, 'remove_rescuing_network', diff --git a/ironic/tests/unit/drivers/modules/test_pxe.py b/ironic/tests/unit/drivers/modules/test_pxe.py index 2d95c61e13..454d508c5d 100644 --- a/ironic/tests/unit/drivers/modules/test_pxe.py +++ b/ironic/tests/unit/drivers/modules/test_pxe.py @@ -916,11 +916,10 @@ class PXEBootTestCase(db_base.DbTestCase): mock_deploy_img_info.return_value = { 'rescue_kernel': 'a', 'rescue_ramdisk': 'r'} - self.node.provision_state = states.RESCUING self.node.save() with task_manager.acquire(self.context, self.node.uuid) as task: dhcp_opts = pxe_utils.dhcp_options_for_instance(task) - task.driver.boot.prepare_ramdisk(task, {'foo': 'bar'}, mode=mode) + task.driver.boot.prepare_ramdisk(task, {'foo': 'bar'}) mock_deploy_img_info.assert_called_once_with(task.node, mode=mode) provider_mock.update_dhcp.assert_called_once_with(task, dhcp_opts) set_boot_device_mock.assert_called_once_with(task, @@ -1078,14 +1077,18 @@ class PXEBootTestCase(db_base.DbTestCase): image_info = {kernel_label: ['', '/path/to/' + kernel_label], ramdisk_label: ['', '/path/to/' + ramdisk_label]} get_image_info_mock.return_value = image_info - task.driver.boot.clean_up_ramdisk(task, mode=mode) + task.driver.boot.clean_up_ramdisk(task) clean_up_pxe_env_mock.assert_called_once_with(task, image_info) get_image_info_mock.assert_called_once_with(task.node, mode=mode) def test_clean_up_ramdisk(self): + self.node.provision_state = states.DEPLOYING + self.node.save() self._test_clean_up_ramdisk() def test_clean_up_ramdisk_rescue(self): + self.node.provision_state = states.RESCUING + self.node.save() self._test_clean_up_ramdisk(mode='rescue') @mock.patch.object(manager_utils, 'node_set_boot_device', autospec=True)