From 0a7fc6059a70486793353f20fb7152782a3f398a Mon Sep 17 00:00:00 2001 From: Ruby Loo Date: Thu, 2 Feb 2017 22:27:01 +0000 Subject: [PATCH] exception from driver_factory.default_interface() This changes driver_factory.default_interface() so that instead of returning None if there is no calculated default interface, it raises exception.NoValidDefaultForInterface. This is a follow up to 6206c47720eb16719088540a48b53fb22a4eb999. Change-Id: I0c3d5d75b5a37af02f3660968cf3f2c669e52019 Partial-Bug: #1524745 --- ironic/common/driver_factory.py | 33 +++++++--- ironic/common/exception.py | 7 ++- ironic/conductor/base_manager.py | 5 +- ironic/conductor/manager.py | 63 +++++++++++-------- ironic/conductor/rpcapi.py | 13 ++++ ironic/drivers/hardware_type.py | 16 +++-- .../tests/unit/common/test_driver_factory.py | 41 +++++++++++- .../tests/unit/conductor/test_base_manager.py | 5 +- ironic/tests/unit/conductor/test_manager.py | 38 +++++++++++ ironic/tests/unit/drivers/test_generic.py | 22 +++++++ 10 files changed, 195 insertions(+), 48 deletions(-) diff --git a/ironic/common/driver_factory.py b/ironic/common/driver_factory.py index e6105e6e2a..d8331da3d3 100644 --- a/ironic/common/driver_factory.py +++ b/ironic/common/driver_factory.py @@ -142,7 +142,8 @@ def get_interface(driver_or_hw_type, interface_type, interface_name): return impl_instance -def default_interface(driver_or_hw_type, interface_type): +def default_interface(driver_or_hw_type, interface_type, + driver_name=None, node=None): """Calculate and return the default interface implementation. Finds the first implementation that is supported by the hardware type @@ -150,9 +151,13 @@ def default_interface(driver_or_hw_type, interface_type): :param driver_or_hw_type: classic driver or hardware type instance object. :param interface_type: type of the interface (e.g. 'boot'). - :returns: an entrypoint name of the calculated default implementation - or None if no default implementation can be found. + :param driver_name: entrypoint name of the driver_or_hw_type object. Is + used for exception message. + :param node: the identifier of a node. If specified, is used for exception + message. + :returns: an entrypoint name of the calculated default implementation. :raises: InterfaceNotFoundInEntrypoint if the entry point was not found. + :raises: NoValidDefaultForInterface if no default interface can be found. """ factory = _INTERFACE_LOADERS[interface_type] is_hardware_type = isinstance(driver_or_hw_type, @@ -185,6 +190,21 @@ def default_interface(driver_or_hw_type, interface_type): except KeyError: continue + if impl_name is None: + # NOTE(rloo). No i18n on driver_type_str because translating substrings + # on their own may cause the final string to look odd. + if is_hardware_type: + driver_type_str = 'hardware type' + else: + driver_type_str = 'driver' + driver_name = driver_name or driver_or_hw_type.__class__.__name__ + node_info = "" + if node is not None: + node_info = _(' node %s with') % node + raise exception.NoValidDefaultForInterface( + interface_type=interface_type, driver_type=driver_type_str, + driver=driver_name, node_info=node_info) + return impl_name @@ -234,11 +254,8 @@ def check_and_update_node_interfaces(node, driver_or_hw_type=None): # Not changing the result, proceeding with the next interface continue - impl_name = default_interface(driver_or_hw_type, iface) - - if impl_name is None: - raise exception.NoValidDefaultForInterface( - interface_type=iface, node=node.uuid, driver=node.driver) + impl_name = default_interface(driver_or_hw_type, iface, + driver_name=node.driver, node=node.uuid) # Set the calculated default and set result to True setattr(node, field_name, impl_name) diff --git a/ironic/common/exception.py b/ironic/common/exception.py index 12e8ddd39e..bc1e5faccc 100644 --- a/ironic/common/exception.py +++ b/ironic/common/exception.py @@ -343,8 +343,11 @@ class IncompatibleInterface(InvalidParameterValue): class NoValidDefaultForInterface(InvalidParameterValue): - _msg_fmt = _("No default value found for %(interface_type)s interface " - "for node %(node)s with driver or hardware type %(driver)s.") + # NOTE(rloo): in the line below, there is no blank space after 'For' + # because node_info could be an empty string. If node_info + # is not empty, it should start with a space. + _msg_fmt = _("For%(node_info)s %(driver_type)s '%(driver)s', no default " + "value found for %(interface_type)s interface.") class ImageNotFound(NotFound): diff --git a/ironic/conductor/base_manager.py b/ironic/conductor/base_manager.py index b472b78fbb..2456d2f3b8 100644 --- a/ironic/conductor/base_manager.py +++ b/ironic/conductor/base_manager.py @@ -274,10 +274,7 @@ class BaseConductorManager(object): interface_map = driver_factory.enabled_supported_interfaces(ht) for interface_type, interface_names in interface_map.items(): default_interface = driver_factory.default_interface( - ht, interface_type) - if default_interface is None: - raise exception.NoValidDefaultForInterface( - interface_type=interface_type, node=None, driver=ht) + ht, interface_type, driver_name=ht_name) self.conductor.register_hardware_interfaces(ht_name, interface_type, interface_names, diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 968f728510..ea9247617b 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -136,6 +136,8 @@ class ConductorManager(base_manager.BaseConductorManager): :param context: an admin context :param node_obj: a changed (but not saved) node object. + :raises: NoValidDefaultForInterface if no default can be calculated + for some interfaces, and explicit values must be provided. """ node_id = node_obj.uuid @@ -334,6 +336,7 @@ class ConductorManager(base_manager.BaseConductorManager): exception.InvalidParameterValue, exception.UnsupportedDriverExtension, exception.DriverNotFound, + exception.NoValidDefaultForInterface, exception.InterfaceNotFoundInEntrypoint) def driver_vendor_passthru(self, context, driver_name, driver_method, http_method, info): @@ -362,6 +365,9 @@ class ConductorManager(base_manager.BaseConductorManager): :raises: DriverNotFound if the supplied driver is not loaded. :raises: NoFreeConductorWorker when there is no free worker to start async task. + :raises: NoValidDefaultForInterface if no default interface + implementation can be found for this driver's vendor + interface. :raises: InterfaceNotFoundInEntrypoint if the default interface for a hardware type is invalid. :returns: A dictionary containing: @@ -381,17 +387,16 @@ class ConductorManager(base_manager.BaseConductorManager): driver = driver_factory.get_driver_or_hardware_type(driver_name) vendor = None if isinstance(driver, hardware_type.AbstractHardwareType): - vendor_name = driver_factory.default_interface(driver, 'vendor') - if vendor_name is not None: - vendor = driver_factory.get_interface(driver, 'vendor', - vendor_name) + vendor_name = driver_factory.default_interface( + driver, 'vendor', driver_name=driver_name) + vendor = driver_factory.get_interface(driver, 'vendor', + vendor_name) else: vendor = getattr(driver, 'vendor', None) - - if not vendor: - raise exception.UnsupportedDriverExtension( - driver=driver_name, - extension='vendor interface') + if not vendor: + raise exception.UnsupportedDriverExtension( + driver=driver_name, + extension='vendor interface') try: vendor_opts = vendor.driver_routes[driver_method] @@ -449,6 +454,7 @@ class ConductorManager(base_manager.BaseConductorManager): @METRICS.timer('ConductorManager.get_driver_vendor_passthru_methods') @messaging.expected_exceptions(exception.UnsupportedDriverExtension, exception.DriverNotFound, + exception.NoValidDefaultForInterface, exception.InterfaceNotFoundInEntrypoint) def get_driver_vendor_passthru_methods(self, context, driver_name): """Retrieve information about vendor methods of the given driver. @@ -460,6 +466,9 @@ class ConductorManager(base_manager.BaseConductorManager): :raises: UnsupportedDriverExtension if current driver does not have vendor interface. :raises: DriverNotFound if the supplied driver is not loaded. + :raises: NoValidDefaultForInterface if no default interface + implementation can be found for this driver's vendor + interface. :raises: InterfaceNotFoundInEntrypoint if the default interface for a hardware type is invalid. :returns: dictionary of : entries. @@ -472,17 +481,16 @@ class ConductorManager(base_manager.BaseConductorManager): driver = driver_factory.get_driver_or_hardware_type(driver_name) vendor = None if isinstance(driver, hardware_type.AbstractHardwareType): - vendor_name = driver_factory.default_interface(driver, 'vendor') - if vendor_name is not None: - vendor = driver_factory.get_interface(driver, 'vendor', - vendor_name) + vendor_name = driver_factory.default_interface( + driver, 'vendor', driver_name=driver_name) + vendor = driver_factory.get_interface(driver, 'vendor', + vendor_name) else: vendor = getattr(driver, 'vendor', None) - - if not vendor: - raise exception.UnsupportedDriverExtension( - driver=driver_name, - extension='vendor interface') + if not vendor: + raise exception.UnsupportedDriverExtension( + driver=driver_name, + extension='vendor interface') return get_vendor_passthru_metadata(vendor.driver_routes) @@ -2380,6 +2388,7 @@ class ConductorManager(base_manager.BaseConductorManager): @METRICS.timer('ConductorManager.get_raid_logical_disk_properties') @messaging.expected_exceptions(exception.UnsupportedDriverExtension, + exception.NoValidDefaultForInterface, exception.InterfaceNotFoundInEntrypoint) def get_raid_logical_disk_properties(self, context, driver_name): """Get the logical disk properties for RAID configuration. @@ -2392,6 +2401,9 @@ class ConductorManager(base_manager.BaseConductorManager): :param driver_name: name of the driver :raises: UnsupportedDriverExtension, if the driver doesn't support RAID configuration. + :raises: NoValidDefaultForInterface if no default interface + implementation can be found for this driver's RAID + interface. :raises: InterfaceNotFoundInEntrypoint if the default interface for a hardware type is invalid. :returns: A dictionary containing the properties and a textual @@ -2403,16 +2415,15 @@ class ConductorManager(base_manager.BaseConductorManager): driver = driver_factory.get_driver_or_hardware_type(driver_name) raid_iface = None if isinstance(driver, hardware_type.AbstractHardwareType): - raid_iface_name = driver_factory.default_interface(driver, 'raid') - if raid_iface_name is not None: - raid_iface = driver_factory.get_interface(driver, 'raid', - raid_iface_name) + raid_iface_name = driver_factory.default_interface( + driver, 'raid', driver_name=driver_name) + raid_iface = driver_factory.get_interface(driver, 'raid', + raid_iface_name) else: raid_iface = getattr(driver, 'raid', None) - - if not raid_iface: - raise exception.UnsupportedDriverExtension( - driver=driver_name, extension='raid') + if not raid_iface: + raise exception.UnsupportedDriverExtension( + driver=driver_name, extension='raid') return raid_iface.get_logical_disk_properties() diff --git a/ironic/conductor/rpcapi.py b/ironic/conductor/rpcapi.py index 793967ae77..d559b0ed6a 100644 --- a/ironic/conductor/rpcapi.py +++ b/ironic/conductor/rpcapi.py @@ -158,6 +158,8 @@ class ConductorAPI(object): :returns: created node object. :raises: InterfaceNotFoundInEntrypoint if validation fails for any dynamic interfaces (e.g. network_interface). + :raises: NoValidDefaultForInterface if no default can be calculated + for some interfaces, and explicit values must be provided. """ cctxt = self.client.prepare(topic=topic or self.topic, version='1.36') return cctxt.call(context, 'create_node', node_obj=node_obj) @@ -178,6 +180,8 @@ class ConductorAPI(object): :param node_obj: a changed (but not saved) node object. :param topic: RPC topic. Defaults to self.topic. :returns: updated node object, including all fields. + :raises: NoValidDefaultForInterface if no default can be calculated + for some interfaces, and explicit values must be provided. """ cctxt = self.client.prepare(topic=topic or self.topic, version='1.1') @@ -268,6 +272,9 @@ class ConductorAPI(object): async task. :raises: InterfaceNotFoundInEntrypoint if the default interface for a hardware type is invalid. + :raises: NoValidDefaultForInterface if no default interface + implementation can be found for this driver's vendor + interface. :returns: A dictionary containing: :return: The response of the invoked vendor method @@ -311,6 +318,9 @@ class ConductorAPI(object): :raises: DriverNotFound if the supplied driver is not loaded. :raises: InterfaceNotFoundInEntrypoint if the default interface for a hardware type is invalid. + :raises: NoValidDefaultForInterface if no default interface + implementation can be found for this driver's vendor + interface. :returns: dictionary of : entries. """ @@ -675,6 +685,9 @@ class ConductorAPI(object): support RAID configuration. :raises: InterfaceNotFoundInEntrypoint if the default interface for a hardware type is invalid. + :raises: NoValidDefaultForInterface if no default interface + implementation can be found for this driver's RAID + interface. :returns: A dictionary containing the properties that can be mentioned for logical disks and a textual description for them. """ diff --git a/ironic/drivers/hardware_type.py b/ironic/drivers/hardware_type.py index 36b7880f19..2d0713a589 100644 --- a/ironic/drivers/hardware_type.py +++ b/ironic/drivers/hardware_type.py @@ -20,6 +20,7 @@ import abc import six +from ironic.common import exception from ironic.drivers import base as driver_base from ironic.drivers.modules.network import noop as noop_net from ironic.drivers.modules import noop @@ -106,9 +107,14 @@ class AbstractHardwareType(object): properties = {} for iface_type in driver_base.ALL_INTERFACES: - default_iface = driver_factory.default_interface(self, iface_type) - if default_iface is not None: - iface = driver_factory.get_interface(self, iface_type, - default_iface) - properties.update(iface.get_properties()) + try: + default_iface = driver_factory.default_interface(self, + iface_type) + except (exception.InterfaceNotFoundInEntrypoint, + exception.NoValidDefaultForInterface): + continue + + iface = driver_factory.get_interface(self, iface_type, + default_iface) + properties.update(iface.get_properties()) return properties diff --git a/ironic/tests/unit/common/test_driver_factory.py b/ironic/tests/unit/common/test_driver_factory.py index cfb2f3640d..e0cc2eec5c 100644 --- a/ironic/tests/unit/common/test_driver_factory.py +++ b/ironic/tests/unit/common/test_driver_factory.py @@ -310,8 +310,35 @@ class DefaultInterfaceTestCase(db_base.DbTestCase): # manual-management supports no power interfaces self.config(default_power_interface=None) self.config(enabled_power_interfaces=[]) - iface = driver_factory.default_interface(self.driver, 'power') - self.assertIsNone(iface) + self.assertRaisesRegex( + exception.NoValidDefaultForInterface, + "For hardware type 'ManualManagementHardware', no default " + "value found for power interface.", + driver_factory.default_interface, self.driver, 'power') + + 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') + + 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') class TestFakeHardware(hardware_type.AbstractHardwareType): @@ -502,6 +529,16 @@ class HardwareTypeLoadTestCase(db_base.DbTestCase): driver_factory.check_and_update_node_interfaces, node) + def test_no_raid_interface_no_default(self): + # NOTE(rloo): It doesn't seem possible to not have a default interface + # 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) + def _test_enabled_supported_interfaces(self, enable_storage): ht = fake_hardware.FakeHardware() expected = { diff --git a/ironic/tests/unit/conductor/test_base_manager.py b/ironic/tests/unit/conductor/test_base_manager.py index aff8c8f9d3..60b0ef3f02 100644 --- a/ironic/tests/unit/conductor/test_base_manager.py +++ b/ironic/tests/unit/conductor/test_base_manager.py @@ -353,13 +353,16 @@ class RegisterInterfacesTestCase(mgr_utils.ServiceSetUpMixin, ('deploy', ['agent', 'iscsi']), )), ] - default_mock.return_value = None + default_mock.side_effect = exception.NoValidDefaultForInterface("boo") self.assertRaises( exception.NoValidDefaultForInterface, self.service._register_and_validate_hardware_interfaces, hardware_types) + default_mock.assert_called_once_with( + hardware_types['fake-hardware'], + mock.ANY, driver_name='fake-hardware') unreg_mock.assert_called_once_with(mock.ANY) self.assertFalse(reg_mock.called) diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index 4b2b45e1ac..016fdaac2e 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -946,6 +946,25 @@ class VendorPassthruTestCase(mgr_utils.ServiceSetUpMixin, self.context, 'does_not_exist', 'test_method', 'POST', {}) + @mock.patch.object(driver_factory, 'default_interface', autospec=True) + def test_driver_vendor_passthru_no_default_interface(self, + mock_def_iface): + self.service.init_host() + # NOTE(rloo): service.init_host() will call + # driver_factory.default_interface() and we want these to + # succeed, so we set the side effect *after* that call. + mock_def_iface.reset_mock() + mock_def_iface.side_effect = exception.NoValidDefaultForInterface('no') + exc = self.assertRaises(messaging.ExpectedException, + self.service.driver_vendor_passthru, + self.context, 'fake-hardware', 'test_method', + 'POST', {}) + mock_def_iface.assert_called_once_with(mock.ANY, 'vendor', + driver_name='fake-hardware') + # Compare true exception hidden by @messaging.expected_exceptions + self.assertEqual(exception.NoValidDefaultForInterface, + exc.exc_info[0]) + @mock.patch.object(driver_factory, 'get_interface') def _test_get_driver_vendor_passthru_methods(self, is_hw_type, mock_get_if): @@ -994,6 +1013,25 @@ class VendorPassthruTestCase(mgr_utils.ServiceSetUpMixin, self.assertEqual(exception.UnsupportedDriverExtension, exc.exc_info[0]) + @mock.patch.object(driver_factory, 'default_interface', autospec=True) + def test_get_driver_vendor_passthru_methods_no_default_interface( + self, mock_def_iface): + self.service.init_host() + # NOTE(rloo): service.init_host() will call + # driver_factory.default_interface() and we want these to + # succeed, so we set the side effect *after* that call. + mock_def_iface.reset_mock() + mock_def_iface.side_effect = exception.NoValidDefaultForInterface('no') + exc = self.assertRaises( + messaging.rpc.ExpectedException, + self.service.get_driver_vendor_passthru_methods, + self.context, 'fake-hardware') + mock_def_iface.assert_called_once_with(mock.ANY, 'vendor', + driver_name='fake-hardware') + # Compare true exception hidden by @messaging.expected_exceptions + self.assertEqual(exception.NoValidDefaultForInterface, + exc.exc_info[0]) + @mock.patch.object(drivers_base.VendorInterface, 'driver_validate') def test_driver_vendor_passthru_validation_failed(self, validate_mock): validate_mock.side_effect = exception.MissingParameterValue('error') diff --git a/ironic/tests/unit/drivers/test_generic.py b/ironic/tests/unit/drivers/test_generic.py index c70999167f..4c4e5e9b6b 100644 --- a/ironic/tests/unit/drivers/test_generic.py +++ b/ironic/tests/unit/drivers/test_generic.py @@ -12,7 +12,12 @@ # License for the specific language governing permissions and limitations # under the License. +import mock + +from ironic.common import driver_factory +from ironic.common import exception from ironic.conductor import task_manager +from ironic.drivers import base as driver_base from ironic.drivers.modules import agent from ironic.drivers.modules import fake from ironic.drivers.modules import inspector @@ -57,3 +62,20 @@ class ManualManagementHardwareTestCase(db_base.DbTestCase): self.assertIsInstance(task.driver.deploy, agent.AgentDeploy) self.assertIsInstance(task.driver.inspect, inspector.Inspector) self.assertIsInstance(task.driver.raid, agent.AgentRAID) + + def test_get_properties(self): + # These properties are from vendor (agent) and boot (pxe) interfaces + expected_prop_keys = [ + 'deploy_forces_oob_reboot', 'deploy_kernel', 'deploy_ramdisk'] + hardware_type = driver_factory.get_hardware_type("manual-management") + properties = hardware_type.get_properties() + self.assertEqual(sorted(expected_prop_keys), sorted(properties.keys())) + + @mock.patch.object(driver_factory, 'default_interface', autospec=True) + def test_get_properties_none(self, mock_def_iface): + hardware_type = driver_factory.get_hardware_type("manual-management") + mock_def_iface.side_effect = exception.NoValidDefaultForInterface("no") + properties = hardware_type.get_properties() + self.assertEqual({}, properties) + self.assertEqual(len(driver_base.ALL_INTERFACES), + mock_def_iface.call_count)