From dc05d18b566174afd772b18e256089a664e4c5b6 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Thu, 5 Mar 2015 17:45:43 +0100 Subject: [PATCH] Start using in-band inspection Start using in-band inspection with ironic-discoverd, but do not instantiate the driver interface when discoverd support is disabled in the config file. * Check the CONF option prior to instantiating DiscoverdInspect class and binding it to any driver interface * Updates unit tests to better test the loading (or lack thereof) of the driver.inspect interface, based on the CONF option If in-band discovery is enabled by setting [discoverd] enabled=True in the config file, then the following drivers will bind it to their inspect interface: * pxe_ssh * pxe_ipmitool * pxe_ipminative * pxe_drac This patch also fixes a few nits (copied from previous reviews). The bug tagged below presently affects only the "fake" driver. Without the above-mentioned changes, enabling discoverd for any production-facing driver would have led to the problem described there. Updated UnsupportedDriverExtension error message to suggest that the extension might be disabled. Co-authored-by: Devananda van der Veen Closes-bug: #1431999 Implements: blueprint inband-properties-discovery Change-Id: I1dd844525852b1ec806f286d01dfc95313c6ad94 --- ironic/common/exception.py | 3 +- ironic/drivers/drac.py | 3 + ironic/drivers/fake.py | 3 + ironic/drivers/modules/discoverd.py | 36 ++++++++---- ironic/drivers/pxe.py | 7 +++ ironic/tests/drivers/test_discoverd.py | 77 ++++++++++++++++++-------- 6 files changed, 94 insertions(+), 35 deletions(-) diff --git a/ironic/common/exception.py b/ironic/common/exception.py index ec0dc5f77e..f539da8721 100644 --- a/ironic/common/exception.py +++ b/ironic/common/exception.py @@ -341,7 +341,8 @@ class OrphanedObjectError(IronicException): class UnsupportedDriverExtension(Invalid): - message = _('Driver %(driver)s does not support %(extension)s.') + message = _('Driver %(driver)s does not support %(extension)s ' + '(disabled or not implemented).') class IncompatibleObjectVersion(IronicException): diff --git a/ironic/drivers/drac.py b/ironic/drivers/drac.py index 2ac2200814..6f3fb2e65a 100644 --- a/ironic/drivers/drac.py +++ b/ironic/drivers/drac.py @@ -19,6 +19,7 @@ from oslo_utils import importutils from ironic.common import exception from ironic.common.i18n import _ from ironic.drivers import base +from ironic.drivers.modules import discoverd from ironic.drivers.modules.drac import management from ironic.drivers.modules.drac import power from ironic.drivers.modules import pxe @@ -37,3 +38,5 @@ class PXEDracDriver(base.BaseDriver): self.deploy = pxe.PXEDeploy() self.management = management.DracManagement() self.vendor = pxe.VendorPassthru() + self.inspect = discoverd.DiscoverdInspect.create_if_enabled( + 'PXEDracDriver') diff --git a/ironic/drivers/fake.py b/ironic/drivers/fake.py index d949f46409..aa97e56863 100644 --- a/ironic/drivers/fake.py +++ b/ironic/drivers/fake.py @@ -215,6 +215,9 @@ class FakeIPMIToolDiscoverdDriver(base.BaseDriver): self.deploy = fake.FakeDeploy() self.vendor = ipmitool.VendorPassthru() self.management = ipmitool.IPMIManagement() + # NOTE(dtantsur): unlike other uses of DiscoverdInspect, this one is + # unconditional, as this driver is designed for testing discoverd + # integration. self.inspect = discoverd.DiscoverdInspect() diff --git a/ironic/drivers/modules/discoverd.py b/ironic/drivers/modules/discoverd.py index 4bc7b8fc36..3a27279140 100644 --- a/ironic/drivers/modules/discoverd.py +++ b/ironic/drivers/modules/discoverd.py @@ -23,6 +23,7 @@ from ironic.common import exception from ironic.common.i18n import _ from ironic.common.i18n import _LE from ironic.common.i18n import _LI +from ironic.common.i18n import _LW from ironic.common import keystone from ironic.common import states from ironic.conductor import task_manager @@ -57,10 +58,25 @@ if ironic_discoverd: class DiscoverdInspect(base.InspectInterface): """In-band inspection via ironic-discoverd project.""" + @classmethod + def create_if_enabled(cls, driver_name): + """Create instance of DiscoverdInspect if it's enabled. + + Reports log warning with given driver_name if it's not. + + :return: DiscoverdInspect instance or None + """ + if CONF.discoverd.enabled: + return cls() + else: + LOG.warn(_LW("Inspection via ironic-discoverd is disabled in " + "configuration for driver %s, set " + "[discoverd]enabled = true to enable"), driver_name) + def __init__(self): if not CONF.discoverd.enabled: - LOG.info('ironic-discoverd support is disabled') - return + raise exception.DriverLoadError( + _('ironic-discoverd support is disabled')) if not ironic_discoverd: raise exception.DriverLoadError( @@ -70,7 +86,7 @@ class DiscoverdInspect(base.InspectInterface): version = getattr(ironic_discoverd, '__version_info__', (0, 2)) if version < (1, 0): raise exception.DriverLoadError( - _('ironic-discoverd version is too old: required >= 1.0.0 ' + _('ironic-discoverd version is too old: required >= 1.0.0, ' 'got %s') % '.'.join(str(x) for x in version)) def get_properties(self): @@ -81,18 +97,15 @@ class DiscoverdInspect(base.InspectInterface): return {} # no properties def validate(self, task): - """Validate the driver-specific management information. + """Validate the driver-specific inspection information. If invalid, raises an exception; otherwise returns None. :param task: a task from TaskManager. - :raises: UnsupportedDriverExtension if discoverd support is disabled """ - if not CONF.discoverd.enabled: - raise exception.UnsupportedDriverExtension( - _('ironic-discoverd support is disabled in ' - 'configuration, set [discoverd]enabled to true ' - 'to enable')) + # NOTE(deva): this is not callable if discoverd is disabled + # so don't raise an exception -- just pass. + pass def inspect_hardware(self, task): """Inspect hardware to obtain the hardware properties. @@ -185,7 +198,8 @@ def _check_status(task): LOG.error(_LE('Inspection failed for node %(uuid)s ' 'with error: %(err)s'), {'uuid': node.uuid, 'err': status['error']}) - node.last_error = _('ironic-discoverd: %s') % status['error'] + node.last_error = (_('ironic-discoverd inspection failed: %s') + % status['error']) task.process_event('fail') elif status.get('finished'): LOG.info(_LI('Inspection finished successfully for node %s'), diff --git a/ironic/drivers/pxe.py b/ironic/drivers/pxe.py index 39afdeddcf..ce2c44bc09 100644 --- a/ironic/drivers/pxe.py +++ b/ironic/drivers/pxe.py @@ -25,6 +25,7 @@ from ironic.drivers import base from ironic.drivers.modules.amt import management as amt_management from ironic.drivers.modules.amt import power as amt_power from ironic.drivers.modules.amt import vendor as amt_vendor +from ironic.drivers.modules import discoverd from ironic.drivers.modules import iboot from ironic.drivers.modules.ilo import deploy as ilo_deploy from ironic.drivers.modules.ilo import management as ilo_management @@ -56,6 +57,8 @@ class PXEAndIPMIToolDriver(base.BaseDriver): self.deploy = pxe.PXEDeploy() self.management = ipmitool.IPMIManagement() self.vendor = pxe.VendorPassthru() + self.inspect = discoverd.DiscoverdInspect.create_if_enabled( + 'PXEAndIPMIToolDriver') class PXEAndSSHDriver(base.BaseDriver): @@ -75,6 +78,8 @@ class PXEAndSSHDriver(base.BaseDriver): self.deploy = pxe.PXEDeploy() self.management = ssh.SSHManagement() self.vendor = pxe.VendorPassthru() + self.inspect = discoverd.DiscoverdInspect.create_if_enabled( + 'PXEAndSSHDriver') class PXEAndIPMINativeDriver(base.BaseDriver): @@ -98,6 +103,8 @@ class PXEAndIPMINativeDriver(base.BaseDriver): self.deploy = pxe.PXEDeploy() self.management = ipminative.NativeIPMIManagement() self.vendor = pxe.VendorPassthru() + self.inspect = discoverd.DiscoverdInspect.create_if_enabled( + 'PXEAndIPMINativeDriver') class PXEAndSeaMicroDriver(base.BaseDriver): diff --git a/ironic/tests/drivers/test_discoverd.py b/ironic/tests/drivers/test_discoverd.py index 85ee8f38d3..3e8945d2a9 100644 --- a/ironic/tests/drivers/test_discoverd.py +++ b/ironic/tests/drivers/test_discoverd.py @@ -26,9 +26,51 @@ from ironic.tests.db import base as db_base from ironic.tests.objects import utils as obj_utils +class DisabledTestCase(db_base.DbTestCase): + def setUp(self): + super(DisabledTestCase, self).setUp() + + def _do_mock(self): + # NOTE(dtantsur): fake driver always has inspection, using another one + mgr_utils.mock_the_extension_manager("pxe_ssh") + self.driver = driver_factory.get_driver("pxe_ssh") + + def test_disabled(self): + self.config(enabled=False, group='discoverd') + self._do_mock() + self.assertIsNone(self.driver.inspect) + # NOTE(dtantsur): it's expected that fake_discoverd fails to load + # in this case + self.assertRaises(exception.DriverLoadError, + mgr_utils.mock_the_extension_manager, + "fake_discoverd") + + def test_enabled(self): + self.config(enabled=True, group='discoverd') + self._do_mock() + self.assertIsNotNone(self.driver.inspect) + + @mock.patch.object(discoverd, 'ironic_discoverd', None) + def test_init_discoverd_not_imported(self): + self.assertRaises(exception.DriverLoadError, + discoverd.DiscoverdInspect) + + @mock.patch.object(ironic_discoverd, '__version_info__', (1, 0, 0)) + def test_init_ok(self): + self.config(enabled=True, group='discoverd') + discoverd.DiscoverdInspect() + + @mock.patch.object(ironic_discoverd, '__version_info__', (0, 2, 2)) + def test_init_old_version(self): + self.config(enabled=True, group='discoverd') + self.assertRaises(exception.DriverLoadError, + discoverd.DiscoverdInspect) + + class BaseTestCase(db_base.DbTestCase): def setUp(self): super(BaseTestCase, self).setUp() + self.config(enabled=True, group='discoverd') mgr_utils.mock_the_extension_manager("fake_discoverd") self.driver = driver_factory.get_driver("fake_discoverd") self.node = obj_utils.get_test_node(self.context) @@ -37,37 +79,26 @@ class BaseTestCase(db_base.DbTestCase): self.task.shared = False self.task.node = self.node self.task.driver = self.driver - self.config(enabled=True, group='discoverd') -class ValidateTestCase(BaseTestCase): +class CommonFunctionsTestCase(BaseTestCase): def test_validate_ok(self): self.driver.inspect.validate(self.task) - def test_validate_disabled(self): - self.config(enabled=False, group='discoverd') - self.assertRaises(exception.UnsupportedDriverExtension, - self.driver.inspect.validate, self.task) + def test_get_properties(self): + res = self.driver.inspect.get_properties() + self.assertEqual({}, res) - @mock.patch.object(ironic_discoverd, '__version_info__', (1, 0, 0)) - def test_init_ok(self): - discoverd.DiscoverdInspect() - self.config(enabled=False, group='discoverd') - discoverd.DiscoverdInspect() + def test_create_if_enabled(self): + res = discoverd.DiscoverdInspect.create_if_enabled('driver') + self.assertIsInstance(res, discoverd.DiscoverdInspect) - @mock.patch.object(ironic_discoverd, '__version_info__', (0, 2, 2)) - def test_old_version(self): - self.assertRaises(exception.DriverLoadError, - discoverd.DiscoverdInspect) + @mock.patch.object(discoverd.LOG, 'warn') + def test_create_if_enabled_disabled(self, warn_mock): self.config(enabled=False, group='discoverd') - discoverd.DiscoverdInspect() - - @mock.patch.object(discoverd, 'ironic_discoverd', None) - def test_not_imported(self): - self.assertRaises(exception.DriverLoadError, - discoverd.DiscoverdInspect) - self.config(enabled=False, group='discoverd') - discoverd.DiscoverdInspect() + res = discoverd.DiscoverdInspect.create_if_enabled('driver') + self.assertIsNone(res) + self.assertTrue(warn_mock.called) @mock.patch.object(eventlet, 'spawn_n', lambda f, *a, **kw: f(*a, **kw))