openstack
/
ironic
Code Issues Proposed changes

Improve conductor driver validation at startup 

This changes the driver loading validation in the conductor
startup to check for at least one classic *or* dynamic driver.
Previously the conductor would fail to start if no classic drivers
were loaded. This allows the conductor to use only dynamic
drivers, without loading any classic drivers.

It also now checks classic driver names against dynamic driver
names, and fails to start if there is a conflict there. This
would totally break the hash ring and cause mass confusion,
so we cannot allow it.

Change-Id: Id368690697f90471d09f16eaa4925338dadebd0f
Partial-Bug: #1524745
This commit is contained in:
Jim Rollenhagen 2017-01-27 12:26:22 +00:00
parent 646a1f76a9
commit 0071c7e1ec
4 changed files with 90 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
ironic/common/exception.py
View File

@ -416,6 +416,11 @@ class VolumeTargetNotFound(NotFound):
_msg_fmt = _("Volume target %(target)s could not be found.")
class DriverNameConflict(IronicException):
_msg_fmt = _("Classic and dynamic drivers cannot have the "
"same names '%(names)s'.")
class NoDriversLoaded(IronicException):
_msg_fmt = _("Conductor %(conductor)s cannot be started "
"because no drivers were loaded.")

43
ironic/conductor/base_manager.py
View File

@ -60,6 +60,8 @@ class BaseConductorManager(object):
:raises: NoDriversLoaded when no drivers are enabled on the conductor.
:raises: DriverNotFound if a driver is enabled that does not exist.
:raises: DriverLoadError if an enabled driver cannot be loaded.
:raises: DriverNameConflict if a classic driver and a dynamic driver
are both enabled and have the same name.
"""
if self._started:
raise RuntimeError(_('Attempt to start an already running '
@ -86,18 +88,36 @@ class BaseConductorManager(object):
# startup so that all the interfaces are loaded at the very
# beginning, and failures prevent the conductor from starting.
drivers = driver_factory.drivers()
hardware_types = driver_factory.hardware_types()
driver_factory.NetworkInterfaceFactory()
driver_factory.StorageInterfaceFactory()
if not drivers:
msg = _LE("Conductor %s cannot be started because no drivers "
"were loaded. This could be because no drivers were "
"specified in 'enabled_drivers' config option.")
LOG.error(msg, self.host)
raise exception.NoDriversLoaded(conductor=self.host)
# NOTE(jroll) this is passed to the dbapi, which requires a list, not
# a generator (which keys() returns in py3)
driver_names = list(drivers)
hardware_type_names = list(hardware_types)
# check that at least one driver is loaded, whether classic or dynamic
if not driver_names and not hardware_type_names:
msg = _LE("Conductor %s cannot be started because no drivers "
"were loaded. This could be because no classic drivers "
"were specified in the 'enabled_drivers' config option "
"and no dynamic drivers were specified in the "
"'enabled_hardware_types' config option.")
LOG.error(msg, self.host)
raise exception.NoDriversLoaded(conductor=self.host)
# check for name clashes between classic and dynamic drivers
name_clashes = set(driver_names).intersection(hardware_type_names)
if name_clashes:
name_clashes = ', '.join(name_clashes)
msg = _LE("Conductor %(host)s cannot be started because there is "
"one or more name conflicts between classic drivers and "
"dynamic drivers (%(names)s). Check any external driver "
"plugins and the 'enabled_drivers' and "
"'enabled_hardware_types' config options.")
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
@ -148,7 +168,7 @@ class BaseConductorManager(object):
# register hardware types and interfaces supported by this conductor
# and validate them against other conductors
try:
self._register_and_validate_hardware_interfaces()
self._register_and_validate_hardware_interfaces(hardware_types)
except (exception.DriverLoadError, exception.DriverNotFound,
exception.ConductorHardwareInterfacesAlreadyRegistered,
exception.InterfaceNotFoundInEntrypoint) as e:
@ -156,10 +176,6 @@ class BaseConductorManager(object):
LOG.error(_LE('Failed to register hardware types. %s'), e)
self.del_host()
# TODO(jroll) validate here that at least one driver OR
# hardware type is loaded. If not, call del_host and raise
# NoDriversLoaded.
# Start periodic tasks
self._periodic_tasks_worker = self._executor.submit(
self._periodic_tasks.start, allow_empty=True)
@ -231,7 +247,7 @@ class BaseConductorManager(object):
self._executor.shutdown(wait=True)
self._started = False
def _register_and_validate_hardware_interfaces(self):
def _register_and_validate_hardware_interfaces(self, hardware_types):
"""Register and validate hardware interfaces for this conductor.
Registers a row in the database for each combination of
@ -243,13 +259,14 @@ class BaseConductorManager(object):
same, and warns if not (we can't error out, otherwise all conductors
must be restarted at once to change configuration).
:param hardware_types: Dictionary mapping hardware type name to
hardware type object.
:raises: ConductorHardwareInterfacesAlreadyRegistered
:raises: InterfaceNotFoundInEntrypoint
"""
# first unregister, in case we have cruft laying around
self.conductor.unregister_all_hardware_interfaces()
hardware_types = driver_factory.hardware_types()
for ht_name, ht in hardware_types.items():
interface_map = driver_factory.enabled_supported_interfaces(ht)
for interface_type, interface_names in interface_map.items():

52
ironic/tests/unit/conductor/test_base_manager.py
View File

@ -159,13 +159,44 @@ class StartStopTestCase(mgr_utils.ServiceSetUpMixin, tests_db_base.DbTestCase):
self.assertFalse(mock_reg.called)
@mock.patch.object(base_manager, 'LOG')
@mock.patch.object(driver_factory, 'HardwareTypesFactory')
@mock.patch.object(driver_factory, 'DriverFactory')
def test_start_fails_on_no_driver(self, df_mock, log_mock):
def test_start_fails_on_no_driver_or_hw_types(self, df_mock, ht_mock,
log_mock):
driver_factory_mock = mock.MagicMock(names=[])
df_mock.return_value = driver_factory_mock
ht_mock.return_value = driver_factory_mock
self.assertRaises(exception.NoDriversLoaded,
self.service.init_host)
self.assertTrue(log_mock.error.called)
df_mock.assert_called_once_with()
ht_mock.assert_called_once_with()
@mock.patch.object(base_manager, 'LOG')
@mock.patch.object(base_manager.BaseConductorManager, 'del_host')
@mock.patch.object(driver_factory, 'DriverFactory')
def test_starts_with_only_dynamic_drivers(self, df_mock, del_mock,
log_mock):
# don't load any classic drivers
driver_factory_mock = mock.MagicMock(names=[])
df_mock.return_value = driver_factory_mock
self.service.init_host()
self.assertFalse(log_mock.error.called)
df_mock.assert_called_once_with()
self.assertFalse(del_mock.called)
@mock.patch.object(base_manager, 'LOG')
@mock.patch.object(base_manager.BaseConductorManager, 'del_host')
@mock.patch.object(driver_factory, 'HardwareTypesFactory')
def test_starts_with_only_classic_drivers(self, ht_mock, del_mock,
log_mock):
# don't load any dynamic drivers
driver_factory_mock = mock.MagicMock(names=[])
ht_mock.return_value = driver_factory_mock
self.service.init_host()
self.assertFalse(log_mock.error.called)
ht_mock.assert_called_once_with()
self.assertFalse(del_mock.called)
@mock.patch.object(base_manager, 'LOG')
@mock.patch.object(base_manager.BaseConductorManager,
@ -178,6 +209,19 @@ class StartStopTestCase(mgr_utils.ServiceSetUpMixin, tests_db_base.DbTestCase):
self.assertTrue(log_mock.error.called)
del_mock.assert_called_once_with()
@mock.patch.object(base_manager, 'LOG')
@mock.patch.object(driver_factory, 'HardwareTypesFactory')
@mock.patch.object(driver_factory, 'DriverFactory')
def test_start_fails_on_name_conflict(self, df_mock, ht_mock, log_mock):
driver_factory_mock = mock.MagicMock(names=['dupe-driver'])
df_mock.return_value = driver_factory_mock
ht_mock.return_value = driver_factory_mock
self.assertRaises(exception.DriverNameConflict,
self.service.init_host)
self.assertTrue(log_mock.error.called)
df_mock.assert_called_once_with()
ht_mock.assert_called_once_with()
def test_prevent_double_start(self):
self._start_service()
self.assertRaisesRegex(RuntimeError, 'already running',
@ -247,7 +291,6 @@ class ManagerSpawnWorkerTestCase(tests_base.TestCase):
@mock.patch.object(objects.Conductor, 'register_hardware_interfaces',
autospec=True)
@mock.patch.object(driver_factory, 'default_interface', autospec=True)
@mock.patch.object(driver_factory, 'hardware_types', autospec=True)
@mock.patch.object(driver_factory, 'enabled_supported_interfaces',
autospec=True)
@mgr_utils.mock_record_keepalive
@ -259,12 +302,11 @@ class RegisterInterfacesTestCase(mgr_utils.ServiceSetUpMixin,
def test__register_and_validate_hardware_interfaces(self,
esi_mock,
ht_mock,
default_mock,
reg_mock,
unreg_mock):
# these must be same order as esi_mock side effect
ht_mock.return_value = collections.OrderedDict((
hardware_types = collections.OrderedDict((
('fake-hardware', fake_hardware.FakeHardware()),
('manual-management', generic.ManualManagementHardware),
))
@ -290,7 +332,7 @@ class RegisterInterfacesTestCase(mgr_utils.ServiceSetUpMixin,
['agent', 'fake'], 'agent'),
]
self.service._register_and_validate_hardware_interfaces()
self.service._register_and_validate_hardware_interfaces(hardware_types)
unreg_mock.assert_called_once_with(mock.ANY)
# we're iterating over dicts, don't worry about order

8
releasenotes/notes/dynamic-driver-conductor-startup-validation-2d56d6ebe748a6bb.yaml Normal file
View File

@ -0,0 +1,8 @@
---
features:
- The conductor process no longer requires at least one classic driver
to start. Instead, it requires at least one classic driver *or* at least
one dynamic driver.
upgrade:
- The conductor process will refuse to start if a dynamic driver and
a classic driver with the same name are both enabled.