From 7ead2067fc392426dae74d79a1f0e9605fee41b0 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Tue, 24 Apr 2018 12:56:40 +0200 Subject: [PATCH] Collect periodic tasks from all enabled hardware interfaces Currently we only collect periodic tasks from interfaces used in enabled classic drivers. Meaning, periodics are not collected from interfaces that are only used in hardware types. This patch corrects it. This patch does not enable collection of periodic tasks from hardware types, since we did not collect them from classic drivers. I don't remember the reason for that, and we may want to fix it later. Change-Id: Ib1963f3f67a758a6b2405387bfe7b3e30cc31ed8 Story: #2001884 Task: #14357 --- ironic/common/driver_factory.py | 9 ++ ironic/conductor/base_manager.py | 104 ++++++++++-------- .../tests/unit/conductor/test_base_manager.py | 46 +++++--- .../hw-ifaces-periodics-af8c9b93ecca9fcd.yaml | 6 + 4 files changed, 104 insertions(+), 61 deletions(-) create mode 100644 releasenotes/notes/hw-ifaces-periodics-af8c9b93ecca9fcd.yaml diff --git a/ironic/common/driver_factory.py b/ironic/common/driver_factory.py index 5c69c58748..da7f5be5b7 100644 --- a/ironic/common/driver_factory.py +++ b/ironic/common/driver_factory.py @@ -398,6 +398,15 @@ def interfaces(interface_type): return _get_all_drivers(_INTERFACE_LOADERS[interface_type]()) +def all_interfaces(): + """Get all interfaces for all interface types. + + :returns: Dictionary mapping interface type to dictionary mapping + interface name to interface object. + """ + return {iface: interfaces(iface) for iface in _INTERFACE_LOADERS} + + def enabled_supported_interfaces(hardware_type): """Get usable interfaces for a given hardware type. diff --git a/ironic/conductor/base_manager.py b/ironic/conductor/base_manager.py index 2aaaab9a5e..e43edb8a4e 100644 --- a/ironic/conductor/base_manager.py +++ b/ironic/conductor/base_manager.py @@ -149,33 +149,7 @@ class BaseConductorManager(object): LOG.error(msg, {'host': self.host, 'names': name_clashes}) raise exception.DriverNameConflict(names=name_clashes) - # Collect driver-specific periodic tasks. - # Conductor periodic tasks accept context argument, driver periodic - # tasks accept this manager and context. We have to ensure that the - # same driver interface class is not traversed twice, otherwise - # we'll have several instances of the same task. - LOG.debug('Collecting periodic tasks') - self._periodic_task_callables = [] - periodic_task_classes = set() - self._collect_periodic_tasks(self, (admin_context,)) - for driver_obj in drivers.values(): - for iface_name in driver_obj.all_interfaces: - iface = getattr(driver_obj, iface_name, None) - if iface and iface.__class__ not in periodic_task_classes: - self._collect_periodic_tasks(iface, (self, admin_context)) - periodic_task_classes.add(iface.__class__) - - if (len(self._periodic_task_callables) > - CONF.conductor.workers_pool_size): - LOG.warning('This conductor has %(tasks)d periodic tasks ' - 'enabled, but only %(workers)d task workers ' - 'allowed by [conductor]workers_pool_size option', - {'tasks': len(self._periodic_task_callables), - 'workers': CONF.conductor.workers_pool_size}) - - self._periodic_tasks = periodics.PeriodicWorker( - self._periodic_task_callables, - executor_factory=periodics.ExistingExecutor(self._executor)) + self._collect_periodic_tasks(admin_context) # Check for required config options if object_store_endpoint_type is # radosgw @@ -267,6 +241,66 @@ class BaseConductorManager(object): state, 'provision_updated_at', last_error=last_error) + def _collect_periodic_tasks(self, admin_context): + """Collect driver-specific periodic tasks. + + Conductor periodic tasks accept context argument, driver periodic + tasks accept this manager and context. We have to ensure that the + same driver interface class is not traversed twice, otherwise + we'll have several instances of the same task. + + :param admin_context: Administrator context to pass to tasks. + """ + LOG.debug('Collecting periodic tasks') + # collected callables + periodic_task_callables = [] + # list of visited classes to avoid adding the same tasks twice + periodic_task_classes = set() + + def _collect_from(obj, args): + """Collect tasks from the given object. + + :param obj: the object to collect tasks from. + :param args: a tuple of arguments to pass to tasks. + """ + if obj and obj.__class__ not in periodic_task_classes: + for name, member in inspect.getmembers(obj): + if periodics.is_periodic(member): + LOG.debug('Found periodic task %(owner)s.%(member)s', + {'owner': obj.__class__.__name__, + 'member': name}) + periodic_task_callables.append((member, args, {})) + periodic_task_classes.add(obj.__class__) + + # First, collect tasks from the conductor itself + _collect_from(self, (admin_context,)) + + # Second, collect tasks from hardware interfaces + for ifaces in driver_factory.all_interfaces().values(): + for iface in ifaces.values(): + _collect_from(iface, args=(self, admin_context)) + # TODO(dtantsur): allow periodics on hardware types themselves? + + # Finally, collect tasks from interfaces of classic drivers, since they + # are not necessary registered as new-style hardware interfaces. + for driver_obj in driver_factory.drivers().values(): + for iface_name in driver_obj.all_interfaces: + iface = getattr(driver_obj, iface_name, None) + _collect_from(iface, args=(self, admin_context)) + + if len(periodic_task_callables) > CONF.conductor.workers_pool_size: + LOG.warning('This conductor has %(tasks)d periodic tasks ' + 'enabled, but only %(workers)d task workers ' + 'allowed by [conductor]workers_pool_size option', + {'tasks': len(periodic_task_callables), + 'workers': CONF.conductor.workers_pool_size}) + + self._periodic_tasks = periodics.PeriodicWorker( + periodic_task_callables, + executor_factory=periodics.ExistingExecutor(self._executor)) + # This is only used in tests currently. Delete it? + self._periodic_task_callables = periodic_task_callables + def del_host(self, deregister=True): # Conductor deregistration fails if called on non-initialized # conductor (e.g. when rpc server is unreachable). @@ -331,22 +365,6 @@ class BaseConductorManager(object): # TODO(jroll) validate against other conductor, warn if different # how do we do this performantly? :| - def _collect_periodic_tasks(self, obj, args): - """Collect periodic tasks from a given object. - - Populates self._periodic_task_callables with tuples - (callable, args, kwargs). - - :param obj: object containing periodic tasks as methods - :param args: tuple with arguments to pass to every task - """ - for name, member in inspect.getmembers(obj): - if periodics.is_periodic(member): - LOG.debug('Found periodic task %(owner)s.%(member)s', - {'owner': obj.__class__.__name__, - 'member': name}) - self._periodic_task_callables.append((member, args, {})) - def _on_periodic_tasks_stop(self, fut): try: fut.result() diff --git a/ironic/tests/unit/conductor/test_base_manager.py b/ironic/tests/unit/conductor/test_base_manager.py index ff62fdb89e..a128aa8f2b 100644 --- a/ironic/tests/unit/conductor/test_base_manager.py +++ b/ironic/tests/unit/conductor/test_base_manager.py @@ -111,16 +111,21 @@ class StartStopTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): self.assertEqual(2, net_factory.call_count) self.assertEqual(2, storage_factory.call_count) - @mock.patch.object(driver_factory.DriverFactory, '__getitem__') - def test_start_registers_driver_specific_tasks(self, get_mock): - init_names = ['fake1'] - self.config(enabled_drivers=init_names) - + @mock.patch.object(driver_factory, 'all_interfaces', autospec=True) + @mock.patch.object(driver_factory, 'hardware_types', autospec=True) + @mock.patch.object(driver_factory, 'drivers', autospec=True) + def test_start_registers_driver_specific_tasks(self, mock_drivers, + mock_hw_types, mock_ifaces): class TestInterface(object): @periodics.periodic(spacing=100500) def iface(self): pass + class TestInterface2(object): + @periodics.periodic(spacing=100500) + def iface(self): + pass + class Driver(object): core_interfaces = [] standard_interfaces = ['iface'] @@ -132,22 +137,27 @@ class StartStopTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): def task(self, context): pass - obj = Driver() - get_mock.return_value = mock.Mock(obj=obj) + driver = Driver() + iface1 = TestInterface() + iface2 = TestInterface2() + expected = [iface1.iface, iface2.iface] - with mock.patch.object( - driver_factory.DriverFactory()._extension_manager, - 'names') as mock_names: - mock_names.return_value = init_names - self._start_service(start_periodic_tasks=True) + mock_drivers.return_value = {'fake1': driver} + mock_ifaces.return_value = { + 'management': {'fake1': iface1}, + 'power': {'fake2': iface2} + } + + self._start_service(start_periodic_tasks=True) tasks = {c[0] for c in self.service._periodic_task_callables} - self.assertTrue(periodics.is_periodic(obj.iface.iface)) - self.assertIn(obj.iface.iface, tasks) + for item in expected: + self.assertTrue(periodics.is_periodic(item)) + self.assertIn(item, tasks) # no periodic tasks from the Driver object - self.assertTrue(periodics.is_periodic(obj.task)) - self.assertNotIn(obj.task, tasks) + self.assertTrue(periodics.is_periodic(driver.task)) + self.assertNotIn(driver.task, tasks) @mock.patch.object(driver_factory.DriverFactory, '__init__') def test_start_fails_on_missing_driver(self, mock_df): @@ -194,7 +204,7 @@ class StartStopTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): df_mock.return_value = driver_factory_mock self.service.init_host() self.assertFalse(log_mock.error.called) - df_mock.assert_called_once_with() + df_mock.assert_called_with() self.assertFalse(del_mock.called) @mock.patch.object(base_manager, 'LOG') @@ -208,7 +218,7 @@ class StartStopTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): ht_mock.return_value = driver_factory_mock self.service.init_host() self.assertFalse(log_mock.error.called) - ht_mock.assert_called_once_with() + ht_mock.assert_called_with() self.assertFalse(del_mock.called) @mock.patch.object(base_manager, 'LOG') diff --git a/releasenotes/notes/hw-ifaces-periodics-af8c9b93ecca9fcd.yaml b/releasenotes/notes/hw-ifaces-periodics-af8c9b93ecca9fcd.yaml new file mode 100644 index 0000000000..1ac44129ca --- /dev/null +++ b/releasenotes/notes/hw-ifaces-periodics-af8c9b93ecca9fcd.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Fixes collection of periodic tasks from hardware interfaces that are not + used in any enabled classic drivers. See `bug 2001884 + `_ for details.