diff --git a/doc/source/contributor/deploy-steps.rst b/doc/source/contributor/deploy-steps.rst index a6cd6809d2..03856d231a 100644 --- a/doc/source/contributor/deploy-steps.rst +++ b/doc/source/contributor/deploy-steps.rst @@ -251,10 +251,6 @@ periodic task that operates on relevant nodes: predicate=lambda n: n.driver_internal_info.get('in_my_action'), ) def check_my_action(self, task, manager, context): - # Double-check that the node is managed by this interface - if not isinstance(task.driver.management, MyManagement): - return - if not needs_actions(): # insert your checks here return diff --git a/ironic/conductor/periodics.py b/ironic/conductor/periodics.py index ead5cbf08a..70bc7bc939 100644 --- a/ironic/conductor/periodics.py +++ b/ironic/conductor/periodics.py @@ -23,6 +23,7 @@ from oslo_log import log from ironic.common import exception from ironic.conductor import base_manager from ironic.conductor import task_manager +from ironic.drivers import base as driver_base LOG = log.getLogger(__name__) @@ -57,6 +58,9 @@ def node_periodic(purpose, spacing, enabled=True, filters=None, * for conductor manager: ``(self, task, context)`` * for hardware interfaces: ``(self, task, manager, context)``. + When the periodic is running on a hardware interface, only tasks + using this interface are considered. + ``NodeNotFound`` and ``NodeLocked`` exceptions are ignored. Raise ``Stop`` to abort the current iteration of the task and reschedule it. @@ -103,6 +107,10 @@ def node_periodic(purpose, spacing, enabled=True, filters=None, manager = args[0] context = args[1] + interface_type = (getattr(self, 'interface_type', None) + if isinstance(self, driver_base.BaseInterface) + else None) + if callable(limit): local_limit = limit() else: @@ -125,6 +133,11 @@ def node_periodic(purpose, spacing, enabled=True, filters=None, with task_manager.acquire(context, node_uuid, purpose=purpose, shared=shared_task) as task: + if interface_type is not None: + impl = getattr(task.driver, interface_type) + if not isinstance(impl, self.__class__): + continue + result = func(self, task, *args, **kwargs) except exception.NodeNotFound: LOG.info("During %(action)s, node %(node)s was not found " diff --git a/ironic/drivers/modules/drac/bios.py b/ironic/drivers/modules/drac/bios.py index 795e4f1508..8e36ef4c7b 100644 --- a/ironic/drivers/modules/drac/bios.py +++ b/ironic/drivers/modules/drac/bios.py @@ -167,10 +167,6 @@ class DracWSManBIOS(base.BIOSInterface): :param context: context of the request, needed when acquiring a lock on a node. For access control. """ - # skip a node not being managed by idrac driver - if not isinstance(task.driver.bios, DracWSManBIOS): - return - # check bios_config_job_id exist & checks job is completed if task.node.driver_internal_info.get("bios_config_job_ids"): self._check_node_bios_jobs(task) diff --git a/ironic/drivers/modules/drac/management.py b/ironic/drivers/modules/drac/management.py index 9b2b534311..8272ce5059 100644 --- a/ironic/drivers/modules/drac/management.py +++ b/ironic/drivers/modules/drac/management.py @@ -496,9 +496,6 @@ class DracRedfishManagement(redfish_management.RedfishManagement): ) def _query_import_configuration_status(self, task, manager, context): """Period job to check import configuration task.""" - if not isinstance(task.driver.management, DracRedfishManagement): - return - self._check_import_configuration_task( task, task.node.driver_internal_info.get( 'import_task_monitor_url')) diff --git a/ironic/drivers/modules/drac/raid.py b/ironic/drivers/modules/drac/raid.py index 726f57d3af..a85e4d3726 100644 --- a/ironic/drivers/modules/drac/raid.py +++ b/ironic/drivers/modules/drac/raid.py @@ -1497,9 +1497,6 @@ class DracRedfishRAID(redfish_raid.RedfishRAID): ) def _query_raid_tasks_status(self, task, manager, context): """Periodic task to check the progress of running RAID tasks""" - if not isinstance(task.driver.raid, DracRedfishRAID): - return - self._check_raid_tasks_status( task, task.node.driver_internal_info.get('raid_task_monitor_uris')) @@ -1757,9 +1754,6 @@ class DracWSManRAID(base.RAIDInterface): ) def _query_raid_config_job_status(self, task, manager, context): """Periodic task to check the progress of running RAID config jobs.""" - if not isinstance(task.driver.raid, DracWSManRAID): - return - self._check_node_raid_jobs(task) @METRICS.timer('DracRAID._check_node_raid_jobs') diff --git a/ironic/drivers/modules/irmc/raid.py b/ironic/drivers/modules/irmc/raid.py index 3368e887d3..26737ea11e 100644 --- a/ironic/drivers/modules/irmc/raid.py +++ b/ironic/drivers/modules/irmc/raid.py @@ -443,8 +443,6 @@ class IRMCRAID(base.RAIDInterface): """Periodic tasks to check the progress of running RAID config.""" node = task.node node_uuid = task.node.uuid - if not isinstance(task.driver.raid, IRMCRAID): - return if task.node.target_raid_config is None: return task.upgrade_lock() diff --git a/ironic/drivers/modules/pxe_base.py b/ironic/drivers/modules/pxe_base.py index ab5b0d5357..a89c9e4431 100644 --- a/ironic/drivers/modules/pxe_base.py +++ b/ironic/drivers/modules/pxe_base.py @@ -470,9 +470,6 @@ class PXEBaseMixin(object): self._check_boot_status(task) def _check_boot_status(self, task): - if not isinstance(task.driver.boot, PXEBaseMixin): - return - if not _should_retry_boot(task.node): return diff --git a/ironic/drivers/modules/redfish/management.py b/ironic/drivers/modules/redfish/management.py index ab1a105efb..29844fae90 100644 --- a/ironic/drivers/modules/redfish/management.py +++ b/ironic/drivers/modules/redfish/management.py @@ -863,21 +863,16 @@ class RedfishManagement(base.ManagementInterface): ) def _query_firmware_update_failed(self, task, manager, context): """Periodic job to check for failed firmware updates.""" - if not isinstance(task.driver.management, RedfishManagement): - return - - node = task.node - # A firmware update failed. Discard any remaining firmware # updates so when the user takes the node out of # maintenance mode, pending firmware updates do not # automatically continue. LOG.warning('Firmware update failed for node %(node)s. ' 'Discarding remaining firmware updates.', - {'node': node.uuid}) + {'node': task.node.uuid}) task.upgrade_lock() - self._clear_firmware_updates(node) + self._clear_firmware_updates(task.node) @METRICS.timer('RedfishManagement._query_firmware_update_status') @periodics.node_periodic( @@ -889,9 +884,6 @@ class RedfishManagement(base.ManagementInterface): ) def _query_firmware_update_status(self, task, manager, context): """Periodic job to check firmware update tasks.""" - if not isinstance(task.driver.management, RedfishManagement): - return - self._check_node_firmware_update(task) @METRICS.timer('RedfishManagement._check_node_firmware_update') diff --git a/ironic/drivers/modules/redfish/raid.py b/ironic/drivers/modules/redfish/raid.py index 95052bb467..33a7853ea1 100644 --- a/ironic/drivers/modules/redfish/raid.py +++ b/ironic/drivers/modules/redfish/raid.py @@ -1023,21 +1023,16 @@ class RedfishRAID(base.RAIDInterface): ) def _query_raid_config_failed(self, task, manager, context): """Periodic job to check for failed RAID configuration.""" - if not isinstance(task.driver.raid, RedfishRAID): - return - - node = task.node - # A RAID config failed. Discard any remaining RAID # configs so when the user takes the node out of # maintenance mode, pending RAID configs do not # automatically continue. LOG.warning('RAID configuration failed for node %(node)s. ' 'Discarding remaining RAID configurations.', - {'node': node.uuid}) + {'node': task.node.uuid}) task.upgrade_lock() - self._clear_raid_configs(node) + self._clear_raid_configs(task.node) @METRICS.timer('RedfishRAID._query_raid_config_status') @periodics.node_periodic( @@ -1049,9 +1044,6 @@ class RedfishRAID(base.RAIDInterface): ) def _query_raid_config_status(self, task, manager, context): """Periodic job to check RAID config tasks.""" - if not isinstance(task.driver.raid, RedfishRAID): - return - self._check_node_raid_config(task) def _get_error_messages(self, response): diff --git a/ironic/tests/unit/conductor/test_periodics.py b/ironic/tests/unit/conductor/test_periodics.py index 85868163a0..b35570f641 100644 --- a/ironic/tests/unit/conductor/test_periodics.py +++ b/ironic/tests/unit/conductor/test_periodics.py @@ -18,6 +18,7 @@ from ironic.common import context as ironic_context from ironic.conductor import base_manager from ironic.conductor import periodics from ironic.conductor import task_manager +from ironic.drivers.modules import fake from ironic.tests.unit.db import base as db_base from ironic.tests.unit.objects import utils as obj_utils @@ -59,6 +60,19 @@ class PeriodicTestService(base_manager.BaseConductorManager): raise periodics.Stop() +class PeriodicTestInterface(fake.FakePower): + + def __init__(self, test): + self.test = test + self.nodes = [] + + @periodics.node_periodic(purpose="herding cats", spacing=42) + def simple(self, task, manager, context): + self.test.assertIsInstance(manager, PeriodicTestService) + self.test.assertIsInstance(context, ironic_context.RequestContext) + self.nodes.append(task.node.uuid) + + @mock.patch.object(PeriodicTestService, 'iter_nodes', autospec=True) class NodePeriodicTestCase(db_base.DbTestCase): @@ -133,3 +147,29 @@ class NodePeriodicTestCase(db_base.DbTestCase): mock_iter_nodes.assert_called_once_with(self.service, filters=None, fields=()) self.assertEqual(['stop'], self.service.nodes) + + @mock.patch.object(task_manager, 'acquire', autospec=True) + def test_interface_check(self, mock_acquire, mock_iter_nodes): + mock_iter_nodes.return_value = iter([ + (uuidutils.generate_uuid(), 'driver1', ''), + (self.uuid, 'driver2', 'group'), + ]) + iface = PeriodicTestInterface(self) + tasks = [ + mock.Mock(spec=task_manager.TaskManager, + # This will not match the subclass + driver=mock.Mock(power=fake.FakePower())), + mock.Mock(spec=task_manager.TaskManager, + node=self.node, + driver=mock.Mock(power=iface)), + ] + mock_acquire.side_effect = [ + mock.MagicMock(**{'__enter__.return_value': task}) + for task in tasks + ] + + iface.simple(self.service, self.context) + + mock_iter_nodes.assert_called_once_with(self.service, + filters=None, fields=()) + self.assertEqual([self.uuid], iface.nodes) diff --git a/ironic/tests/unit/drivers/modules/test_pxe.py b/ironic/tests/unit/drivers/modules/test_pxe.py index 8847b9813d..1f071b727f 100644 --- a/ironic/tests/unit/drivers/modules/test_pxe.py +++ b/ironic/tests/unit/drivers/modules/test_pxe.py @@ -36,7 +36,6 @@ from ironic.drivers import base as drivers_base from ironic.drivers.modules import agent_base from ironic.drivers.modules import boot_mode_utils from ironic.drivers.modules import deploy_utils -from ironic.drivers.modules import fake from ironic.drivers.modules import ipxe from ironic.drivers.modules import pxe from ironic.drivers.modules import pxe_base @@ -1047,6 +1046,7 @@ class PXEValidateRescueTestCase(db_base.DbTestCase): class PXEBootRetryTestCase(db_base.DbTestCase): boot_interface = 'pxe' + boot_interface_class = pxe.PXEBoot def setUp(self): super(PXEBootRetryTestCase, self).setUp() @@ -1058,31 +1058,28 @@ class PXEBootRetryTestCase(db_base.DbTestCase): boot_interface=self.boot_interface, provision_state=states.DEPLOYWAIT) - @mock.patch.object(pxe.PXEBoot, '_check_boot_status', autospec=True) - def test_check_boot_timeouts(self, mock_check_status, mock_power, - mock_boot_dev): + def test_check_boot_timeouts(self, mock_power, mock_boot_dev): def _side_effect(iface, task): self.assertEqual(self.node.uuid, task.node.uuid) - mock_check_status.side_effect = _side_effect + fake_node = obj_utils.create_test_node( + self.context, + uuid=uuidutils.generate_uuid(), + driver='fake-hardware', + boot_interface='fake', + provision_state=states.DEPLOYWAIT) + manager = mock.Mock(spec=['iter_nodes']) manager.iter_nodes.return_value = [ - (uuidutils.generate_uuid(), 'fake-hardware', ''), - (self.node.uuid, self.node.driver, self.node.conductor_group) + (fake_node.uuid, 'fake-hardware', ''), + (self.node.uuid, self.node.driver, self.node.conductor_group), ] - iface = pxe.PXEBoot() - iface._check_boot_timeouts(manager, self.context) - mock_check_status.assert_called_once_with(iface, mock.ANY) - - def test_check_boot_status_another_boot_interface(self, mock_power, - mock_boot_dev): - with task_manager.acquire(self.context, self.node.uuid, - shared=True) as task: - task.driver.boot = fake.FakeBoot() - pxe.PXEBoot()._check_boot_status(task) - self.assertTrue(task.shared) - self.assertFalse(mock_power.called) - self.assertFalse(mock_boot_dev.called) + with mock.patch.object(self.boot_interface_class, '_check_boot_status', + autospec=True) as mock_check_status: + mock_check_status.side_effect = _side_effect + iface = self.boot_interface_class() + iface._check_boot_timeouts(manager, self.context) + mock_check_status.assert_called_once_with(iface, mock.ANY) def test_check_boot_status_recent_power_change(self, mock_power, mock_boot_dev): @@ -1133,3 +1130,4 @@ class PXEBootRetryTestCase(db_base.DbTestCase): class iPXEBootRetryTestCase(PXEBootRetryTestCase): boot_interface = 'ipxe' + boot_interface_class = ipxe.iPXEBoot