From cfcea55cf6d05cbc90af83d91cfd1ee9719f07fd Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Tue, 7 Dec 2021 16:50:19 +0100 Subject: [PATCH] Automatically configure enabled_***_interfaces This change makes it easier to configure power and management interfaces (and thus vendor drivers) by figuring out reasonable defaults. Story: #2009316 Task: #43717 Change-Id: I8779603e566be5a84daf6f680c0bbe2f191923d9 --- devstack/lib/ironic | 15 +------ ironic/common/driver_factory.py | 36 +++++++++++++-- ironic/conductor/base_manager.py | 24 ---------- ironic/conf/default.py | 4 +- .../tests/unit/common/test_driver_factory.py | 45 ++++++++----------- .../tests/unit/conductor/test_base_manager.py | 21 +++------ .../notes/auto-ifaces-fdb8c680eab711f4.yaml | 9 ++++ 7 files changed, 70 insertions(+), 84 deletions(-) create mode 100644 releasenotes/notes/auto-ifaces-fdb8c680eab711f4.yaml diff --git a/devstack/lib/ironic b/devstack/lib/ironic index 78b45f8122..3a463f411f 100644 --- a/devstack/lib/ironic +++ b/devstack/lib/ironic @@ -217,9 +217,9 @@ IRONIC_ENABLED_BOOT_INTERFACES=${IRONIC_ENABLED_BOOT_INTERFACES:-"fake,ipxe"} IRONIC_ENABLED_CONSOLE_INTERFACES=${IRONIC_ENABLED_CONSOLE_INTERFACES:-"fake,no-console"} IRONIC_ENABLED_DEPLOY_INTERFACES=${IRONIC_ENABLED_DEPLOY_INTERFACES:-"fake,direct,ramdisk"} IRONIC_ENABLED_INSPECT_INTERFACES=${IRONIC_ENABLED_INSPECT_INTERFACES:-"fake,no-inspect"} -IRONIC_ENABLED_MANAGEMENT_INTERFACES=${IRONIC_ENABLED_MANAGEMENT_INTERFACES:-"fake,ipmitool,noop"} +IRONIC_ENABLED_MANAGEMENT_INTERFACES=${IRONIC_ENABLED_MANAGEMENT_INTERFACES:-""} IRONIC_ENABLED_NETWORK_INTERFACES=${IRONIC_ENABLED_NETWORK_INTERFACES:-"flat,noop"} -IRONIC_ENABLED_POWER_INTERFACES=${IRONIC_ENABLED_POWER_INTERFACES:-"fake,ipmitool"} +IRONIC_ENABLED_POWER_INTERFACES=${IRONIC_ENABLED_POWER_INTERFACES:-""} IRONIC_ENABLED_RAID_INTERFACES=${IRONIC_ENABLED_RAID_INTERFACES:-"fake,agent,no-raid"} IRONIC_ENABLED_RESCUE_INTERFACES=${IRONIC_ENABLED_RESCUE_INTERFACES:-"fake,no-rescue"} IRONIC_ENABLED_STORAGE_INTERFACES=${IRONIC_ENABLED_STORAGE_INTERFACES:-"fake,cinder,noop"} @@ -1712,18 +1712,7 @@ function configure_ironic_conductor { fi done - if is_deployed_by_redfish; then - # TODO(lucasagomes): We need to make it easier to configure - # specific driver interfaces in DevStack - iniset $IRONIC_CONF_FILE DEFAULT enabled_power_interfaces "redfish" - iniset $IRONIC_CONF_FILE DEFAULT enabled_management_interfaces "redfish" - fi - if is_deployed_by_snmp; then - # TODO(lucasagomes): We need to make it easier to configure - # specific driver interfaces in DevStack - iniset $IRONIC_CONF_FILE DEFAULT enabled_power_interfaces "snmp" - iniset $IRONIC_CONF_FILE DEFAULT enabled_management_interfaces "noop" iniset $IRONIC_CONF_FILE pxe enable_netboot_fallback True fi diff --git a/ironic/common/driver_factory.py b/ironic/common/driver_factory.py index 57463111b8..8d50207026 100644 --- a/ironic/common/driver_factory.py +++ b/ironic/common/driver_factory.py @@ -17,7 +17,7 @@ import collections from oslo_concurrency import lockutils from oslo_log import log -from stevedore import named +import stevedore from ironic.common import exception from ironic.common.i18n import _ @@ -404,7 +404,7 @@ class BaseDriverFactory(object): cls._set_enabled_drivers() cls._extension_manager = ( - named.NamedExtensionManager( + stevedore.NamedExtensionManager( cls._entrypoint_name, cls._enabled_driver_list, invoke_on_load=True, @@ -429,6 +429,34 @@ class BaseDriverFactory(object): return ((ext.name, ext.obj) for ext in self._extension_manager) +class InterfaceFactory(BaseDriverFactory): + + # Name of a HardwareType attribute with a list of supported interfaces + _supported_driver_list_field = '' + + @classmethod + def _set_enabled_drivers(cls): + super()._set_enabled_drivers() + if cls._enabled_driver_list: + return + + tmp_ext_mgr = stevedore.ExtensionManager( + cls._entrypoint_name, + invoke_on_load=False, # do not create interfaces + on_load_failure_callback=cls._catch_driver_not_found) + cls_names = {v.plugin: k for (k, v) in tmp_ext_mgr.items()} + + # Fallback: calculate based on hardware type defaults + for hw_type in hardware_types().values(): + supported = getattr(hw_type, cls._supported_driver_list_field)[0] + try: + name = cls_names[supported] + except KeyError: + raise KeyError("%s not in %s" % (supported, cls_names)) + if name not in cls._enabled_driver_list: + cls._enabled_driver_list.append(name) + + def _warn_if_unsupported(ext): if not ext.obj.supported: LOG.warning('Driver "%s" is UNSUPPORTED. It has been deprecated ' @@ -443,10 +471,12 @@ class HardwareTypesFactory(BaseDriverFactory): _INTERFACE_LOADERS = { name: type('%sInterfaceFactory' % name.capitalize(), - (BaseDriverFactory,), + (InterfaceFactory,), {'_entrypoint_name': 'ironic.hardware.interfaces.%s' % name, '_enabled_driver_list_config_option': 'enabled_%s_interfaces' % name, + '_supported_driver_list_field': + 'supported_%s_interfaces' % name, '_logging_template': "Loaded the following %s interfaces: %%s" % name}) for name in driver_base.ALL_INTERFACES diff --git a/ironic/conductor/base_manager.py b/ironic/conductor/base_manager.py index d53e6af1ee..aa684408f7 100644 --- a/ironic/conductor/base_manager.py +++ b/ironic/conductor/base_manager.py @@ -40,7 +40,6 @@ from ironic.conductor import task_manager from ironic.conductor import utils from ironic.conf import CONF from ironic.db import api as dbapi -from ironic.drivers import base as driver_base from ironic.drivers.modules import deploy_utils from ironic import objects from ironic.objects import fields as obj_fields @@ -49,27 +48,6 @@ from ironic.objects import fields as obj_fields LOG = log.getLogger(__name__) -def _check_enabled_interfaces(): - """Sanity-check enabled_*_interfaces configs. - - We do this before we even bother to try to load up drivers. If we have any - dynamic drivers enabled, then we need interfaces enabled as well. - - :raises: ConfigInvalid if an enabled interfaces config option is empty. - """ - empty_confs = [] - iface_types = ['enabled_%s_interfaces' % i - for i in driver_base.ALL_INTERFACES] - for iface_type in iface_types: - conf_value = getattr(CONF, iface_type) - if not conf_value: - empty_confs.append(iface_type) - if empty_confs: - msg = (_('Configuration options %s cannot be an empty list.') % - ', '.join(empty_confs)) - raise exception.ConfigInvalid(error_msg=msg) - - class BaseConductorManager(object): def __init__(self, host, topic): @@ -146,8 +124,6 @@ class BaseConductorManager(object): use_groups=self._use_groups()) """Consistent hash ring which maps drivers to conductors.""" - _check_enabled_interfaces() - # NOTE(tenbrae): these calls may raise DriverLoadError or # DriverNotFound # NOTE(vdrok): Instantiate network and storage interface factory on diff --git a/ironic/conf/default.py b/ironic/conf/default.py index 3a6d3721da..66555d1467 100644 --- a/ironic/conf/default.py +++ b/ironic/conf/default.py @@ -121,7 +121,7 @@ driver_opts = [ cfg.StrOpt('default_inspect_interface', help=_DEFAULT_IFACE_HELP.format('inspect')), cfg.ListOpt('enabled_management_interfaces', - default=['ipmitool', 'redfish'], + default=None, # automatically calculate help=_ENABLED_IFACE_HELP.format('management')), cfg.StrOpt('default_management_interface', help=_DEFAULT_IFACE_HELP.format('management')), @@ -131,7 +131,7 @@ driver_opts = [ cfg.StrOpt('default_network_interface', help=_DEFAULT_IFACE_HELP.format('network')), cfg.ListOpt('enabled_power_interfaces', - default=['ipmitool', 'redfish'], + default=None, # automatically calculate help=_ENABLED_IFACE_HELP.format('power')), cfg.StrOpt('default_power_interface', help=_DEFAULT_IFACE_HELP.format('power')), diff --git a/ironic/tests/unit/common/test_driver_factory.py b/ironic/tests/unit/common/test_driver_factory.py index 77626a0842..c4857d21ce 100644 --- a/ironic/tests/unit/common/test_driver_factory.py +++ b/ironic/tests/unit/common/test_driver_factory.py @@ -319,39 +319,31 @@ class DefaultInterfaceTestCase(db_base.DbTestCase): iface = driver_factory.default_interface(self.driver, 'deploy') self.assertEqual('ansible', iface) - def test_calculated_no_answer(self): - # manual-management supports no power interfaces + def test_calculated_fallback(self): self.config(default_power_interface=None) self.config(enabled_power_interfaces=[]) - self.assertRaisesRegex( - exception.NoValidDefaultForInterface, - "For hardware type 'ManualManagementHardware', no default " - "value found for power interface.", - driver_factory.default_interface, self.driver, 'power') + iface = driver_factory.default_interface(self.driver, 'power') + self.assertEqual('agent', iface) def test_calculated_no_answer_drivername(self): # manual-management instance (of entry-point driver named 'foo') # supports no power interfaces self.config(default_power_interface=None) self.config(enabled_power_interfaces=[]) - self.assertRaisesRegex( - exception.NoValidDefaultForInterface, - "For hardware type 'foo', no default value found for power " - "interface.", - driver_factory.default_interface, self.driver, 'power', - driver_name='foo') + self.assertEqual( + "agent", + driver_factory.default_interface(self.driver, 'power', + driver_name='foo')) def test_calculated_no_answer_drivername_node(self): # for a node with manual-management instance (of entry-point driver # named 'foo'), no default power interface is supported self.config(default_power_interface=None) self.config(enabled_power_interfaces=[]) - self.assertRaisesRegex( - exception.NoValidDefaultForInterface, - "For node bar with hardware type 'foo', no default " - "value found for power interface.", - driver_factory.default_interface, self.driver, 'power', - driver_name='foo', node='bar') + self.assertEqual( + "agent", + driver_factory.default_interface(self.driver, 'power', + driver_name='foo', node='bar')) @mock.patch.object(driver_factory, 'get_interface', autospec=True) def test_check_exception_IncompatibleInterface(self, mock_get_interface): @@ -490,15 +482,18 @@ class HardwareTypeLoadTestCase(db_base.DbTestCase): task_manager.acquire, self.context, node.id) mock_get_hw_type.assert_called_once_with('fake-2') - def test_build_driver_for_task_no_defaults(self): + def test_build_driver_for_task_fallback_defaults(self): self.config(dhcp_provider=None, group='dhcp') + self.config(enabled_hardware_types=['fake-hardware']) for iface in drivers_base.ALL_INTERFACES: if iface not in ['network', 'storage']: self.config(**{'enabled_%s_interfaces' % iface: []}) self.config(**{'default_%s_interface' % iface: None}) node = obj_utils.create_test_node(self.context, driver='fake-hardware') - self.assertRaises(exception.NoValidDefaultForInterface, - task_manager.acquire, self.context, node.id) + with task_manager.acquire(self.context, node.id) as task: + for iface in drivers_base.ALL_INTERFACES: + impl = getattr(task.driver, iface) + self.assertIsNotNone(impl) def test_build_driver_for_task_calculated_defaults(self): self.config(dhcp_provider=None, group='dhcp') @@ -581,10 +576,8 @@ class HardwareTypeLoadTestCase(db_base.DbTestCase): # for storage, so we'll test this case with raid. self.config(enabled_raid_interfaces=[]) node = obj_utils.get_test_node(self.context, driver='fake-hardware') - self.assertRaisesRegex( - exception.NoValidDefaultForInterface, - "raid interface", - driver_factory.check_and_update_node_interfaces, node) + self.assertTrue(driver_factory.check_and_update_node_interfaces(node)) + self.assertEqual('fake', node.raid_interface) def _test_enabled_supported_interfaces(self, enable_storage): ht = fake_hardware.FakeHardware() diff --git a/ironic/tests/unit/conductor/test_base_manager.py b/ironic/tests/unit/conductor/test_base_manager.py index 7ae2c74eef..f92c6e58c8 100644 --- a/ironic/tests/unit/conductor/test_base_manager.py +++ b/ironic/tests/unit/conductor/test_base_manager.py @@ -193,11 +193,11 @@ class StartStopTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): self.assertTrue(mock_df.called) self.assertFalse(mock_reg.called) - def test_start_fails_on_no_enabled_interfaces(self): - self.config(enabled_boot_interfaces=[]) - self.assertRaisesRegex(exception.ConfigInvalid, - 'options enabled_boot_interfaces', - self.service.init_host) + def test_start_with_no_enabled_interfaces(self): + self.config(enabled_boot_interfaces=[], + enabled_deploy_interfaces=[], + enabled_hardware_types=['fake-hardware']) + self._start_service() @mock.patch.object(base_manager, 'LOG', autospec=True) @mock.patch.object(driver_factory, 'HardwareTypesFactory', autospec=True) @@ -311,17 +311,6 @@ class StartStopTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): self.assertEqual(2, mock_dbapi.call_count) -class CheckInterfacesTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): - def test__check_enabled_interfaces_success(self): - base_manager._check_enabled_interfaces() - - def test__check_enabled_interfaces_failure(self): - self.config(enabled_boot_interfaces=[]) - self.assertRaisesRegex(exception.ConfigInvalid, - 'options enabled_boot_interfaces', - base_manager._check_enabled_interfaces) - - class KeepAliveTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): def test__conductor_service_record_keepalive(self): self._start_service() diff --git a/releasenotes/notes/auto-ifaces-fdb8c680eab711f4.yaml b/releasenotes/notes/auto-ifaces-fdb8c680eab711f4.yaml new file mode 100644 index 0000000000..9495ad27ba --- /dev/null +++ b/releasenotes/notes/auto-ifaces-fdb8c680eab711f4.yaml @@ -0,0 +1,9 @@ +--- +features: + - | + The configuration options ``enabled_power_interfaces`` and + ``enabled_management_interfaces`` are now empty by default. If left empty, + their values will be calculated based on ``enabled_hardware_types``. + - | + The Bare Metal service is now capable of calculating the default value + for any ``enabled_***_interfaces`` based on ``enabled_hardware_types``.