From c6a42b999aed3f0caae604db61c79f5dd6097321 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Fri, 27 Aug 2021 17:56:59 +0200 Subject: [PATCH] Clean up error handling in the emulator * Make sure we always return a redfish-style response * Don't excessively re-raise exceptions * Use NotFound/BadRequest initially instead of generic FishyError Change-Id: I313d79b35ad7a09615d6f0ec5764471b50a1609b --- sushy_tools/emulator/main.py | 47 ++++++------------- sushy_tools/emulator/resources/managers.py | 6 ++- .../resources/systems/libvirtdriver.py | 12 ++--- .../emulator/resources/systems/novadriver.py | 14 +++--- sushy_tools/emulator/resources/vmedia.py | 2 +- sushy_tools/error.py | 2 +- sushy_tools/tests/unit/emulator/test_main.py | 4 +- 7 files changed, 35 insertions(+), 52 deletions(-) diff --git a/sushy_tools/emulator/main.py b/sushy_tools/emulator/main.py index 83227822..98ecc915 100755 --- a/sushy_tools/emulator/main.py +++ b/sushy_tools/emulator/main.py @@ -36,7 +36,6 @@ from sushy_tools.emulator.resources.systems import novadriver from sushy_tools.emulator.resources import vmedia as vmddriver from sushy_tools.emulator.resources import volumes as voldriver from sushy_tools import error -from sushy_tools.error import FishyError def _render_error(message): @@ -177,7 +176,7 @@ def ensure_instance_access(decorated_func): @functools.wraps(decorated_func) def decorator(*args, **kwargs): if instance_denied(**kwargs): - raise FishyError('Error finding instance') + raise error.NotFound() return decorated_func(*args, **kwargs) @@ -340,11 +339,7 @@ def jsonify(obj_type, obj_version, obj): def manager_resource(identity): app.logger.debug('Serving resources for manager "%s"', identity) - try: - manager = app.managers.get_manager(identity) - except error.FishyError as exc: - return str(exc), 404 - + manager = app.managers.get_manager(identity) systems = app.managers.get_managed_systems(manager) chassis = app.managers.get_managed_chassis(manager) @@ -404,20 +399,13 @@ def virtual_media_collection_resource(identity): methods=['GET']) @returns_json def virtual_media_resource(identity, device): - try: - device_name = app.vmedia.get_device_name( - identity, device) + device_name = app.vmedia.get_device_name( + identity, device) - media_types = app.vmedia.get_device_media_types( - identity, device) + media_types = app.vmedia.get_device_media_types( + identity, device) - device_info = app.vmedia.get_device_image_info(identity, device) - - except error.FishyError as ex: - app.logger.warning( - 'Virtual media %s at manager %s error: ' - '%s', device, identity, ex) - return 'Not found', 404 + device_info = app.vmedia.get_device_image_info(identity, device) app.logger.debug('Serving virtual media %s at ' 'manager "%s"', device, identity) @@ -454,14 +442,7 @@ def virtual_media_patch(identity, device): if not isinstance(verify, bool): raise error.BadRequest("VerifyCertificate must be a boolean") - try: - app.vmedia.update_device_info(identity, device, verify=verify) - except error.FishyError as ex: - app.logger.warning( - 'Virtual media %s at manager %s error: ' - '%s', device, identity, ex) - raise error.NotFound("Virtual media device not found") - + app.vmedia.update_device_info(identity, device, verify=verify) return '', 204 else: raise error.BadRequest("Empty or malformed patch") @@ -670,7 +651,7 @@ def ethernet_interface(identity, nic_id): return flask.render_template( 'ethernet_interface.json', identity=identity, nic=nic) - return 'Not found', 404 + raise error.NotFound() @app.route('/redfish/v1/Systems//Processors', @@ -697,7 +678,7 @@ def processor(identity, processor_id): return flask.render_template( 'processor.json', identity=identity, processor=proc) - return 'Not found', 404 + raise error.NotFound() @app.route('/redfish/v1/Systems//Actions/ComputerSystem.Reset', @@ -791,7 +772,7 @@ def simple_storage(identity, simple_storage_id): storage_controller = simple_storage_controllers[simple_storage_id] except KeyError: app.logger.debug('"%s" Simple Storage resource was not found') - return 'Not found', 404 + raise error.NotFound() return flask.render_template('simple_storage.json', identity=identity, simple_storage=storage_controller) @@ -823,7 +804,7 @@ def storage(identity, storage_id): return flask.render_template( 'storage.json', identity=identity, storage=stg) - return 'Not found', 404 + raise error.NotFound() @app.route('/redfish/v1/Systems//Storage//Drives/', @@ -839,7 +820,7 @@ def drive_resource(identity, stg_id, drv_id): return flask.render_template( 'drive.json', identity=identity, storage_id=stg_id, drive=drv) - return 'Not found', 404 + raise error.NotFound() @app.route('/redfish/v1/Systems//Storage//Volumes', @@ -902,7 +883,7 @@ def volume(identity, stg_id, vol_id): 'volume.json', identity=identity, storage_id=stg_id, volume=vol) - return 'Not Found', 404 + raise error.NotFound() @app.route('/redfish/v1/Registries') diff --git a/sushy_tools/emulator/resources/managers.py b/sushy_tools/emulator/resources/managers.py index 709d689a..4e5a8859 100644 --- a/sushy_tools/emulator/resources/managers.py +++ b/sushy_tools/emulator/resources/managers.py @@ -26,16 +26,18 @@ class FakeDriver(base.DriverBase): """Get a manager by its identity :returns: Redfish manager UUID. + :raises: NotFound if the manager cannot be found """ try: system_uuid = self._systems.uuid(identity) system_name = self._systems.name(identity) except error.AliasAccessError: raise - except error.FishyError: + except error.NotFound: + # Re-raise hiding the fact that managers are backed by systems msg = 'Manager with UUID %s was not found' % identity self._logger.error(msg) - raise error.FishyError(msg) + raise error.NotFound(msg) else: result = {'Id': system_uuid, 'UUID': system_uuid, diff --git a/sushy_tools/emulator/resources/systems/libvirtdriver.py b/sushy_tools/emulator/resources/systems/libvirtdriver.py index 5b65f527..a0e806fe 100644 --- a/sushy_tools/emulator/resources/systems/libvirtdriver.py +++ b/sushy_tools/emulator/resources/systems/libvirtdriver.py @@ -185,7 +185,7 @@ class LibvirtDriver(AbstractSystemsDriver): self._logger.debug(msg) - raise error.FishyError(msg) + raise error.NotFound(msg) raise error.AliasAccessError(domain.UUIDString()) @@ -213,7 +213,7 @@ class LibvirtDriver(AbstractSystemsDriver): in place of system name if there are duplicates. :param identity: libvirt domain name or UUID - + :raises: NotFound if the system cannot be found :returns: computer system UUID """ domain = self._get_domain(identity, readonly=True) @@ -223,7 +223,7 @@ class LibvirtDriver(AbstractSystemsDriver): """Get computer system name by name :param identity: libvirt domain name or UUID - + :raises: NotFound if the system cannot be found :returns: computer system name """ domain = self._get_domain(identity, readonly=True) @@ -501,7 +501,7 @@ class LibvirtDriver(AbstractSystemsDriver): msg = ('Unknown boot mode requested: ' '%(boot_mode)s' % {'boot_mode': boot_mode}) - raise error.FishyError(msg) + raise error.BadRequest(msg) os_elements = tree.findall('os') if len(os_elements) != 1: @@ -939,7 +939,7 @@ class LibvirtDriver(AbstractSystemsDriver): lv_device = self.BOOT_DEVICE_MAP[device] except KeyError: - raise error.FishyError( + raise error.BadRequest( 'Unknown device %s at %s' % (device, identity)) disk_elements = device_element.findall('disk') @@ -1026,7 +1026,7 @@ class LibvirtDriver(AbstractSystemsDriver): lv_device = self.BOOT_DEVICE_MAP[device] except KeyError: - raise error.FishyError( + raise error.BadRequest( 'Unknown device %s at %s' % (device, identity)) device_element = domain_tree.find('devices') diff --git a/sushy_tools/emulator/resources/systems/novadriver.py b/sushy_tools/emulator/resources/systems/novadriver.py index f5013b60..e4bb04af 100644 --- a/sushy_tools/emulator/resources/systems/novadriver.py +++ b/sushy_tools/emulator/resources/systems/novadriver.py @@ -76,7 +76,7 @@ class OpenStackDriver(AbstractSystemsDriver): self._logger.debug(msg) - raise error.FishyError(msg) + raise error.NotFound(msg) @memoize.memoize(permanent_cache=PERMANENT_CACHE) def _get_flavor(self, identity): @@ -188,7 +188,7 @@ class OpenStackDriver(AbstractSystemsDriver): # NOTE(etingof) can't support `state == "Nmi"` as # openstacksdk does not seem to support that else: - raise error.FishyError( + raise error.BadRequest( 'Unknown ResetType "%(state)s"' % {'state': state}) def get_boot_device(self, identity): @@ -234,7 +234,7 @@ class OpenStackDriver(AbstractSystemsDriver): msg = ('Unknown power state requested: ' '%(boot_source)s' % {'boot_source': boot_source}) - raise error.FishyError(msg) + raise error.BadRequest(msg) # NOTE(etingof): the following probably only works with # libvirt-backed compute nodes @@ -271,7 +271,7 @@ class OpenStackDriver(AbstractSystemsDriver): msg = ('The cloud driver %(driver)s does not allow changing boot ' 'mode through Redfish' % {'driver': self.driver}) - raise error.FishyError(msg) + raise error.NotSupportedError(msg) def get_total_memory(self, identity): """Get computer system total memory @@ -307,17 +307,17 @@ class OpenStackDriver(AbstractSystemsDriver): def get_bios(self, identity): """Not supported as Openstack SDK does not expose API for BIOS""" - raise error.FishyError( + raise error.NotSupportedError( 'Operation not supported by the virtualization driver') def set_bios(self, identity, attributes): """Not supported as Openstack SDK does not expose API for BIOS""" - raise error.FishyError( + raise error.NotSupportedError( 'Operation not supported by the virtualization driver') def reset_bios(self, identity): """Not supported as Openstack SDK does not expose API for BIOS""" - raise error.FishyError( + raise error.NotSupportedError( 'Operation not supported by the virtualization driver') def get_nics(self, identity): diff --git a/sushy_tools/emulator/resources/vmedia.py b/sushy_tools/emulator/resources/vmedia.py index e2174118..5df7fcfd 100644 --- a/sushy_tools/emulator/resources/vmedia.py +++ b/sushy_tools/emulator/resources/vmedia.py @@ -76,7 +76,7 @@ class StaticDriver(base.DriverBase): return self._devices[(identity, device)] except KeyError: - raise error.FishyError( + raise error.NotFound( 'No such virtual media device %s owned by resource ' '%s' % (device, identity)) diff --git a/sushy_tools/error.py b/sushy_tools/error.py index f258a9a8..ecb1e69e 100644 --- a/sushy_tools/error.py +++ b/sushy_tools/error.py @@ -33,7 +33,7 @@ class NotSupportedError(FishyError): class NotFound(FishyError): """Entity not found.""" - def __init__(self, msg, code=404): + def __init__(self, msg='Not found', code=404): super().__init__(msg, code) diff --git a/sushy_tools/tests/unit/emulator/test_main.py b/sushy_tools/tests/unit/emulator/test_main.py index b9e0cd61..1d051466 100644 --- a/sushy_tools/tests/unit/emulator/test_main.py +++ b/sushy_tools/tests/unit/emulator/test_main.py @@ -536,7 +536,7 @@ class VirtualMediaTestCase(EmulatorTestCase): self.assertFalse(response.json['VerifyCertificate']) def test_virtual_media_not_found(self, managers_mock, vmedia_mock): - vmedia_mock.return_value.get_device_name.side_effect = error.FishyError + vmedia_mock.return_value.get_device_name.side_effect = error.NotFound response = self.app.get( '/redfish/v1/Managers/%s/VirtualMedia/DVD-ROM' % self.uuid) @@ -555,7 +555,7 @@ class VirtualMediaTestCase(EmulatorTestCase): def test_virtual_media_update_not_found(self, managers_mock, vmedia_mock): vmedia_mock = vmedia_mock.return_value - vmedia_mock.update_device_info.side_effect = error.FishyError + vmedia_mock.update_device_info.side_effect = error.NotFound response = self.app.patch( '/redfish/v1/Managers/%s/VirtualMedia/DVD-ROM' % self.uuid,