openstack
/
ironic
Code Issues Proposed changes

Clean up driver loading in init_host 

init_host currently sets self._driver_factory and self.drivers, but
those aren't used elsewhere in the code. Don't set them as instance
attributes, but rather let them be discarded after init_host.

Additionally, BaseConductorManager._get_driver appears to be a copy of
driver_factory.get_driver; just proxy over to the driver_factory method
instead.

As tests appear to enforce ordering of the drivers in init_host, use an
OrderedDict in driver_factory.drivers() to maintain this. I don't
believe this is required for correctness, however I don't intend to
change behavior in this patch.

Change-Id: I861980d4402a7b0744376c274ce15631547368f3
This commit is contained in:
Jim Rollenhagen 2016-02-27 14:30:52 -05:00
parent 6af9da97e1
commit 22aa8a9eb4
5 changed files with 43 additions and 44 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
ironic/common/driver_factory.py
View File

@ -13,6 +13,8 @@
# License for the specific language governing permissions and limitations
# under the License.
import collections
from oslo_concurrency import lockutils
from oslo_config import cfg
from oslo_log import log
@ -69,7 +71,12 @@ def get_driver(driver_name):
def drivers():
"""Get all drivers as a dict name -> driver object."""
factory = DriverFactory()
return {name: factory[name].obj for name in factory.names}
# NOTE(jroll) I don't think this needs to be ordered, but
# ConductorManager.init_host seems to depend on this behavior (or at
# least the unit tests for it do), and it can't hurt much to keep it
# that way.
return collections.OrderedDict((name, factory[name].obj)
for name in factory.names)
class DriverFactory(object):

42
ironic/conductor/base_manager.py
View File

@ -66,26 +66,14 @@ class BaseConductorManager(object):
self.notifier = rpc.get_notifier()
self._started = False
def _get_driver(self, driver_name):
"""Get the driver.
:param driver_name: name of the driver.
:returns: the driver; an instance of a class which implements
:class:`ironic.drivers.base.BaseDriver`.
:raises: DriverNotFound if the driver is not loaded.
"""
try:
return self._driver_factory[driver_name].obj
except KeyError:
raise exception.DriverNotFound(driver_name=driver_name)
def init_host(self, admin_context=None):
"""Initialize the conductor host.
:param admin_context: the admin context to pass to periodic tasks.
:raises: RuntimeError when conductor is already running
:raises: NoDriversLoaded when no drivers are enabled on the conductor
:raises: RuntimeError when conductor is already running.
: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.
"""
if self._started:
raise RuntimeError(_('Attempt to start an already running '
@ -107,21 +95,19 @@ class BaseConductorManager(object):
self.ring_manager = hash.HashRingManager()
"""Consistent hash ring which maps drivers to conductors."""
# NOTE(deva): instantiating DriverFactory may raise DriverLoadError
# or DriverNotFound
self._driver_factory = driver_factory.DriverFactory()
"""Driver factory loads all enabled drivers."""
self.drivers = self._driver_factory.names
"""List of driver names which this conductor supports."""
if not self.drivers:
# NOTE(deva): this call may raise DriverLoadError or DriverNotFound
drivers = driver_factory.drivers()
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)
# 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
@ -131,7 +117,7 @@ class BaseConductorManager(object):
self._periodic_task_callables = []
periodic_task_classes = set()
self._collect_periodic_tasks(self, (admin_context,))
for driver_obj in driver_factory.drivers().values():
for driver_obj in drivers.values():
self._collect_periodic_tasks(driver_obj, (self, admin_context))
for iface_name in (driver_obj.core_interfaces +
driver_obj.standard_interfaces +
@ -158,7 +144,7 @@ class BaseConductorManager(object):
try:
# Register this conductor with the cluster
cdr = self.dbapi.register_conductor({'hostname': self.host,
'drivers': self.drivers})
'drivers': driver_names})
except exception.ConductorAlreadyRegistered:
# This conductor was already registered and did not shut down
# properly, so log a warning and update the record.
@ -167,7 +153,7 @@ class BaseConductorManager(object):
"was previously registered. Updating registration"),
{'hostname': self.host})
cdr = self.dbapi.register_conductor({'hostname': self.host,
'drivers': self.drivers},
'drivers': driver_names},
update_existing=True)
self.conductor = cdr

9
ironic/conductor/manager.py
View File

@ -54,6 +54,7 @@ from oslo_utils import excutils
from oslo_utils import uuidutils
from ironic.common import dhcp_factory
from ironic.common import driver_factory
from ironic.common import exception
from ironic.common.glance_service import service_utils as glance_utils
from ironic.common.i18n import _
@ -394,7 +395,7 @@ class ConductorManager(base_manager.BaseConductorManager):
# Any locking in a top-level vendor action will need to be done by the
# implementation, as there is little we could reasonably lock on here.
LOG.debug("RPC driver_vendor_passthru for driver %s." % driver_name)
driver = self._get_driver(driver_name)
driver = driver_factory.get_driver(driver_name)
if not getattr(driver, 'vendor', None):
raise exception.UnsupportedDriverExtension(
driver=driver_name,
@ -466,7 +467,7 @@ class ConductorManager(base_manager.BaseConductorManager):
# implementation, as there is little we could reasonably lock on here.
LOG.debug("RPC get_driver_vendor_passthru_methods for driver %s"
% driver_name)
driver = self._get_driver(driver_name)
driver = driver_factory.get_driver(driver_name)
if not getattr(driver, 'vendor', None):
raise exception.UnsupportedDriverExtension(
driver=driver_name,
@ -1818,7 +1819,7 @@ class ConductorManager(base_manager.BaseConductorManager):
"""
LOG.debug("RPC get_driver_properties called for driver %s.",
driver_name)
driver = self._get_driver(driver_name)
driver = driver_factory.get_driver(driver_name)
return driver.get_properties()
@periodics.periodic(spacing=CONF.conductor.send_sensor_data_interval)
@ -2130,7 +2131,7 @@ class ConductorManager(base_manager.BaseConductorManager):
LOG.debug("RPC get_raid_logical_disk_properties "
"called for driver %s" % driver_name)
driver = self._get_driver(driver_name)
driver = driver_factory.get_driver(driver_name)
if not getattr(driver, 'raid', None):
raise exception.UnsupportedDriverExtension(
driver=driver_name, extension='raid')

16
ironic/tests/unit/common/test_driver_factory.py
View File

@ -17,6 +17,7 @@ from stevedore import dispatch
from ironic.common import driver_factory
from ironic.common import exception
from ironic.drivers import base as drivers_base
from ironic.tests import base
@ -62,3 +63,18 @@ class DriverLoadTestCase(base.TestCase):
'__init__', self._fake_init_driver_err):
driver_factory.DriverFactory._init_extension_manager()
self.assertEqual(2, mock_em.call_count)
class GetDriverTestCase(base.TestCase):
def setUp(self):
super(GetDriverTestCase, self).setUp()
driver_factory.DriverFactory._extension_manager = None
self.config(enabled_drivers=['fake'])
def test_get_driver_known(self):
driver = driver_factory.get_driver('fake')
self.assertIsInstance(driver, drivers_base.BaseDriver)
def test_get_driver_unknown(self):
self.assertRaises(exception.DriverNotFound,
driver_factory.get_driver, 'unknown_driver')

11
ironic/tests/unit/conductor/test_manager.py
View File

@ -2299,23 +2299,12 @@ class DoNodeVerifyTestCase(mgr_utils.ServiceSetUpMixin,
@mgr_utils.mock_record_keepalive
class MiscTestCase(mgr_utils.ServiceSetUpMixin, mgr_utils.CommonMixIn,
tests_db_base.DbTestCase):
def test_get_driver_known(self):
self._start_service()
driver = self.service._get_driver('fake')
self.assertIsInstance(driver, drivers_base.BaseDriver)
def test_get_driver_unknown(self):
self._start_service()
self.assertRaises(exception.DriverNotFound,
self.service._get_driver, 'unknown_driver')
def test__mapped_to_this_conductor(self):
self._start_service()
n = utils.get_test_node()
self.assertTrue(self.service._mapped_to_this_conductor(n['uuid'],
'fake'))
self.assertFalse(self.service._mapped_to_this_conductor(n['uuid'],
'otherdriver'))
@mock.patch.object(images, 'is_whole_disk_image')