From ef7598ac2896d08a89e50ccb82a47244e63d6248 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Tue, 5 Jan 2021 17:28:15 +0000 Subject: [PATCH] api: Normalize exception handling for os-hypervisors Many of the functions implementing the various 'os-hypervisors' endpoints share common code. In particular any of these functions contain calls to both the 'instance_get_all_by_host' and 'service_get_by_compute_host' APIs of 'nova.compute.api.HostAPI' so we can include instance and service information in the responses. All of these calls are guarded with exception handlers, but the exceptions handled differ between resources. There is one exception we need to care about for 'instance_get_all_by_host': 'HostMappingNotFound', which is raised because the API is decorated with the 'target_cell' decorator. The 'service_get_by_compute_host' API is also decorated by the 'target_host_cell' decorator, however, it can also raise 'ComputeHostNotFound'. This exception is possible because the 'service_get_by_compute_host' API calls 'nova.objects.Service.get_by_compute_host', which in turns calls 'nova.db.sqlalchemy.api.service_get_by_compute_host', via the '_db_service_get_by_compute_host' helper. Not all of the functions that called 'service_get_by_compute_host' were correctly guarding against 'ComputeHostNotFound'. In addition to this, the call to the 'get_host_uptime' API used by the '/os-hypervisors/uptime' API can raise 'HostNotFound' if the service has been deleted but the compute node still has to be manually cleaned up. Conversely, a number of functions were handling 'ValueError' even though this couldn't realistically be raised by the test. Resolve all of the above. Change-Id: Iacabaea31311ae14084b55341608e16e531e6bd5 Signed-off-by: Stephen Finucane Related-Bug: #1646255 --- nova/api/openstack/compute/hypervisors.py | 126 +++++++++++++----- .../api/openstack/compute/test_hypervisors.py | 104 ++++++++++++--- 2 files changed, 178 insertions(+), 52 deletions(-) diff --git a/nova/api/openstack/compute/hypervisors.py b/nova/api/openstack/compute/hypervisors.py index 107996a18e8..8232fd9f4f1 100644 --- a/nova/api/openstack/compute/hypervisors.py +++ b/nova/api/openstack/compute/hypervisors.py @@ -172,18 +172,22 @@ def _get_hypervisors(self, req, detail=False, limit=None, marker=None, context, hyp.host) service = self.host_api.service_get_by_compute_host( context, hyp.host) - hypervisors_list.append( - self._view_hypervisor( - hyp, service, detail, req, servers=instances, - with_servers=with_servers)) - except (exception.ComputeHostNotFound, - exception.HostMappingNotFound): + except ( + exception.ComputeHostNotFound, + exception.HostMappingNotFound, + ): # The compute service could be deleted which doesn't delete # the compute node record, that has to be manually removed # from the database so we just ignore it when listing nodes. LOG.debug('Unable to find service for compute node %s. The ' 'service may be deleted and compute nodes need to ' 'be manually cleaned up.', hyp.host) + continue + + hypervisors_list.append( + self._view_hypervisor( + hyp, service, detail, req, servers=instances, + with_servers=with_servers)) hypervisors_dict = dict(hypervisors=hypervisors_list) if links: @@ -311,16 +315,30 @@ def _show(self, req, id, with_servers=False): try: hyp = self.host_api.compute_node_get(context, id) - instances = None - if with_servers: + except exception.ComputeHostNotFound: + # If the ComputeNode is missing, that's a straight up 404 + msg = _("Hypervisor with ID '%s' could not be found.") % id + raise webob.exc.HTTPNotFound(explanation=msg) + + instances = None + if with_servers: + try: instances = self.host_api.instance_get_all_by_host( context, hyp.host) + except exception.HostMappingNotFound: + msg = _("Hypervisor with ID '%s' could not be found.") % id + raise webob.exc.HTTPNotFound(explanation=msg) + + try: service = self.host_api.service_get_by_compute_host( context, hyp.host) - except (ValueError, exception.ComputeHostNotFound, - exception.HostMappingNotFound): + except ( + exception.ComputeHostNotFound, + exception.HostMappingNotFound, + ): msg = _("Hypervisor with ID '%s' could not be found.") % id raise webob.exc.HTTPNotFound(explanation=msg) + return dict(hypervisor=self._view_hypervisor( hyp, service, True, req, instances, with_servers)) @@ -333,18 +351,30 @@ def uptime(self, req, id): try: hyp = self.host_api.compute_node_get(context, id) - except (ValueError, exception.ComputeHostNotFound): + except exception.ComputeHostNotFound: + # If the ComputeNode is missing, that's a straight up 404 + msg = _("Hypervisor with ID '%s' could not be found.") % id + raise webob.exc.HTTPNotFound(explanation=msg) + + try: + service = self.host_api.service_get_by_compute_host( + context, hyp.host) + except ( + exception.ComputeHostNotFound, + exception.HostMappingNotFound, + ): msg = _("Hypervisor with ID '%s' could not be found.") % id raise webob.exc.HTTPNotFound(explanation=msg) # Get the uptime try: - host = hyp.host - uptime = self.host_api.get_host_uptime(context, host) - service = self.host_api.service_get_by_compute_host(context, host) + uptime = self.host_api.get_host_uptime(context, hyp.host) except NotImplementedError: common.raise_feature_not_supported() - except exception.ComputeServiceUnavailable as e: + except ( + exception.ComputeServiceUnavailable, + exception.HostNotFound, + ) as e: raise webob.exc.HTTPBadRequest(explanation=e.format_message()) except exception.HostMappingNotFound: # NOTE(danms): This mirrors the compute_node_get() behavior @@ -366,18 +396,33 @@ def search(self, req, id): """ context = req.environ['nova.context'] context.can(hv_policies.BASE_POLICY_NAME % 'search', target={}) - hypervisors = self._get_compute_nodes_by_name_pattern(context, id) - try: - return dict(hypervisors=[ - self._view_hypervisor( - hyp, - self.host_api.service_get_by_compute_host(context, - hyp.host), - False, req) - for hyp in hypervisors]) - except exception.HostMappingNotFound: - msg = _("No hypervisor matching '%s' could be found.") % id - raise webob.exc.HTTPNotFound(explanation=msg) + + # Get all compute nodes with a hypervisor_hostname that matches + # the given pattern. If none are found then it's a 404 error. + compute_nodes = self._get_compute_nodes_by_name_pattern(context, id) + + hypervisors = [] + for compute_node in compute_nodes: + try: + service = self.host_api.service_get_by_compute_host( + context, compute_node.host) + except exception.ComputeHostNotFound: + # The compute service could be deleted which doesn't delete + # the compute node record, that has to be manually removed + # from the database so we just ignore it when listing nodes. + LOG.debug( + 'Unable to find service for compute node %s. The ' + 'service may be deleted and compute nodes need to ' + 'be manually cleaned up.', compute_node.host) + continue + except exception.HostMappingNotFound as e: + raise webob.exc.HTTPNotFound(explanation=e.format_message()) + + hypervisor = self._view_hypervisor( + compute_node, service, False, req) + hypervisors.append(hypervisor) + + return {'hypervisors': hypervisors} @wsgi.Controller.api_version('2.1', '2.52') @wsgi.expected_errors(404) @@ -390,20 +435,39 @@ def servers(self, req, id): """ context = req.environ['nova.context'] context.can(hv_policies.BASE_POLICY_NAME % 'servers', target={}) + + # Get all compute nodes with a hypervisor_hostname that matches + # the given pattern. If none are found then it's a 404 error. compute_nodes = self._get_compute_nodes_by_name_pattern(context, id) + hypervisors = [] for compute_node in compute_nodes: try: instances = self.host_api.instance_get_all_by_host(context, compute_node.host) + except exception.HostMappingNotFound as e: + raise webob.exc.HTTPNotFound(explanation=e.format_message()) + + try: service = self.host_api.service_get_by_compute_host( context, compute_node.host) + except exception.ComputeHostNotFound: + # The compute service could be deleted which doesn't delete + # the compute node record, that has to be manually removed + # from the database so we just ignore it when listing nodes. + LOG.debug( + 'Unable to find service for compute node %s. The ' + 'service may be deleted and compute nodes need to ' + 'be manually cleaned up.', compute_node.host) + continue except exception.HostMappingNotFound as e: raise webob.exc.HTTPNotFound(explanation=e.format_message()) - hyp = self._view_hypervisor(compute_node, service, False, req, - instances) - hypervisors.append(hyp) - return dict(hypervisors=hypervisors) + + hypervisor = self._view_hypervisor( + compute_node, service, False, req, instances) + hypervisors.append(hypervisor) + + return {'hypervisors': hypervisors} @wsgi.expected_errors(()) def statistics(self, req): diff --git a/nova/tests/unit/api/openstack/compute/test_hypervisors.py b/nova/tests/unit/api/openstack/compute/test_hypervisors.py index 16ab33f523b..f46bca98b40 100644 --- a/nova/tests/unit/api/openstack/compute/test_hypervisors.py +++ b/nova/tests/unit/api/openstack/compute/test_hypervisors.py @@ -133,7 +133,13 @@ def fake_compute_node_search_by_hypervisor(context, hypervisor_re): def fake_compute_node_get(context, compute_id): for hyper in TEST_HYPERS_OBJ: - if hyper.uuid == compute_id or hyper.id == int(compute_id): + if hyper.uuid == compute_id: + return hyper + + if ( + (isinstance(compute_id, int) or compute_id.isdigit()) and + hyper.id == int(compute_id) + ): return hyper raise exception.ComputeHostNotFound(host=compute_id) @@ -543,33 +549,49 @@ def test_show_withid(self): self.assertEqual(dict(hypervisor=self.DETAIL_HYPERS_DICTS[0]), result) + def test_uptime(self): + with mock.patch.object( + self.controller.host_api, 'get_host_uptime', + return_value="fake uptime" + ) as mock_get_uptime: + req = self._get_request(True) + hyper_id = self._get_hyper_id() + + result = self.controller.uptime(req, hyper_id) + + expected_dict = copy.deepcopy(self.INDEX_HYPER_DICTS[0]) + expected_dict.update({'uptime': "fake uptime"}) + self.assertEqual(dict(hypervisor=expected_dict), result) + self.assertEqual(1, mock_get_uptime.call_count) + def test_uptime_noid(self): req = self._get_request(True) hyper_id = uuids.hyper3 if self.expect_uuid_for_id else '3' self.assertRaises(exc.HTTPNotFound, self.controller.uptime, req, hyper_id) - def test_uptime_notimplemented(self): - with mock.patch.object(self.controller.host_api, 'get_host_uptime', - side_effect=exc.HTTPNotImplemented() - ) as mock_get_uptime: + def test_uptime_not_implemented(self): + with mock.patch.object( + self.controller.host_api, 'get_host_uptime', + side_effect=NotImplementedError, + ) as mock_get_uptime: req = self._get_request(True) hyper_id = self._get_hyper_id() - self.assertRaises(exc.HTTPNotImplemented, - self.controller.uptime, req, hyper_id) + self.assertRaises( + exc.HTTPNotImplemented, + self.controller.uptime, req, hyper_id) self.assertEqual(1, mock_get_uptime.call_count) - def test_uptime_implemented(self): - with mock.patch.object(self.controller.host_api, 'get_host_uptime', - return_value="fake uptime" - ) as mock_get_uptime: + def test_uptime_host_not_found(self): + with mock.patch.object( + self.controller.host_api, 'get_host_uptime', + side_effect=exception.HostNotFound('foo'), + ) as mock_get_uptime: req = self._get_request(True) hyper_id = self._get_hyper_id() - result = self.controller.uptime(req, hyper_id) - - expected_dict = copy.deepcopy(self.INDEX_HYPER_DICTS[0]) - expected_dict.update({'uptime': "fake uptime"}) - self.assertEqual(dict(hypervisor=expected_dict), result) + self.assertRaises( + exc.HTTPBadRequest, + self.controller.uptime, req, hyper_id) self.assertEqual(1, mock_get_uptime.call_count) def test_uptime_non_integer_id(self): @@ -668,11 +690,31 @@ def test_servers(self, mock_get): def test_servers_not_mapped(self): req = self._get_request(True) - with mock.patch.object(self.controller.host_api, - 'instance_get_all_by_host') as m: - m.side_effect = exception.HostMappingNotFound(name='something') - self.assertRaises(exc.HTTPNotFound, - self.controller.servers, req, 'hyper') + with mock.patch.object( + self.controller.host_api, 'instance_get_all_by_host', + side_effect=exception.HostMappingNotFound(name='something'), + ): + self.assertRaises( + exc.HTTPNotFound, + self.controller.servers, req, 'hyper') + + def test_servers_compute_host_not_found(self): + req = self._get_request(True) + + with test.nested( + mock.patch.object( + self.controller.host_api, 'instance_get_all_by_host', + side_effect=fake_instance_get_all_by_host, + ), + mock.patch.object( + self.controller.host_api, 'service_get_by_compute_host', + side_effect=exception.ComputeHostNotFound(host='foo'), + ), + ): + # The result should be empty since every attempt to fetch the + # service for a hypervisor "failed" + result = self.controller.servers(req, 'hyper') + self.assertEqual({'hypervisors': []}, result) def test_servers_non_id(self): with mock.patch.object(self.controller.host_api, @@ -1021,6 +1063,26 @@ def test_servers_not_mapped(self): result = self.controller.index(req) self.assertEqual(dict(hypervisors=[]), result) + def test_servers_compute_host_not_found(self): + req = self._get_request( + use_admin_context=True, + url='/os-hypervisors?with_servers=1') + + with test.nested( + mock.patch.object( + self.controller.host_api, 'instance_get_all_by_host', + side_effect=fake_instance_get_all_by_host, + ), + mock.patch.object( + self.controller.host_api, 'service_get_by_compute_host', + side_effect=exception.ComputeHostNotFound(host='foo'), + ), + ): + # The result should be empty since every attempt to fetch the + # service for a hypervisor "failed" + result = self.controller.index(req) + self.assertEqual({'hypervisors': []}, result) + def test_list_with_servers(self): """Tests GET /os-hypervisors?with_servers=True""" instances = [