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 <stephenfin@redhat.com>
Related-Bug: #1646255

nova/api/openstack/compute/hypervisors.py

@@ -172,18 +172,22 @@ class HypervisorsController(wsgi.Controller):
                         context, hyp.host)
                 service = self.host_api.service_get_by_compute_host(
                     context, hyp.host)
-                hypervisors_list.append(
+            except (
-                    self._view_hypervisor(
+                exception.ComputeHostNotFound,
-                        hyp, service, detail, req, servers=instances,
+                exception.HostMappingNotFound,
-                        with_servers=with_servers))
+            ):
-            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 @@ class HypervisorsController(wsgi.Controller):
 
         try:
             hyp = self.host_api.compute_node_get(context, id)
-            instances = None
+        except exception.ComputeHostNotFound:
-            if with_servers:
+            # If the ComputeNode is missing, that's a straight up 404
-                instances = self.host_api.instance_get_all_by_host(
-                    context, hyp.host)
-            service = self.host_api.service_get_by_compute_host(
-                context, hyp.host)
-        except (ValueError, exception.ComputeHostNotFound,
-                exception.HostMappingNotFound):
             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 (
+            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 @@ class HypervisorsController(wsgi.Controller):
 
         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, hyp.host)
-            uptime = self.host_api.get_host_uptime(context, host)
-            service = self.host_api.service_get_by_compute_host(context, 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 @@ class HypervisorsController(wsgi.Controller):
         """
         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:
+        # Get all compute nodes with a hypervisor_hostname that matches
-            return dict(hypervisors=[
+        # the given pattern. If none are found then it's a 404 error.
-                self._view_hypervisor(
+        compute_nodes = self._get_compute_nodes_by_name_pattern(context, id)
-                    hyp,
+
-                    self.host_api.service_get_by_compute_host(context,
+        hypervisors = []
-                                                              hyp.host),
+        for compute_node in compute_nodes:
-                    False, req)
+            try:
-                for hyp in hypervisors])
+                service = self.host_api.service_get_by_compute_host(
-        except exception.HostMappingNotFound:
+                    context, compute_node.host)
-            msg = _("No hypervisor matching '%s' could be found.") % id
+            except exception.ComputeHostNotFound:
-            raise webob.exc.HTTPNotFound(explanation=msg)
+                # 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 @@ class HypervisorsController(wsgi.Controller):
         """
         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)
-                service = self.host_api.service_get_by_compute_host(
-                    context, compute_node.host)
             except exception.HostMappingNotFound as e:
                 raise webob.exc.HTTPNotFound(explanation=e.format_message())
-            hyp = self._view_hypervisor(compute_node, service, False, req,
+
-                                        instances)
+            try:
-            hypervisors.append(hyp)
+                service = self.host_api.service_get_by_compute_host(
-        return dict(hypervisors=hypervisors)
+                    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, instances)
+            hypervisors.append(hypervisor)
+
+        return {'hypervisors': hypervisors}
 
     @wsgi.expected_errors(())
     def statistics(self, req):

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 @@ class HypervisorsTestV21(test.NoDBTestCase):
 
         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):
+    def test_uptime_not_implemented(self):
-        with mock.patch.object(self.controller.host_api, 'get_host_uptime',
+        with mock.patch.object(
-                               side_effect=exc.HTTPNotImplemented()
+            self.controller.host_api, 'get_host_uptime',
-                               ) as mock_get_uptime:
+            side_effect=NotImplementedError,
+        ) as mock_get_uptime:
             req = self._get_request(True)
             hyper_id = self._get_hyper_id()
-            self.assertRaises(exc.HTTPNotImplemented,
+            self.assertRaises(
-                              self.controller.uptime, req, hyper_id)
+                exc.HTTPNotImplemented,
+                self.controller.uptime, req, hyper_id)
             self.assertEqual(1, mock_get_uptime.call_count)
 
-    def test_uptime_implemented(self):
+    def test_uptime_host_not_found(self):
-        with mock.patch.object(self.controller.host_api, 'get_host_uptime',
+        with mock.patch.object(
-                               return_value="fake uptime"
+            self.controller.host_api, 'get_host_uptime',
-                               ) as mock_get_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)
+            self.assertRaises(
-
+                exc.HTTPBadRequest,
-            expected_dict = copy.deepcopy(self.INDEX_HYPER_DICTS[0])
+                self.controller.uptime, req, hyper_id)
-            expected_dict.update({'uptime': "fake uptime"})
-            self.assertEqual(dict(hypervisor=expected_dict), result)
             self.assertEqual(1, mock_get_uptime.call_count)
 
     def test_uptime_non_integer_id(self):
@@ -668,11 +690,31 @@ class HypervisorsTestV21(test.NoDBTestCase):
 
     def test_servers_not_mapped(self):
         req = self._get_request(True)
-        with mock.patch.object(self.controller.host_api,
+        with mock.patch.object(
-                               'instance_get_all_by_host') as m:
+            self.controller.host_api, 'instance_get_all_by_host',
-            m.side_effect = exception.HostMappingNotFound(name='something')
+            side_effect=exception.HostMappingNotFound(name='something'),
-            self.assertRaises(exc.HTTPNotFound,
+        ):
-                              self.controller.servers, req, 'hyper')
+            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 @@ class HypervisorsTestV253(HypervisorsTestV252):
             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 = [