From 152a3654ce0b03c70a489919611eeaec343cc8ea Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Thu, 6 Apr 2017 15:48:54 +0000 Subject: [PATCH] Logic for skipping deployment with BFV In order to boot from volume, the deploy driver needs to know when not to actually deploy. This change wires in the checks to skip deployment activities if it appears that we have a valid root volume target defined. Co-Authored-By: Hironori Shiina Partial-Bug: #1559691 Change-Id: I62e622a2b053f685c2da42ca5106bdb9bdd22dc6 --- ironic/drivers/modules/agent.py | 47 ++++++++++---- ironic/drivers/modules/iscsi_deploy.py | 27 ++++++-- ironic/drivers/modules/pxe.py | 43 ++++++++++--- ironic/drivers/modules/storage/cinder.py | 15 +++-- .../drivers/modules/storage/test_cinder.py | 16 +++++ .../tests/unit/drivers/modules/test_agent.py | 36 ++++++++++- .../unit/drivers/modules/test_iscsi_deploy.py | 50 +++++++++++++++ ironic/tests/unit/drivers/modules/test_pxe.py | 61 +++++++++++++++++++ 8 files changed, 265 insertions(+), 30 deletions(-) diff --git a/ironic/drivers/modules/agent.py b/ironic/drivers/modules/agent.py index 8d6c4b719b..32d0b249e6 100644 --- a/ironic/drivers/modules/agent.py +++ b/ironic/drivers/modules/agent.py @@ -330,10 +330,17 @@ class AgentDeploy(AgentDeployMixin, base.DeployInterface): task.driver.boot.validate(task) node = task.node + + # Validate node capabilities + deploy_utils.validate_capabilities(node) + + if not task.driver.storage.should_write_image(task): + # NOTE(TheJulia): There is no reason to validate + # image properties if we will not be writing an image + # in a boot from volume case. As such, return to the caller. + return + params = {} - # TODO(jtaryma): Skip validation of image_source if - # task.driver.storage.should_write_image() - # returns False. image_source = node.instance_info.get('image_source') params['instance_info.image_source'] = image_source error_msg = _('Node %s failed to validate deploy image info. Some ' @@ -358,9 +365,6 @@ class AgentDeploy(AgentDeployMixin, base.DeployInterface): '%(node)s. Error: %(error)s') % {'node': node.uuid, 'error': e}) - # Validate node capabilities - deploy_utils.validate_capabilities(node) - validate_image_proxies(node) @METRICS.timer('AgentDeploy.deploy') @@ -376,8 +380,21 @@ class AgentDeploy(AgentDeployMixin, base.DeployInterface): :param task: a TaskManager instance. :returns: status of the deploy. One of ironic.common.states. """ - manager_utils.node_power_action(task, states.REBOOT) - return states.DEPLOYWAIT + if task.driver.storage.should_write_image(task): + manager_utils.node_power_action(task, states.REBOOT) + return states.DEPLOYWAIT + else: + # TODO(TheJulia): At some point, we should de-dupe this code + # as it is nearly identical to the iscsi deploy interface. + # This is not being done now as it is expected to be + # refactored in the near future. + manager_utils.node_power_action(task, states.POWER_OFF) + task.driver.network.remove_provisioning_network(task) + task.driver.network.configure_tenant_networks(task) + task.driver.boot.prepare_instance(task) + manager_utils.node_power_action(task, states.POWER_ON) + LOG.info('Deployment to node %s done', task.node.uuid) + return states.DEPLOYDONE @METRICS.timer('AgentDeploy.tear_down') @task_manager.require_exclusive_lock @@ -425,14 +442,20 @@ class AgentDeploy(AgentDeployMixin, base.DeployInterface): # Adding the node to provisioning network so that the dhcp # options get added for the provisioning port. manager_utils.node_power_action(task, states.POWER_OFF) - # NOTE(vdrok): in case of rebuild, we have tenant network already - # configured, unbind tenant ports if present - task.driver.network.unconfigure_tenant_networks(task) - task.driver.network.add_provisioning_network(task) + if task.driver.storage.should_write_image(task): + # NOTE(vdrok): in case of rebuild, we have tenant network + # already configured, unbind tenant ports if present + task.driver.network.unconfigure_tenant_networks(task) + task.driver.network.add_provisioning_network(task) # Signal to storage driver to attach volumes task.driver.storage.attach_volumes(task) if node.provision_state == states.ACTIVE: + # Call is due to conductor takeover task.driver.boot.prepare_instance(task) + elif not task.driver.storage.should_write_image(task): + # We have nothing else to do as this is handled in the + # backend storage system. + return elif node.provision_state != states.ADOPTING: node.instance_info = deploy_utils.build_instance_info_for_deploy( task) diff --git a/ironic/drivers/modules/iscsi_deploy.py b/ironic/drivers/modules/iscsi_deploy.py index cf150c5fce..c8a257693c 100644 --- a/ironic/drivers/modules/iscsi_deploy.py +++ b/ironic/drivers/modules/iscsi_deploy.py @@ -424,6 +424,11 @@ class ISCSIDeploy(AgentDeployMixin, base.DeployInterface): # Check the boot_mode, boot_option and disk_label capabilities values. deploy_utils.validate_capabilities(node) + # Edit early if we are not writing a volume as the validate + # tasks evaluate root device hints. + if not task.driver.storage.should_write_image(task): + return + # TODO(rameshg87): iscsi_ilo driver uses this method. Remove # and copy-paste it's contents here once iscsi_ilo deploy driver # broken down into separate boot and deploy implementations. @@ -443,12 +448,24 @@ class ISCSIDeploy(AgentDeployMixin, base.DeployInterface): :returns: deploy state DEPLOYWAIT. """ node = task.node - cache_instance_image(task.context, node) - check_image_size(task) + if task.driver.storage.should_write_image(task): + cache_instance_image(task.context, node) + check_image_size(task) + manager_utils.node_power_action(task, states.REBOOT) - manager_utils.node_power_action(task, states.REBOOT) - - return states.DEPLOYWAIT + return states.DEPLOYWAIT + else: + # TODO(TheJulia): At some point, we should de-dupe this code + # as it is nearly identical to the agent deploy interface. + # This is not being done now as it is expected to be + # refactored in the near future. + manager_utils.node_power_action(task, states.POWER_OFF) + task.driver.network.remove_provisioning_network(task) + task.driver.network.configure_tenant_networks(task) + manager_utils.node_power_action(task, states.POWER_ON) + task.process_event('done') + LOG.info('Deployment to node %s done', node.uuid) + return states.DEPLOYDONE @METRICS.timer('ISCSIDeploy.tear_down') @task_manager.require_exclusive_lock diff --git a/ironic/drivers/modules/pxe.py b/ironic/drivers/modules/pxe.py index 435e6ee763..833e20ae7c 100644 --- a/ironic/drivers/modules/pxe.py +++ b/ironic/drivers/modules/pxe.py @@ -419,6 +419,11 @@ class PXEBoot(base.BootInterface): validate_boot_parameters_for_trusted_boot(node) _parse_driver_info(node) + # NOTE(TheJulia): If we're not writing an image, we can skip + # the remainder of this method. + if not task.driver.storage.should_write_image(task): + return + d_info = deploy_utils.get_image_instance_info(node) if (node.driver_internal_info.get('is_whole_disk_image') or deploy_utils.get_boot_option(node) == 'local'): @@ -530,12 +535,34 @@ class PXEBoot(base.BootInterface): boot_option = deploy_utils.get_boot_option(node) boot_device = None - if boot_option != "local": - # Make sure that the instance kernel/ramdisk is cached. - # This is for the takeover scenario for active nodes. - instance_image_info = _get_instance_image_info( - task.node, task.context) - _cache_ramdisk_kernel(task.context, task.node, instance_image_info) + if deploy_utils.is_iscsi_boot(task): + dhcp_opts = pxe_utils.dhcp_options_for_instance(task) + provider = dhcp_factory.DHCPFactory() + provider.update_dhcp(task, dhcp_opts) + + # configure iPXE for iscsi boot + pxe_config_path = pxe_utils.get_pxe_config_file_path( + task.node.uuid) + if not os.path.isfile(pxe_config_path): + pxe_options = _build_pxe_config_options(task, {}) + pxe_config_template = ( + deploy_utils.get_pxe_config_template(node)) + pxe_utils.create_pxe_config( + task, pxe_options, pxe_config_template) + deploy_utils.switch_pxe_config( + pxe_config_path, None, + deploy_utils.get_boot_mode_for_deploy(node), False, + iscsi_boot=True) + boot_device = boot_devices.PXE + + elif boot_option != "local": + if task.driver.storage.should_write_image(task): + # Make sure that the instance kernel/ramdisk is cached. + # This is for the takeover scenario for active nodes. + instance_image_info = _get_instance_image_info( + task.node, task.context) + _cache_ramdisk_kernel(task.context, task.node, + instance_image_info) # If it's going to PXE boot we need to update the DHCP server dhcp_opts = pxe_utils.dhcp_options_for_instance(task) @@ -548,7 +575,9 @@ class PXEBoot(base.BootInterface): 'root_uuid_or_disk_id' ] except KeyError: - if not iwdi: + if not task.driver.storage.should_write_image(task): + pass + elif not iwdi: LOG.warning("The UUID for the root partition can't be " "found, unable to switch the pxe config from " "deployment mode to service (boot) mode for " diff --git a/ironic/drivers/modules/storage/cinder.py b/ironic/drivers/modules/storage/cinder.py index 7c797e4c44..fed9068c51 100644 --- a/ironic/drivers/modules/storage/cinder.py +++ b/ironic/drivers/modules/storage/cinder.py @@ -352,15 +352,20 @@ class CinderStorage(base.StorageInterface): def should_write_image(self, task): """Determines if deploy should perform the image write-out. - NOTE: This method will be written as part of the changes to the - boot logic, and for now should always return false to indicate - that the deployment image write-out process should be skipped. - :param task: The task object. :returns: True if the deployment write-out process should be executed. """ - return False + # NOTE(TheJulia): There is no reason to check if a root volume + # exists here because if the validation has already been passed + # then we know that there should be a volume. If there is an + # image_source, then we should expect to write it out. + instance_info = task.node.instance_info + if 'image_source' not in instance_info: + for volume in task.volume_targets: + if volume['boot_index'] == 0: + return False + return True def _generate_connector(self, task): """Generate cinder connector value based upon the node. diff --git a/ironic/tests/unit/drivers/modules/storage/test_cinder.py b/ironic/tests/unit/drivers/modules/storage/test_cinder.py index 696a035ebb..a92c474bf3 100644 --- a/ironic/tests/unit/drivers/modules/storage/test_cinder.py +++ b/ironic/tests/unit/drivers/modules/storage/test_cinder.py @@ -590,3 +590,19 @@ class CinderInterfaceTestCase(db_base.DbTestCase): self.assertEqual(1, mock_log.error.call_count) # CONF.cinder.action_retries + 1, number of retries is set to 3. self.assertEqual(4, mock_detach.call_count) + + def test_should_write_image(self): + self.node = object_utils.create_test_node(self.context, + storage_interface='cinder') + object_utils.create_test_volume_target( + self.context, node_id=self.node.id, volume_type='iscsi', + boot_index=0, volume_id='1234') + + with task_manager.acquire(self.context, self.node.id) as task: + self.assertFalse(self.interface.should_write_image(task)) + + self.node.instance_info = {'image_source': 'fake-value'} + self.node.save() + + with task_manager.acquire(self.context, self.node.id) as task: + self.assertTrue(self.interface.should_write_image(task)) diff --git a/ironic/tests/unit/drivers/modules/test_agent.py b/ironic/tests/unit/drivers/modules/test_agent.py index abecf466c5..a7e637ebaf 100644 --- a/ironic/tests/unit/drivers/modules/test_agent.py +++ b/ironic/tests/unit/drivers/modules/test_agent.py @@ -256,13 +256,47 @@ class TestAgentDeploy(db_base.DbTestCase): task.driver.boot, task) show_mock.assert_called_once_with(self.context, 'fake-image') + @mock.patch.object(pxe.PXEBoot, 'validate', autospec=True) + @mock.patch.object(deploy_utils, 'check_for_missing_params', + autospec=True) + @mock.patch.object(deploy_utils, 'validate_capabilities', autospec=True) + @mock.patch.object(noop_storage.NoopStorage, 'should_write_image', + autospec=True) + def test_validate_storage_should_write_image_false(self, mock_write, + mock_capabilities, + mock_params, + mock_pxe_validate): + mock_write.return_value = False + with task_manager.acquire(self.context, self.node.uuid, + shared=True) as task: + + self.driver.validate(task) + mock_capabilities.assert_called_once_with(task.node) + self.assertFalse(mock_params.called) + + @mock.patch.object(pxe.PXEBoot, 'prepare_instance', autospec=True) @mock.patch('ironic.conductor.utils.node_power_action', autospec=True) - def test_deploy(self, power_mock): + def test_deploy(self, power_mock, mock_pxe_instance): with task_manager.acquire( self.context, self.node['uuid'], shared=False) as task: driver_return = self.driver.deploy(task) self.assertEqual(driver_return, states.DEPLOYWAIT) power_mock.assert_called_once_with(task, states.REBOOT) + self.assertFalse(mock_pxe_instance.called) + + @mock.patch.object(pxe.PXEBoot, 'prepare_instance', autospec=True) + @mock.patch.object(noop_storage.NoopStorage, 'should_write_image', + autospec=True) + def test_deploy_storage_should_write_image_false(self, mock_write, + mock_pxe_instance): + mock_write.return_value = False + self.node.provision_state = states.DEPLOYING + self.node.save() + with task_manager.acquire( + self.context, self.node['uuid'], shared=False) as task: + driver_return = self.driver.deploy(task) + self.assertEqual(driver_return, states.DEPLOYDONE) + self.assertTrue(mock_pxe_instance.called) @mock.patch.object(noop_storage.NoopStorage, 'detach_volumes', autospec=True) diff --git a/ironic/tests/unit/drivers/modules/test_iscsi_deploy.py b/ironic/tests/unit/drivers/modules/test_iscsi_deploy.py index 243f4b45ee..9e9489c02e 100644 --- a/ironic/tests/unit/drivers/modules/test_iscsi_deploy.py +++ b/ironic/tests/unit/drivers/modules/test_iscsi_deploy.py @@ -568,6 +568,27 @@ class ISCSIDeployTestCase(db_base.DbTestCase): validate_capabilities_mock.assert_called_once_with(task.node) validate_mock.assert_called_once_with(task) + @mock.patch.object(noop_storage.NoopStorage, 'should_write_image', + autospec=True) + @mock.patch.object(iscsi_deploy, 'validate', autospec=True) + @mock.patch.object(deploy_utils, 'validate_capabilities', autospec=True) + @mock.patch.object(pxe.PXEBoot, 'validate', autospec=True) + def test_validate_storage_should_write_image_false( + self, pxe_validate_mock, + validate_capabilities_mock, validate_mock, + should_write_image_mock): + should_write_image_mock.return_value = False + with task_manager.acquire(self.context, self.node.uuid, + shared=True) as task: + + task.driver.deploy.validate(task) + + pxe_validate_mock.assert_called_once_with(task.driver.boot, task) + validate_capabilities_mock.assert_called_once_with(task.node) + self.assertFalse(validate_mock.called) + should_write_image_mock.assert_called_once_with( + task.driver.storage, task) + @mock.patch.object(noop_storage.NoopStorage, 'attach_volumes', autospec=True) @mock.patch.object(deploy_utils, 'populate_storage_driver_internal_info', @@ -647,6 +668,35 @@ class ISCSIDeployTestCase(db_base.DbTestCase): mock_check_image_size.assert_called_once_with(task) mock_node_power_action.assert_called_once_with(task, states.REBOOT) + @mock.patch.object(noop_storage.NoopStorage, 'should_write_image', + autospec=True) + @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.' + 'configure_tenant_networks', spec_set=True, autospec=True) + @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.' + 'remove_provisioning_network', spec_set=True, autospec=True) + @mock.patch.object(manager_utils, 'node_power_action', autospec=True) + @mock.patch.object(iscsi_deploy, 'check_image_size', autospec=True) + @mock.patch.object(iscsi_deploy, 'cache_instance_image', autospec=True) + def test_deploy_storage_check_write_image_false(self, + mock_cache_instance_image, + mock_check_image_size, + mock_node_power_action, + mock_remove_network, + mock_tenant_network, + mock_write): + mock_write.return_value = False + self.node.provision_state = states.DEPLOYING + self.node.save() + with task_manager.acquire(self.context, + self.node.uuid, shared=False) as task: + state = task.driver.deploy.deploy(task) + self.assertEqual(state, states.DEPLOYDONE) + self.assertFalse(mock_cache_instance_image.called) + self.assertFalse(mock_check_image_size.called) + mock_remove_network.assert_called_once_with(mock.ANY, task) + mock_tenant_network.assert_called_once_with(mock.ANY, task) + self.assertEqual(2, mock_node_power_action.call_count) + @mock.patch.object(noop_storage.NoopStorage, 'detach_volumes', autospec=True) @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.' diff --git a/ironic/tests/unit/drivers/modules/test_pxe.py b/ironic/tests/unit/drivers/modules/test_pxe.py index fe5de609fb..bb94b087cc 100644 --- a/ironic/tests/unit/drivers/modules/test_pxe.py +++ b/ironic/tests/unit/drivers/modules/test_pxe.py @@ -35,6 +35,7 @@ from ironic.conductor import task_manager from ironic.drivers.modules import agent_base_vendor from ironic.drivers.modules import deploy_utils from ironic.drivers.modules import pxe +from ironic.drivers.modules.storage import noop as noop_storage from ironic.tests.unit.conductor import mgr_utils from ironic.tests.unit.db import base as db_base from ironic.tests.unit.db import utils as db_utils @@ -697,6 +698,18 @@ class PXEBootTestCase(db_base.DbTestCase): task.node.driver_internal_info['is_whole_disk_image'] = True task.driver.boot.validate(task) + @mock.patch.object(base_image_service.BaseImageService, '_show', + autospec=True) + @mock.patch.object(noop_storage.NoopStorage, 'should_write_image', + autospec=True) + def test_validate_skip_check_write_image_false(self, mock_write, + mock_glance): + mock_write.return_value = False + with task_manager.acquire(self.context, self.node.uuid, + shared=True) as task: + task.driver.boot.validate(task) + self.assertFalse(mock_glance.called) + def test_validate_fail_missing_deploy_kernel(self): with task_manager.acquire(self.context, self.node.uuid, shared=True) as task: @@ -1077,6 +1090,54 @@ class PXEBootTestCase(db_base.DbTestCase): self.assertFalse(switch_pxe_config_mock.called) self.assertFalse(set_boot_device_mock.called) + @mock.patch('os.path.isfile', return_value=False, autospec=True) + @mock.patch.object(pxe_utils, 'create_pxe_config', autospec=True) + @mock.patch.object(deploy_utils, 'is_iscsi_boot', autospec=True) + @mock.patch.object(noop_storage.NoopStorage, 'should_write_image', + autospec=True) + @mock.patch.object(deploy_utils, 'try_set_boot_device', autospec=True) + @mock.patch.object(deploy_utils, 'switch_pxe_config', autospec=True) + @mock.patch.object(dhcp_factory, 'DHCPFactory', autospec=True) + @mock.patch.object(pxe, '_cache_ramdisk_kernel', autospec=True) + @mock.patch.object(pxe, '_get_instance_image_info', autospec=True) + def test_prepare_instance_netboot_iscsi( + self, get_image_info_mock, cache_mock, + dhcp_factory_mock, switch_pxe_config_mock, + set_boot_device_mock, should_write_image_mock, + is_iscsi_boot_mock, create_pxe_config_mock, isfile_mock): + http_url = 'http://192.1.2.3:1234' + self.config(ipxe_enabled=True, group='pxe') + self.config(http_url=http_url, group='deploy') + is_iscsi_boot_mock.return_value = True + should_write_image_mock.return_valule = False + provider_mock = mock.MagicMock() + dhcp_factory_mock.return_value = provider_mock + vol_id = uuidutils.generate_uuid() + obj_utils.create_test_volume_target( + self.context, node_id=self.node.id, volume_type='iscsi', + boot_index=0, volume_id='1234', uuid=vol_id, + properties={'target_lun': 0, + 'target_portal': 'fake_host:3260', + 'target_iqn': 'fake_iqn', + 'auth_username': 'fake_username', + 'auth_password': 'fake_password'}) + with task_manager.acquire(self.context, self.node.uuid) as task: + task.node.driver_internal_info = { + 'boot_from_volume': vol_id} + dhcp_opts = pxe_utils.dhcp_options_for_instance(task) + pxe_config_path = pxe_utils.get_pxe_config_file_path( + task.node.uuid) + task.driver.boot.prepare_instance(task) + self.assertFalse(get_image_info_mock.called) + self.assertFalse(cache_mock.called) + provider_mock.update_dhcp.assert_called_once_with(task, dhcp_opts) + create_pxe_config_mock.assert_called_once_with( + task, mock.ANY, CONF.pxe.pxe_config_template) + switch_pxe_config_mock.assert_called_once_with( + pxe_config_path, None, None, False, iscsi_boot=True) + set_boot_device_mock.assert_called_once_with(task, + boot_devices.PXE) + @mock.patch.object(deploy_utils, 'try_set_boot_device', autospec=True) @mock.patch.object(pxe_utils, 'clean_up_pxe_config', autospec=True) def test_prepare_instance_localboot(self, clean_up_pxe_config_mock,