Merge "Refactor deploy_utils methods"

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)
+        new_priorities = {
-        if CONF.deploy.erase_devices_priority is not None:
+            'erase_devices': CONF.deploy.erase_devices_priority,
-            for step in steps:
+        }
-                if (step.get('step') == 'erase_devices' and
+        return deploy_utils.agent_get_clean_steps(
-                        step.get('interface') == 'deploy'):
+            task, interface='deploy',
-                    # Override with operator set priority
+            override_priorities=new_priorities)
-                    step['priority'] = CONF.deploy.erase_devices_priority
-        return steps
 
     def execute_clean_step(self, task, step):
         """Execute a clean step asynchronously on the agent.

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
+    result = []
-    steps = [step for step in steps_list
+    for step in steps_list:
-             if step.get('interface') == 'deploy']
+        if interface and step.get('interface') != interface:
-    return steps
+            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):

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)
+        new_priorities = {
-        if CONF.ilo.clean_priority_erase_devices is not None:
+            'erase_devices': CONF.ilo.clean_priority_erase_devices,
-            for step in steps:
+        }
-                if (step.get('step') == 'erase_devices' and
+        return deploy_utils.agent_get_clean_steps(
-                        step.get('interface') == 'deploy'):
+            task, interface='deploy',
-                    # Override with operator set priority
+            override_priorities=new_priorities)
-                    step['priority'] = CONF.ilo.clean_priority_erase_devices
-
-        return steps
 
     def execute_clean_step(self, task, step):
         """Execute a clean step asynchronously on the agent.

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)
+            task.driver.deploy.get_clean_steps(task)
-            get_clean_step_mock.assert_called_once_with(task)
+            get_clean_step_mock.assert_called_once_with(
-            self.assertEqual(step[0].get('priority'),
+                task, interface='deploy',
-                             CONF.ilo.clean_priority_erase_devices)
+                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)
+            task.driver.deploy.get_clean_steps(task)
-            get_clean_step_mock.assert_called_once_with(task)
+            get_clean_step_mock.assert_called_once_with(
-            self.assertEqual(step[0].get('priority'),
+                task, interface='deploy',
-                             CONF.ilo.clean_priority_erase_devices)
+                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)
+            task.driver.deploy.get_clean_steps(task)
-            get_clean_step_mock.assert_called_once_with(task)
+            get_clean_step_mock.assert_called_once_with(
-            self.assertEqual(step[0].get('priority'), 10)
+                task, interface='deploy',
+                override_priorities={'erase_devices': None})
 
 
 class VendorPassthruTestCase(db_base.DbTestCase):

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)
+            self.driver.get_clean_steps(task)
-            mock_get_clean_steps.assert_called_once_with(task)
+            mock_get_clean_steps.assert_called_once_with(
-        self.assertEqual(expected_steps, steps)
+                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):

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)