From 1b6a6e3916806849025f3a90cd6ec48811b02e8a Mon Sep 17 00:00:00 2001 From: Ghanshyam Mann Date: Thu, 26 Aug 2021 19:46:09 -0500 Subject: [PATCH] Convert features not supported error to HTTPBadRequest There is inconsistency on return code nova API return for "Feature not supported/implemented'. Current return code are 400, 409, and 403. - 400 case: Example: Multiattach Swap Volume Not Supported - 403 case: Cyborg integration - 409 case: Example: Operation Not Supported For SEV , Operation Not Supported For VTPM In xena PTG, we agreed to fix this by returning 400 in all cases - L446: https://etherpad.opendev.org/p/nova-xena-ptg This commit convert all the features not supported error to HTTPBadRequest(400). To avoid converting every NotSupported inherited exception in API controller to HTTPBadRequest generic conversion is added in expected_errors() decorator. Closes-Bug: #1938093 Change-Id: I410924668a73785f1bfe5c79827915d72e1d9e03 --- .../openstack/compute/attach_interfaces.py | 6 +--- nova/api/openstack/compute/evacuate.py | 7 ---- nova/api/openstack/compute/migrate_server.py | 12 +------ nova/api/openstack/compute/rescue.py | 2 -- nova/api/openstack/compute/servers.py | 14 ++------ nova/api/openstack/compute/shelve.py | 4 --- nova/api/openstack/compute/suspend_server.py | 8 +---- nova/api/openstack/compute/volumes.py | 3 +- nova/api/openstack/wsgi.py | 9 +++++ nova/exception.py | 34 ++++++++++++------- nova/tests/functional/test_servers.py | 16 ++++----- .../compute/test_attach_interfaces.py | 17 ++++++++++ .../api/openstack/compute/test_evacuate.py | 17 +++++++--- .../openstack/compute/test_migrate_server.py | 20 ++++++++--- .../unit/api/openstack/compute/test_rescue.py | 21 +++++++++--- .../openstack/compute/test_server_actions.py | 33 ++++++++++++++---- .../unit/api/openstack/compute/test_shelve.py | 20 ++++++----- .../openstack/compute/test_suspend_server.py | 14 +++----- .../api/openstack/compute/test_volumes.py | 2 +- nova/tests/unit/compute/test_api.py | 10 +++--- ...lemented-return-code-bf8beea51705271b.yaml | 8 +++++ 21 files changed, 163 insertions(+), 114 deletions(-) create mode 100644 releasenotes/notes/convert-features-not-implemented-return-code-bf8beea51705271b.yaml diff --git a/nova/api/openstack/compute/attach_interfaces.py b/nova/api/openstack/compute/attach_interfaces.py index 585a9d39d59b..a329420878ff 100644 --- a/nova/api/openstack/compute/attach_interfaces.py +++ b/nova/api/openstack/compute/attach_interfaces.py @@ -180,7 +180,6 @@ class InterfaceAttachmentController(wsgi.Controller): exception.ForbiddenPortsWithAccelerator) as e: raise exc.HTTPBadRequest(explanation=e.format_message()) except ( - exception.OperationNotSupportedForVDPAInterface, exception.InstanceIsLocked, exception.FixedIpAlreadyInUse, exception.PortInUse, @@ -218,10 +217,7 @@ class InterfaceAttachmentController(wsgi.Controller): instance, port_id=port_id) except exception.PortNotFound as e: raise exc.HTTPNotFound(explanation=e.format_message()) - except ( - exception.OperationNotSupportedForVDPAInterface, - exception.InstanceIsLocked, - ) as e: + except exception.InstanceIsLocked as e: raise exc.HTTPConflict(explanation=e.format_message()) except NotImplementedError: common.raise_feature_not_supported() diff --git a/nova/api/openstack/compute/evacuate.py b/nova/api/openstack/compute/evacuate.py index b09e592a2bb3..aa3581275932 100644 --- a/nova/api/openstack/compute/evacuate.py +++ b/nova/api/openstack/compute/evacuate.py @@ -130,13 +130,6 @@ class EvacuateController(wsgi.Controller): exception.ExtendedResourceRequestOldCompute, ) as e: raise exc.HTTPBadRequest(explanation=e.format_message()) - except exception.ForbiddenWithAccelerators as e: - raise exc.HTTPForbidden(explanation=e.format_message()) - except ( - exception.OperationNotSupportedForVTPM, - exception.OperationNotSupportedForVDPAInterface, - ) as e: - raise exc.HTTPConflict(explanation=e.format_message()) if (not api_version_request.is_supported(req, min_version='2.14') and CONF.api.enable_instance_password): diff --git a/nova/api/openstack/compute/migrate_server.py b/nova/api/openstack/compute/migrate_server.py index ebaeccff3d7b..855f51a7c64f 100644 --- a/nova/api/openstack/compute/migrate_server.py +++ b/nova/api/openstack/compute/migrate_server.py @@ -57,14 +57,12 @@ class MigrateServerController(wsgi.Controller): try: self.compute_api.resize(req.environ['nova.context'], instance, host_name=host_name) - except (exception.TooManyInstances, exception.QuotaError, - exception.ForbiddenWithAccelerators) as e: + except (exception.TooManyInstances, exception.QuotaError) as e: raise exc.HTTPForbidden(explanation=e.format_message()) except ( exception.InstanceIsLocked, exception.InstanceNotReady, exception.ServiceUnavailable, - exception.OperationNotSupportedForVDPAInterface, ) as e: raise exc.HTTPConflict(explanation=e.format_message()) except exception.InstanceInvalidState as state_error: @@ -143,12 +141,6 @@ class MigrateServerController(wsgi.Controller): "'%(ex)s'", {'ex': ex}) else: raise exc.HTTPBadRequest(explanation=ex.format_message()) - except ( - exception.OperationNotSupportedForSEV, - exception.OperationNotSupportedForVTPM, - exception.OperationNotSupportedForVDPAInterface, - ) as e: - raise exc.HTTPConflict(explanation=e.format_message()) except exception.InstanceIsLocked as e: raise exc.HTTPConflict(explanation=e.format_message()) except ( @@ -159,8 +151,6 @@ class MigrateServerController(wsgi.Controller): except exception.InstanceInvalidState as state_error: common.raise_http_conflict_for_instance_invalid_state(state_error, 'os-migrateLive', id) - except exception.ForbiddenWithAccelerators as e: - raise exc.HTTPForbidden(explanation=e.format_message()) def _get_force_param_for_live_migration(self, body, host): force = body["os-migrateLive"].get("force", False) diff --git a/nova/api/openstack/compute/rescue.py b/nova/api/openstack/compute/rescue.py index 80ad974fd8aa..baf3510920b5 100644 --- a/nova/api/openstack/compute/rescue.py +++ b/nova/api/openstack/compute/rescue.py @@ -65,8 +65,6 @@ class RescueController(wsgi.Controller): allow_bfv_rescue=allow_bfv_rescue) except ( exception.InstanceIsLocked, - exception.OperationNotSupportedForVTPM, - exception.OperationNotSupportedForVDPAInterface, exception.InvalidVolume, ) as e: raise exc.HTTPConflict(explanation=e.format_message()) diff --git a/nova/api/openstack/compute/servers.py b/nova/api/openstack/compute/servers.py index 65721e849632..a7370223d645 100644 --- a/nova/api/openstack/compute/servers.py +++ b/nova/api/openstack/compute/servers.py @@ -1039,12 +1039,10 @@ class ServersController(wsgi.Controller): try: self.compute_api.resize(context, instance, flavor_id, auto_disk_config=auto_disk_config) - except (exception.QuotaError, - exception.ForbiddenWithAccelerators) as error: + except exception.QuotaError as error: raise exc.HTTPForbidden( explanation=error.format_message()) except ( - exception.OperationNotSupportedForVDPAInterface, exception.InstanceIsLocked, exception.InstanceNotReady, exception.MixedInstanceNotSupportByComputeService, @@ -1067,7 +1065,6 @@ class ServersController(wsgi.Controller): exception.CannotResizeDisk, exception.CannotResizeToSameFlavor, exception.FlavorNotFound, - exception.ForbiddenPortsWithAccelerator, exception.ExtendedResourceRequestOldCompute, ) as e: raise exc.HTTPBadRequest(explanation=e.format_message()) @@ -1212,11 +1209,7 @@ class ServersController(wsgi.Controller): image_href, password, **kwargs) - except ( - exception.InstanceIsLocked, - exception.OperationNotSupportedForVTPM, - exception.OperationNotSupportedForVDPAInterface, - ) as e: + except exception.InstanceIsLocked as e: raise exc.HTTPConflict(explanation=e.format_message()) except exception.InstanceInvalidState as state_error: common.raise_http_conflict_for_instance_invalid_state(state_error, @@ -1230,8 +1223,7 @@ class ServersController(wsgi.Controller): except exception.KeypairNotFound: msg = _("Invalid key_name provided.") raise exc.HTTPBadRequest(explanation=msg) - except (exception.QuotaError, - exception.ForbiddenWithAccelerators) as error: + except exception.QuotaError as error: raise exc.HTTPForbidden(explanation=error.format_message()) except (exception.AutoDiskConfigDisabledByImage, exception.CertificateValidationFailed, diff --git a/nova/api/openstack/compute/shelve.py b/nova/api/openstack/compute/shelve.py index b985d49a9a56..7e1601a2a822 100644 --- a/nova/api/openstack/compute/shelve.py +++ b/nova/api/openstack/compute/shelve.py @@ -49,13 +49,9 @@ class ShelveController(wsgi.Controller): self.compute_api.shelve(context, instance) except ( exception.InstanceIsLocked, - exception.OperationNotSupportedForVTPM, - exception.OperationNotSupportedForVDPAInterface, exception.UnexpectedTaskStateError, ) as e: raise exc.HTTPConflict(explanation=e.format_message()) - except exception.ForbiddenWithAccelerators as e: - raise exc.HTTPForbidden(explanation=e.format_message()) except exception.InstanceInvalidState as state_error: common.raise_http_conflict_for_instance_invalid_state(state_error, 'shelve', id) diff --git a/nova/api/openstack/compute/suspend_server.py b/nova/api/openstack/compute/suspend_server.py index 600f91e8e24b..8495e1eb9212 100644 --- a/nova/api/openstack/compute/suspend_server.py +++ b/nova/api/openstack/compute/suspend_server.py @@ -38,17 +38,11 @@ class SuspendServerController(wsgi.Controller): target={'user_id': server.user_id, 'project_id': server.project_id}) self.compute_api.suspend(context, server) - except ( - exception.OperationNotSupportedForSEV, - exception.OperationNotSupportedForVDPAInterface, - exception.InstanceIsLocked, - ) as e: + except exception.InstanceIsLocked as e: raise exc.HTTPConflict(explanation=e.format_message()) except exception.InstanceInvalidState as state_error: common.raise_http_conflict_for_instance_invalid_state(state_error, 'suspend', id) - except exception.ForbiddenWithAccelerators as e: - raise exc.HTTPForbidden(explanation=e.format_message()) except exception.ForbiddenPortsWithAccelerator as e: raise exc.HTTPBadRequest(explanation=e.format_message()) diff --git a/nova/api/openstack/compute/volumes.py b/nova/api/openstack/compute/volumes.py index 3c39f50272f0..003f96deab82 100644 --- a/nova/api/openstack/compute/volumes.py +++ b/nova/api/openstack/compute/volumes.py @@ -392,8 +392,7 @@ class VolumeAttachmentController(wsgi.Controller): except exception.VolumeNotFound as e: raise exc.HTTPNotFound(explanation=e.format_message()) except (exception.InstanceIsLocked, - exception.DevicePathInUse, - exception.MultiattachNotSupportedByVirtDriver) as e: + exception.DevicePathInUse) as e: raise exc.HTTPConflict(explanation=e.format_message()) except exception.InstanceInvalidState as state_error: common.raise_http_conflict_for_instance_invalid_state(state_error, diff --git a/nova/api/openstack/wsgi.py b/nova/api/openstack/wsgi.py index 8cee8f769a7c..e64b4a2016a4 100644 --- a/nova/api/openstack/wsgi.py +++ b/nova/api/openstack/wsgi.py @@ -671,6 +671,15 @@ def expected_errors(errors): # calls. ResourceExceptionHandler silently # converts NotAuthorized to HTTPForbidden raise + elif isinstance(exc, exception.NotSupported): + # Note(gmann): Special case to handle + # NotSupported exceptions. We want to raise 400 BadRequest + # for the NotSupported exception which is basically used + # to raise for not supported features. Converting it here + # will avoid converting every NotSupported inherited + # exception in API controller. + raise webob.exc.HTTPBadRequest( + explanation=exc.format_message()) elif isinstance(exc, exception.ValidationError): # Note(oomichi): Handle a validation error, which # happens due to invalid API parameters, as an diff --git a/nova/exception.py b/nova/exception.py index e6db034605b3..5ee06a737914 100644 --- a/nova/exception.py +++ b/nova/exception.py @@ -155,12 +155,24 @@ class Forbidden(NovaException): code = 403 -class ForbiddenWithAccelerators(Forbidden): - msg_fmt = _("Forbidden with instances that have accelerators.") +class NotSupported(NovaException): + # This exception use return code as 400 and can be used + # directly or as base exception for operations whihc are not + # supported in Nova. Any feature that is not yet implemented + # but plan to implement in future (example: Cyborg + # integration operations), should use this exception as base + # and override the msg_fmt with feature details. + # Example: MultiattachNotSupportedByVirtDriver exception. + msg_fmt = _("Bad Request - Feature is not supported in Nova") + code = 400 -class ForbiddenPortsWithAccelerator(Forbidden): - msg_fmt = _("Forbidden with Ports that have accelerators.") +class ForbiddenWithAccelerators(NotSupported): + msg_fmt = _("Feature not supported with instances that have accelerators.") + + +class ForbiddenPortsWithAccelerator(NotSupported): + msg_fmt = _("Feature not supported with Ports that have accelerators.") class AdminRequired(Forbidden): @@ -281,14 +293,13 @@ class VolumeExtendFailed(Invalid): "Reason: %(reason)s") -class MultiattachNotSupportedByVirtDriver(NovaException): +class MultiattachNotSupportedByVirtDriver(NotSupported): # This exception indicates the compute hosting the instance does not # support multiattach volumes. This should generally be considered a - # 409 HTTPConflict error in the API since we expect all virt drivers to + # 400 HTTPBadRequest error in the API since we expect all virt drivers to # eventually support multiattach volumes. msg_fmt = _("Volume %(volume_id)s has 'multiattach' set, " "which is not supported for this instance.") - code = 409 class MultiattachNotSupportedOldMicroversion(Invalid): @@ -538,24 +549,21 @@ class UnableToMigrateToSelf(Invalid): "to current host (%(host)s).") -class OperationNotSupportedForSEV(NovaException): +class OperationNotSupportedForSEV(NotSupported): msg_fmt = _("Operation '%(operation)s' not supported for SEV-enabled " "instance (%(instance_uuid)s).") - code = 409 -class OperationNotSupportedForVTPM(NovaException): +class OperationNotSupportedForVTPM(NotSupported): msg_fmt = _("Operation '%(operation)s' not supported for vTPM-enabled " "instance (%(instance_uuid)s).") - code = 409 -class OperationNotSupportedForVDPAInterface(NovaException): +class OperationNotSupportedForVDPAInterface(NotSupported): msg_fmt = _( "Operation '%(operation)s' not supported for instance with " "vDPA ports ((instance_uuid)s)." ) - code = 409 class InvalidHypervisorType(Invalid): diff --git a/nova/tests/functional/test_servers.py b/nova/tests/functional/test_servers.py index 6ff7a670228a..117bbe8a4127 100644 --- a/nova/tests/functional/test_servers.py +++ b/nova/tests/functional/test_servers.py @@ -5909,26 +5909,26 @@ class AcceleratorServerOpsTest(AcceleratorServerBase): ex = self.assertRaises(client.OpenStackApiException, self.api.post_server_action, self.server['id'], {'resize': {'flavorRef': '2', 'OS-DCF:diskConfig': 'AUTO'}}) - self.assertEqual(403, ex.response.status_code) + self.assertEqual(400, ex.response.status_code) self._check_allocations_usage(self.server) def test_suspend_fails(self): ex = self.assertRaises(client.OpenStackApiException, self.api.post_server_action, self.server['id'], {'suspend': {}}) - self.assertEqual(403, ex.response.status_code) + self.assertEqual(400, ex.response.status_code) self._check_allocations_usage(self.server) def test_migrate_fails(self): ex = self.assertRaises(client.OpenStackApiException, self.api.post_server_action, self.server['id'], {'migrate': {}}) - self.assertEqual(403, ex.response.status_code) + self.assertEqual(400, ex.response.status_code) self._check_allocations_usage(self.server) def test_live_migrate_fails(self): ex = self.assertRaises(client.OpenStackApiException, self.api.post_server_action, self.server['id'], {'migrate': {'host': 'accel_host1'}}) - self.assertEqual(403, ex.response.status_code) + self.assertEqual(400, ex.response.status_code) self._check_allocations_usage(self.server) @mock.patch.object(objects.service, 'get_minimum_version_all_cells') @@ -5945,7 +5945,7 @@ class AcceleratorServerOpsTest(AcceleratorServerBase): {'evacuate': { 'host': compute_to_evacuate.host, 'adminPass': 'MySecretPass'}}) - self.assertEqual(403, ex.response.status_code) + self.assertEqual(400, ex.response.status_code) self._check_allocations_usage(self.server) @mock.patch.object(objects.service, 'get_minimum_version_all_cells') @@ -5961,7 +5961,7 @@ class AcceleratorServerOpsTest(AcceleratorServerBase): {'rebuild': { 'imageRef': rebuild_image_ref, 'OS-DCF:diskConfig': 'AUTO'}}) - self.assertEqual(403, ex.response.status_code) + self.assertEqual(400, ex.response.status_code) self._check_allocations_usage(self.server) @mock.patch.object(objects.service, 'get_minimum_version_all_cells') @@ -5973,7 +5973,7 @@ class AcceleratorServerOpsTest(AcceleratorServerBase): ex = self.assertRaises(client.OpenStackApiException, self.api.post_server_action, self.server['id'], {'shelve': {}}) - self.assertEqual(403, ex.response.status_code) + self.assertEqual(400, ex.response.status_code) self._check_allocations_usage(self.server) def _test_shelve_instance_with_compute_rpc_pin( @@ -5990,7 +5990,7 @@ class AcceleratorServerOpsTest(AcceleratorServerBase): ex = self.assertRaises( client.OpenStackApiException, self.api.post_server_action, self.server['id'], {'shelve': {}}) - self.assertEqual(403, ex.response.status_code) + self.assertEqual(400, ex.response.status_code) def test_shelve_instance_5_13(self): body = {'shelve': {}} diff --git a/nova/tests/unit/api/openstack/compute/test_attach_interfaces.py b/nova/tests/unit/api/openstack/compute/test_attach_interfaces.py index 26ebab0bea3b..610bdaa8c32d 100644 --- a/nova/tests/unit/api/openstack/compute/test_attach_interfaces.py +++ b/nova/tests/unit/api/openstack/compute/test_attach_interfaces.py @@ -252,6 +252,23 @@ class InterfaceAttachTestsV21(test.NoDBTestCase): self.assertRaises(exc.HTTPBadRequest, self.attachments.create, self.req, FAKE_UUID1, body=body) + @mock.patch.object( + compute_api.API, 'attach_interface', + side_effect=exception.OperationNotSupportedForVDPAInterface( + instance_uuid=FAKE_UUID1, operation='foo')) + def test_interface_vdpa_interface_not_supported(self, mock_attach): + body = {'interfaceAttachment': {'net_id': FAKE_NET_ID2}} + self.assertRaises(exc.HTTPBadRequest, self.attachments.create, + self.req, FAKE_UUID1, body=body) + + @mock.patch.object( + compute_api.API, 'detach_interface', + side_effect=exception.OperationNotSupportedForVDPAInterface( + instance_uuid=FAKE_UUID1, operation='foo')) + def test_detach_interface_vdpa_interface_not_supported(self, mock_detach): + self.assertRaises(exc.HTTPBadRequest, self.attachments.delete, + self.req, FAKE_UUID1, FAKE_NET_ID1) + def test_attach_interface_with_network_id(self): self.stub_out('nova.compute.api.API.attach_interface', fake_attach_interface) diff --git a/nova/tests/unit/api/openstack/compute/test_evacuate.py b/nova/tests/unit/api/openstack/compute/test_evacuate.py index 3bb9dcea5100..6620d7a1805c 100644 --- a/nova/tests/unit/api/openstack/compute/test_evacuate.py +++ b/nova/tests/unit/api/openstack/compute/test_evacuate.py @@ -115,20 +115,29 @@ class EvacuateTestV21(test.NoDBTestCase): uuid='BAD_UUID') @mock.patch('nova.compute.api.API.evacuate') - def test_evacuate__with_vtpm(self, mock_evacuate): + def test_evacuate_raise_badrequest_with_vtpm(self, mock_evacuate): mock_evacuate.side_effect = exception.OperationNotSupportedForVTPM( instance_uuid=uuids.instance, operation='foo') self._check_evacuate_failure( - webob.exc.HTTPConflict, + webob.exc.HTTPBadRequest, {'host': 'foo', 'onSharedStorage': 'False', 'adminPass': 'bar'}) @mock.patch('nova.compute.api.API.evacuate') - def test_evacuate__with_vdpa_interface(self, mock_evacuate): + def test_evacuate_raise_badrequest_with_vdpa_interface( + self, mock_evacuate): mock_evacuate.side_effect = \ exception.OperationNotSupportedForVDPAInterface( instance_uuid=uuids.instance, operation='foo') self._check_evacuate_failure( - webob.exc.HTTPConflict, + webob.exc.HTTPBadRequest, + {'host': 'foo', 'onSharedStorage': 'False', 'adminPass': 'bar'}) + + @mock.patch('nova.compute.api.API.evacuate') + def test_evacuate_raise_badrequest_for_accelerator(self, mock_evacuate): + mock_evacuate.side_effect = \ + exception.ForbiddenWithAccelerators() + self._check_evacuate_failure( + webob.exc.HTTPBadRequest, {'host': 'foo', 'onSharedStorage': 'False', 'adminPass': 'bar'}) def test_evacuate_with_active_service(self): diff --git a/nova/tests/unit/api/openstack/compute/test_migrate_server.py b/nova/tests/unit/api/openstack/compute/test_migrate_server.py index b4b49f77a160..683759eccc55 100644 --- a/nova/tests/unit/api/openstack/compute/test_migrate_server.py +++ b/nova/tests/unit/api/openstack/compute/test_migrate_server.py @@ -140,6 +140,15 @@ class MigrateServerTestsV21(admin_only_action_common.CommonTests): allowed=0) self._test_migrate_exception(exc_info, webob.exc.HTTPForbidden) + def test_migrate_raise_badrequest_for_accelerator(self): + exc_info = exception.ForbiddenWithAccelerators() + self._test_migrate_exception(exc_info, webob.exc.HTTPBadRequest) + + def test_migrate_raise_badrequest_for_vdpainterface(self): + exc_info = exception.OperationNotSupportedForVDPAInterface( + instance_uuid=uuids.instance, operation='foo') + self._test_migrate_exception(exc_info, webob.exc.HTTPBadRequest) + def _test_migrate_live_succeeded(self, param): instance = self._stub_instance_get() @@ -290,21 +299,21 @@ class MigrateServerTestsV21(admin_only_action_common.CommonTests): self._test_migrate_live_failed_with_exception( exception.OperationNotSupportedForSEV( instance_uuid=uuids.instance, operation='foo'), - expected_exc=webob.exc.HTTPConflict, + expected_exc=webob.exc.HTTPBadRequest, check_response=False) def test_migrate_live_vtpm_not_supported(self): self._test_migrate_live_failed_with_exception( exception.OperationNotSupportedForVTPM( instance_uuid=uuids.instance, operation='foo'), - expected_exc=webob.exc.HTTPConflict, + expected_exc=webob.exc.HTTPBadRequest, check_response=False) def test_migrate_live_vdpa_interfaces_not_supported(self): self._test_migrate_live_failed_with_exception( exception.OperationNotSupportedForVDPAInterface( instance_uuid=uuids.instance, operation='foo'), - expected_exc=webob.exc.HTTPConflict, + expected_exc=webob.exc.HTTPBadRequest, check_response=False) def test_migrate_live_pre_check_error(self): @@ -319,9 +328,10 @@ class MigrateServerTestsV21(admin_only_action_common.CommonTests): @mock.patch('nova.compute.api.API.live_migrate', side_effect=exception.ForbiddenWithAccelerators) - def test_live_migration_raises_http_forbidden(self, mock_migrate): + def test_live_migration_raises_badrequest_for_accelerators( + self, mock_migrate): body = self._get_migration_body(host='hostname') - self.assertRaises(webob.exc.HTTPForbidden, + self.assertRaises(webob.exc.HTTPBadRequest, self.controller._migrate_live, self.req, fakes.FAKE_UUID, body=body) diff --git a/nova/tests/unit/api/openstack/compute/test_rescue.py b/nova/tests/unit/api/openstack/compute/test_rescue.py index c0044f878229..28b8217d1a82 100644 --- a/nova/tests/unit/api/openstack/compute/test_rescue.py +++ b/nova/tests/unit/api/openstack/compute/test_rescue.py @@ -67,10 +67,6 @@ class RescueTestV21(test.NoDBTestCase): @ddt.data( exception.InstanceIsLocked(instance_uuid=uuids.instance), - exception.OperationNotSupportedForVTPM( - instance_uuid=uuids.instance, operation='foo'), - exception.OperationNotSupportedForVDPAInterface( - instance_uuid=uuids.instance, operation='foo'), exception.InvalidVolume(reason='foo'), ) @mock.patch.object(compute.api.API, 'rescue') @@ -83,6 +79,23 @@ class RescueTestV21(test.NoDBTestCase): self.fake_req, UUID, body=body) self.assertTrue(mock_rescue.called) + @ddt.data( + exception.OperationNotSupportedForVTPM( + instance_uuid=uuids.instance, operation='foo'), + exception.OperationNotSupportedForVDPAInterface( + instance_uuid=uuids.instance, operation='foo'), + ) + @mock.patch.object(compute.api.API, 'rescue') + def test_rescue_raise_badrequest_for_not_supported_features( + self, exc, mock_rescue): + """Test that exceptions are translated into HTTP BadRequest errors.""" + mock_rescue.side_effect = exc + body = {"rescue": {"adminPass": "AABBCC112233"}} + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller._rescue, + self.fake_req, UUID, body=body) + self.assertTrue(mock_rescue.called) + def test_rescue_with_preset_password(self): body = {"rescue": {"adminPass": "AABBCC112233"}} resp = self.controller._rescue(self.fake_req, UUID, body=body) diff --git a/nova/tests/unit/api/openstack/compute/test_server_actions.py b/nova/tests/unit/api/openstack/compute/test_server_actions.py index 1276aafb7d63..d07924abe84e 100644 --- a/nova/tests/unit/api/openstack/compute/test_server_actions.py +++ b/nova/tests/unit/api/openstack/compute/test_server_actions.py @@ -376,10 +376,6 @@ class ServerActionsControllerTestV21(test.TestCase): @ddt.data( exception.InstanceIsLocked(instance_uuid=uuids.instance), - exception.OperationNotSupportedForVTPM( - instance_uuid=uuids.instance, operation='foo'), - exception.OperationNotSupportedForVDPAInterface( - instance_uuid=uuids.instance, operation='foo'), ) @mock.patch('nova.compute.api.API.rebuild') def test_rebuild__http_conflict_error(self, exc, mock_rebuild): @@ -390,6 +386,22 @@ class ServerActionsControllerTestV21(test.TestCase): self.req, uuids.instance, body={'rebuild': {'imageRef': uuids.image}}) + @ddt.data( + exception.ForbiddenWithAccelerators(), + exception.OperationNotSupportedForVTPM( + instance_uuid=uuids.instance, operation='foo'), + exception.OperationNotSupportedForVDPAInterface( + instance_uuid=uuids.instance, operation='foo'), + ) + @mock.patch('nova.compute.api.API.rebuild') + def test_rebuild_raises_badrequest_for_not_supported_features( + self, exc, mock_rebuild): + mock_rebuild.side_effect = exc + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller._action_rebuild, + self.req, uuids.instance, + body={'rebuild': {'imageRef': uuids.image}}) + def test_rebuild_raises_conflict_on_invalid_state(self): body = {'rebuild': {'imageRef': uuids.image}} @@ -860,9 +872,18 @@ class ServerActionsControllerTestV21(test.TestCase): @mock.patch('nova.compute.api.API.resize', side_effect=exception.ForbiddenWithAccelerators) - def test_resize_raises_http_forbidden(self, mock_resize): + def test_resize_raises_badrequest_for_accelerator(self, mock_resize): body = dict(resize=dict(flavorRef="http://localhost/3")) - self.assertRaises(webob.exc.HTTPForbidden, + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller._action_resize, + self.req, FAKE_UUID, body=body) + + @mock.patch('nova.compute.api.API.resize', + side_effect=exception.OperationNotSupportedForVDPAInterface( + instance_uuid=FAKE_UUID, operation='foo')) + def test_resize_raises_badrequest_for_vdpaInterface(self, mock_resize): + body = dict(resize=dict(flavorRef="http://localhost/3")) + self.assertRaises(webob.exc.HTTPBadRequest, self.controller._action_resize, self.req, FAKE_UUID, body=body) diff --git a/nova/tests/unit/api/openstack/compute/test_shelve.py b/nova/tests/unit/api/openstack/compute/test_shelve.py index 73e39e3c3014..68e523be4744 100644 --- a/nova/tests/unit/api/openstack/compute/test_shelve.py +++ b/nova/tests/unit/api/openstack/compute/test_shelve.py @@ -41,10 +41,6 @@ class ShelveControllerTest(test.NoDBTestCase): @ddt.data( exception.InstanceIsLocked(instance_uuid=uuids.instance), - exception.OperationNotSupportedForVTPM( - instance_uuid=uuids.instance, operation='foo'), - exception.OperationNotSupportedForVDPAInterface( - instance_uuid=uuids.instance, operation='foo'), exception.UnexpectedTaskStateError( instance_uuid=uuids.instance, expected=None, actual=task_states.SHELVING), @@ -62,17 +58,23 @@ class ShelveControllerTest(test.NoDBTestCase): webob.exc.HTTPConflict, self.controller._shelve, self.req, uuids.fake, {}) + @ddt.data( + exception.ForbiddenWithAccelerators(), + exception.OperationNotSupportedForVTPM( + instance_uuid=uuids.instance, operation='foo'), + exception.OperationNotSupportedForVDPAInterface( + instance_uuid=uuids.instance, operation='foo'), + ) @mock.patch('nova.compute.api.API.shelve') @mock.patch('nova.api.openstack.common.get_instance') - def test_shelve_raise_http_forbidden( - self, mock_get_instance, mock_shelve, + def test_shelve_raise_badrequest_for_not_supported_feature( + self, exc, mock_get_instance, mock_shelve, ): mock_get_instance.return_value = ( fake_instance.fake_instance_obj(self.req.environ['nova.context'])) - mock_shelve.side_effect = exception.ForbiddenWithAccelerators - + mock_shelve.side_effect = exc self.assertRaises( - webob.exc.HTTPForbidden, self.controller._shelve, + webob.exc.HTTPBadRequest, self.controller._shelve, self.req, uuids.fake, {}) @mock.patch('nova.api.openstack.common.get_instance') diff --git a/nova/tests/unit/api/openstack/compute/test_suspend_server.py b/nova/tests/unit/api/openstack/compute/test_suspend_server.py index b4eee6004914..6eeb2b45493f 100644 --- a/nova/tests/unit/api/openstack/compute/test_suspend_server.py +++ b/nova/tests/unit/api/openstack/compute/test_suspend_server.py @@ -21,7 +21,6 @@ from nova.api.openstack.compute import suspend_server as \ suspend_server_v21 from nova import exception from nova.tests.unit.api.openstack.compute import admin_only_action_common -from nova.tests.unit.api.openstack import fakes @ddt.ddt @@ -46,12 +45,14 @@ class SuspendServerTestsV21(admin_only_action_common.CommonTests): instance_uuid=uuids.instance, operation='foo'), exception.OperationNotSupportedForSEV( instance_uuid=uuids.instance, operation='foo'), + exception.ForbiddenWithAccelerators(), ) @mock.patch('nova.compute.api.API.suspend') - def test_suspend__http_conflict_error(self, exc, mock_suspend): + def test_suspend_raise_badrequest_for_not_supported_features( + self, exc, mock_suspend): mock_suspend.side_effect = exc self.assertRaises( - webob.exc.HTTPConflict, + webob.exc.HTTPBadRequest, self.controller._suspend, self.req, uuids.instance, body={}) self.assertTrue(mock_suspend.called) @@ -65,10 +66,3 @@ class SuspendServerTestsV21(admin_only_action_common.CommonTests): def test_actions_with_locked_instance(self): self._test_actions_with_locked_instance(['_suspend', '_resume']) - - @mock.patch('nova.compute.api.API.suspend', - side_effect=exception.ForbiddenWithAccelerators) - def test_suspend_raises_http_forbidden(self, mock_suspend): - self.assertRaises(webob.exc.HTTPForbidden, - self.controller._suspend, - self.req, fakes.FAKE_UUID, body={}) diff --git a/nova/tests/unit/api/openstack/compute/test_volumes.py b/nova/tests/unit/api/openstack/compute/test_volumes.py index 284e5f2da56d..78a37934c451 100644 --- a/nova/tests/unit/api/openstack/compute/test_volumes.py +++ b/nova/tests/unit/api/openstack/compute/test_volumes.py @@ -977,7 +977,7 @@ class VolumeAttachTestsV260(test.NoDBTestCase): side_effect= exception.MultiattachNotSupportedByVirtDriver( volume_id=FAKE_UUID_A)) as attach: - ex = self.assertRaises(webob.exc.HTTPConflict, self._post_attach) + ex = self.assertRaises(webob.exc.HTTPBadRequest, self._post_attach) create_kwargs = attach.call_args[1] self.assertTrue(create_kwargs['supports_multiattach']) self.assertIn("has 'multiattach' set, which is not supported for " diff --git a/nova/tests/unit/compute/test_api.py b/nova/tests/unit/compute/test_api.py index 1ebb525ff4c5..d1f73a4c9dfb 100644 --- a/nova/tests/unit/compute/test_api.py +++ b/nova/tests/unit/compute/test_api.py @@ -7830,7 +7830,7 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase): args_info = {} self.assertRaisesRegex(exception.ForbiddenWithAccelerators, - 'Forbidden with instances that have accelerators.', + 'Feature not supported with instances that have accelerators.', self._test_block_accelerators, instance, args_info) # myfunc was not called self.assertEqual({}, args_info) @@ -7861,7 +7861,7 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase): instance = self._create_instance_obj(flavor=flavor) args_info = {} self.assertRaisesRegex(exception.ForbiddenWithAccelerators, - 'Forbidden with instances that have accelerators.', + 'Feature not supported with instances that have accelerators.', self._test_block_accelerators, instance, args_info, 53) # myfunc was not called self.assertEqual({}, args_info) @@ -7895,7 +7895,7 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase): instance.info_cache.network_info = nw_info args_info = {} self.assertRaisesRegex(exception.ForbiddenPortsWithAccelerator, - 'Forbidden with Ports that have accelerators.', + 'Feature not supported with Ports that have accelerators.', self._test_block_port_accelerators, instance, args_info) self.assertEqual({}, args_info) @@ -7906,7 +7906,7 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase): instance.info_cache.network_info = nw_info args_info = {} self.assertRaisesRegex(exception.ForbiddenPortsWithAccelerator, - 'Forbidden with Ports that have accelerators.', + 'Feature not supported with Ports that have accelerators.', self._test_block_port_accelerators, instance, args_info) self.assertEqual({}, args_info) @@ -8019,7 +8019,7 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase): flavor = self._create_flavor(extra_specs=extra_specs) instance = self._create_instance_obj(flavor=flavor) self.assertRaisesRegex(exception.ForbiddenWithAccelerators, - 'Forbidden with instances that have accelerators.', + 'Feature not supported with instances that have accelerators.', self.compute_api.shelve, self.context, instance) mock_get_arq_uuuids.assert_not_called() diff --git a/releasenotes/notes/convert-features-not-implemented-return-code-bf8beea51705271b.yaml b/releasenotes/notes/convert-features-not-implemented-return-code-bf8beea51705271b.yaml new file mode 100644 index 000000000000..2cb5eb08eb60 --- /dev/null +++ b/releasenotes/notes/convert-features-not-implemented-return-code-bf8beea51705271b.yaml @@ -0,0 +1,8 @@ +--- +upgrade: + - | + A few of the APIs return code was not consistent for the operations/ + features not implemented or supported. It was returned as 403, 400, or + 409 (for Operation Not Supported For SEV , Operation Not Supported For + VTPM cases). Now we have made it consistent and return 400 always when + any operations/features are not implemented or supported.