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,