diff --git a/ironic/drivers/modules/agent.py b/ironic/drivers/modules/agent.py index 308eb568ad..a4f77356de 100644 --- a/ironic/drivers/modules/agent.py +++ b/ironic/drivers/modules/agent.py @@ -298,14 +298,12 @@ class AgentDeploy(base.DeployInterface): :returns: A list of clean step dictionaries """ - steps = deploy_utils.agent_get_clean_steps(task) - if CONF.deploy.erase_devices_priority is not None: - for step in steps: - if (step.get('step') == 'erase_devices' and - step.get('interface') == 'deploy'): - # Override with operator set priority - step['priority'] = CONF.deploy.erase_devices_priority - return steps + new_priorities = { + 'erase_devices': CONF.deploy.erase_devices_priority, + } + return deploy_utils.agent_get_clean_steps( + task, interface='deploy', + override_priorities=new_priorities) def execute_clean_step(self, task, step): """Execute a clean step asynchronously on the agent. diff --git a/ironic/drivers/modules/deploy_utils.py b/ironic/drivers/modules/deploy_utils.py index 0ab811e4c5..237f3dccae 100644 --- a/ironic/drivers/modules/deploy_utils.py +++ b/ironic/drivers/modules/deploy_utils.py @@ -988,15 +988,22 @@ def parse_instance_info_capabilities(node): return capabilities -def agent_get_clean_steps(task): +def agent_get_clean_steps(task, interface=None, override_priorities=None): """Get the list of clean steps from the agent. #TODO(JoshNang) move to BootInterface :param task: a TaskManager object containing the node + :param interface: The interface for which clean steps + are to be returned. If this is not provided, it returns the + clean steps for all interfaces. + :param override_priorities: a dictionary with keys being step names and + values being new priorities for them. If a step isn't in this + dictionary, the step's original priority is used. :raises: NodeCleaningFailure if the agent returns invalid results :returns: A list of clean step dictionaries """ + override_priorities = override_priorities or {} client = agent_client.AgentClient() ports = objects.Port.list_by_node_id( task.context, task.node.id) @@ -1019,10 +1026,16 @@ def agent_get_clean_steps(task): steps_list = [step for step_list in result['clean_steps'].values() for step in step_list] - # Filter steps to only return deploy steps - steps = [step for step in steps_list - if step.get('interface') == 'deploy'] - return steps + result = [] + for step in steps_list: + if interface and step.get('interface') != interface: + continue + new_priority = override_priorities.get(step.get('step')) + if new_priority is not None: + step['priority'] = new_priority + result.append(step) + + return result def agent_execute_clean_step(task, step): diff --git a/ironic/drivers/modules/ilo/deploy.py b/ironic/drivers/modules/ilo/deploy.py index 788dff2016..5dad6ec2b5 100644 --- a/ironic/drivers/modules/ilo/deploy.py +++ b/ironic/drivers/modules/ilo/deploy.py @@ -664,15 +664,12 @@ class IloVirtualMediaAgentDeploy(base.DeployInterface): :param task: a TaskManager object containing the node :returns: A list of clean step dictionaries """ - steps = deploy_utils.agent_get_clean_steps(task) - if CONF.ilo.clean_priority_erase_devices is not None: - for step in steps: - if (step.get('step') == 'erase_devices' and - step.get('interface') == 'deploy'): - # Override with operator set priority - step['priority'] = CONF.ilo.clean_priority_erase_devices - - return steps + new_priorities = { + 'erase_devices': CONF.ilo.clean_priority_erase_devices, + } + return deploy_utils.agent_get_clean_steps( + task, interface='deploy', + override_priorities=new_priorities) def execute_clean_step(self, task, step): """Execute a clean step asynchronously on the agent. diff --git a/ironic/tests/unit/drivers/modules/ilo/test_deploy.py b/ironic/tests/unit/drivers/modules/ilo/test_deploy.py index 25d6e8447c..2bcff23f33 100644 --- a/ironic/tests/unit/drivers/modules/ilo/test_deploy.py +++ b/ironic/tests/unit/drivers/modules/ilo/test_deploy.py @@ -1087,10 +1087,10 @@ class IloVirtualMediaAgentDeployTestCase(db_base.DbTestCase): }] with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: - step = task.driver.deploy.get_clean_steps(task) - get_clean_step_mock.assert_called_once_with(task) - self.assertEqual(step[0].get('priority'), - CONF.ilo.clean_priority_erase_devices) + task.driver.deploy.get_clean_steps(task) + get_clean_step_mock.assert_called_once_with( + task, interface='deploy', + override_priorities={'erase_devices': 20}) @mock.patch.object(deploy_utils, 'agent_get_clean_steps', spec_set=True, autospec=True) @@ -1104,10 +1104,10 @@ class IloVirtualMediaAgentDeployTestCase(db_base.DbTestCase): }] with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: - step = task.driver.deploy.get_clean_steps(task) - get_clean_step_mock.assert_called_once_with(task) - self.assertEqual(step[0].get('priority'), - CONF.ilo.clean_priority_erase_devices) + task.driver.deploy.get_clean_steps(task) + get_clean_step_mock.assert_called_once_with( + task, interface='deploy', + override_priorities={'erase_devices': 0}) @mock.patch.object(deploy_utils, 'agent_get_clean_steps', spec_set=True, autospec=True) @@ -1120,9 +1120,10 @@ class IloVirtualMediaAgentDeployTestCase(db_base.DbTestCase): }] with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: - step = task.driver.deploy.get_clean_steps(task) - get_clean_step_mock.assert_called_once_with(task) - self.assertEqual(step[0].get('priority'), 10) + task.driver.deploy.get_clean_steps(task) + get_clean_step_mock.assert_called_once_with( + task, interface='deploy', + override_priorities={'erase_devices': None}) class VendorPassthruTestCase(db_base.DbTestCase): diff --git a/ironic/tests/unit/drivers/modules/test_agent.py b/ironic/tests/unit/drivers/modules/test_agent.py index 0192df7e76..dd47a49232 100644 --- a/ironic/tests/unit/drivers/modules/test_agent.py +++ b/ironic/tests/unit/drivers/modules/test_agent.py @@ -357,7 +357,9 @@ class TestAgentDeploy(db_base.DbTestCase): mock_get_clean_steps.return_value = mock_steps with task_manager.acquire(self.context, self.node.uuid) as task: steps = self.driver.get_clean_steps(task) - mock_get_clean_steps.assert_called_once_with(task) + mock_get_clean_steps.assert_called_once_with( + task, interface='deploy', + override_priorities={'erase_devices': None}) self.assertEqual(mock_steps, steps) @mock.patch('ironic.drivers.modules.deploy_utils.agent_get_clean_steps', @@ -368,13 +370,12 @@ class TestAgentDeploy(db_base.DbTestCase): self.config(erase_devices_priority=0, group='deploy') mock_steps = [{'priority': 10, 'interface': 'deploy', 'step': 'erase_devices'}] - expected_steps = [{'priority': 0, 'interface': 'deploy', - 'step': 'erase_devices'}] mock_get_clean_steps.return_value = mock_steps with task_manager.acquire(self.context, self.node.uuid) as task: - steps = self.driver.get_clean_steps(task) - mock_get_clean_steps.assert_called_once_with(task) - self.assertEqual(expected_steps, steps) + self.driver.get_clean_steps(task) + mock_get_clean_steps.assert_called_once_with( + task, interface='deploy', + override_priorities={'erase_devices': 0}) @mock.patch.object(deploy_utils, 'prepare_inband_cleaning', autospec=True) def test_prepare_cleaning(self, prepare_inband_cleaning_mock): diff --git a/ironic/tests/unit/drivers/modules/test_deploy_utils.py b/ironic/tests/unit/drivers/modules/test_deploy_utils.py index e49ea359b5..a2fd4fbfc0 100644 --- a/ironic/tests/unit/drivers/modules/test_deploy_utils.py +++ b/ironic/tests/unit/drivers/modules/test_deploy_utils.py @@ -29,6 +29,7 @@ from oslo_config import cfg from oslo_utils import uuidutils import requests import testtools +from testtools import matchers from ironic.common import boot_devices from ironic.common import disk_partitioner @@ -1933,11 +1934,78 @@ class AgentMethodsTestCase(db_base.DbTestCase): # Since steps are returned in dicts, they have non-deterministic # ordering - self.assertEqual(2, len(response)) + self.assertThat(response, matchers.HasLength(3)) self.assertIn(self.clean_steps['clean_steps'][ 'GenericHardwareManager'][0], response) self.assertIn(self.clean_steps['clean_steps'][ 'SpecificHardwareManager'][0], response) + self.assertIn(self.clean_steps['clean_steps'][ + 'SpecificHardwareManager'][1], response) + + @mock.patch('ironic.objects.Port.list_by_node_id', + spec_set=types.FunctionType) + @mock.patch.object(agent_client.AgentClient, 'get_clean_steps', + autospec=True) + def test_get_clean_steps_custom_interface( + self, client_mock, list_ports_mock): + client_mock.return_value = { + 'command_result': self.clean_steps} + list_ports_mock.return_value = self.ports + + with task_manager.acquire( + self.context, self.node.uuid, shared=False) as task: + response = utils.agent_get_clean_steps(task, interface='raid') + client_mock.assert_called_once_with(mock.ANY, task.node, + self.ports) + self.assertEqual('1', task.node.driver_internal_info[ + 'hardware_manager_version']) + + self.assertThat(response, matchers.HasLength(1)) + self.assertIn(self.clean_steps['clean_steps'][ + 'SpecificHardwareManager'][1], response) + + @mock.patch('ironic.objects.Port.list_by_node_id', + spec_set=types.FunctionType) + @mock.patch.object(agent_client.AgentClient, 'get_clean_steps', + autospec=True) + def test_get_clean_steps_override_priorities( + self, client_mock, list_ports_mock): + client_mock.return_value = { + 'command_result': self.clean_steps} + list_ports_mock.return_value = self.ports + + with task_manager.acquire( + self.context, self.node.uuid, shared=False) as task: + new_priorities = {'create_configuration': 42} + response = utils.agent_get_clean_steps( + task, interface='raid', override_priorities=new_priorities) + client_mock.assert_called_once_with(mock.ANY, task.node, + self.ports) + self.assertEqual('1', task.node.driver_internal_info[ + 'hardware_manager_version']) + self.assertEqual(42, response[0]['priority']) + + @mock.patch('ironic.objects.Port.list_by_node_id', + spec_set=types.FunctionType) + @mock.patch.object(agent_client.AgentClient, 'get_clean_steps', + autospec=True) + def test_get_clean_steps_override_priorities_none( + self, client_mock, list_ports_mock): + client_mock.return_value = { + 'command_result': self.clean_steps} + list_ports_mock.return_value = self.ports + + with task_manager.acquire( + self.context, self.node.uuid, shared=False) as task: + # this is simulating the default value of a configuration option + new_priorities = {'create_configuration': None} + response = utils.agent_get_clean_steps( + task, interface='raid', override_priorities=new_priorities) + client_mock.assert_called_once_with(mock.ANY, task.node, + self.ports) + self.assertEqual('1', task.node.driver_internal_info[ + 'hardware_manager_version']) + self.assertEqual(10, response[0]['priority']) @mock.patch('ironic.objects.Port.list_by_node_id', spec_set=types.FunctionType) @@ -1951,7 +2019,7 @@ class AgentMethodsTestCase(db_base.DbTestCase): list_ports_mock.return_value = self.ports with task_manager.acquire( - self.context, self.node['uuid'], shared=False) as task: + self.context, self.node.uuid, shared=False) as task: self.assertRaises(exception.NodeCleaningFailure, utils.agent_get_clean_steps, task)