Handle ComputeHostNotFound when listing hypervisors

Compute node resources must currently be deleted manually
in the database, and as such they can reference service
records which have been deleted via the services delete API.
Because of this when listing hypervisors (compute nodes), we
may get a ComputeHostNotFound error when trying to lookup a
service record for a compute node where the service was
deleted. This causes the API to fail with a 500 since it's not
handled.

This change handles the ComputeHostNotFound when looping over
compute nodes in the hypervisors index and detail methods and
simply ignores them.

Change-Id: I2717274bb1bd370870acbf58c03dc59cee30cc5e
Closes-Bug: #1646255

diff --git a/nova/api/openstack/compute/hypervisors.py b/nova/api/openstack/compute/hypervisors.py
index f20c97da80100f6bb7118444f376aea30bf814db..ce28fe2d8f667cdc71332d2d9555c99fe69b2ad3 100644 (file)
--- a/nova/api/openstack/compute/hypervisors.py
+++ b/nova/api/openstack/compute/hypervisors.py
@@ -15,6 +15,7 @@
 
 """The hypervisors admin extension."""
 
+from oslo_log import log as logging
 from oslo_serialization import jsonutils
 import webob.exc
 
@@ -29,6 +30,7 @@ from nova.i18n import _
 from nova.policies import hypervisors as hv_policies
 from nova import servicegroup
 
+LOG = logging.getLogger(__name__)
 
 ALIAS = "os-hypervisors"
 
@@ -108,12 +110,21 @@ class HypervisorsController(wsgi.Controller):
             msg = _('marker [%s] not found') % marker
             raise webob.exc.HTTPBadRequest(explanation=msg)
         req.cache_db_compute_nodes(compute_nodes)
-        hypervisors_list = [self._view_hypervisor(
-                            hyp,
-                            self.host_api.service_get_by_compute_host(
-                                context, hyp.host),
-                            False, req)
-                            for hyp in compute_nodes]
+        hypervisors_list = []
+        for hyp in compute_nodes:
+            try:
+                service = self.host_api.service_get_by_compute_host(
+                    context, hyp.host)
+                hypervisors_list.append(
+                    self._view_hypervisor(
+                        hyp, service, False, req))
+            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.', hyp.host)
 
         hypervisors_dict = dict(hypervisors=hypervisors_list)
         if links:
@@ -145,10 +156,21 @@ class HypervisorsController(wsgi.Controller):
             msg = _('marker [%s] not found') % marker
             raise webob.exc.HTTPBadRequest(explanation=msg)
         req.cache_db_compute_nodes(compute_nodes)
-        hypervisors_list = [
-            self._view_hypervisor(
-            hyp, self.host_api.service_get_by_compute_host(context, hyp.host),
-            True, req) for hyp in compute_nodes]
+        hypervisors_list = []
+        for hyp in compute_nodes:
+            try:
+                service = self.host_api.service_get_by_compute_host(
+                    context, hyp.host)
+                hypervisors_list.append(
+                    self._view_hypervisor(
+                        hyp, service, True, req))
+            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.', hyp.host)
         hypervisors_dict = dict(hypervisors=hypervisors_list)
         if links:
             hypervisors_links = self._view_builder.get_links(

diff --git a/nova/tests/unit/api/openstack/compute/test_hypervisors.py b/nova/tests/unit/api/openstack/compute/test_hypervisors.py
index 743c8f35e5100260f7510bdc9eadbf885c85f9f5..56478efa4cdce93a3a91f17e62e94103ac3f4a7b 100644 (file)
--- a/nova/tests/unit/api/openstack/compute/test_hypervisors.py
+++ b/nova/tests/unit/api/openstack/compute/test_hypervisors.py
@@ -289,6 +289,41 @@ class HypervisorsTestV21(test.NoDBTestCase):
         self.assertRaises(exception.PolicyNotAuthorized,
                           self.controller.index, req)
 
+    def test_index_compute_host_not_found(self):
+        """Tests that if a service is deleted but the compute node is not we
+        don't fail when listing hypervisors.
+        """
+
+        # two computes, a matching service only exists for the first one
+        compute_nodes = objects.ComputeNodeList(objects=[
+            objects.ComputeNode(**TEST_HYPERS[0]),
+            objects.ComputeNode(**TEST_HYPERS[1])
+        ])
+
+        def fake_service_get_by_compute_host(context, host):
+            if host == TEST_HYPERS[0]['host']:
+                return TEST_SERVICES[0]
+            raise exception.ComputeHostNotFound(host=host)
+
+        @mock.patch.object(self.controller.host_api, 'compute_node_get_all',
+                           return_value=compute_nodes)
+        @mock.patch.object(self.controller.host_api,
+                           'service_get_by_compute_host',
+                           fake_service_get_by_compute_host)
+        def _test(self, compute_node_get_all):
+            req = self._get_request(True)
+            result = self.controller.index(req)
+            self.assertTrue(1, len(result['hypervisors']))
+            expected = {
+                'id': compute_nodes[0].id,
+                'hypervisor_hostname': compute_nodes[0].hypervisor_hostname,
+                'state': 'up',
+                'status': 'enabled',
+            }
+            self.assertDictEqual(expected, result['hypervisors'][0])
+
+        _test(self)
+
     def test_detail(self):
         req = self._get_request(True)
         result = self.controller.detail(req)
@@ -300,6 +335,49 @@ class HypervisorsTestV21(test.NoDBTestCase):
         self.assertRaises(exception.PolicyNotAuthorized,
                           self.controller.detail, req)
 
+    def test_detail_compute_host_not_found(self):
+        """Tests that if a service is deleted but the compute node is not we
+        don't fail when listing hypervisors.
+        """
+
+        # two computes, a matching service only exists for the first one
+        compute_nodes = objects.ComputeNodeList(objects=[
+            objects.ComputeNode(**TEST_HYPERS[0]),
+            objects.ComputeNode(**TEST_HYPERS[1])
+        ])
+
+        def fake_service_get_by_compute_host(context, host):
+            if host == TEST_HYPERS[0]['host']:
+                return TEST_SERVICES[0]
+            raise exception.ComputeHostNotFound(host=host)
+
+        @mock.patch.object(self.controller.host_api, 'compute_node_get_all',
+                           return_value=compute_nodes)
+        @mock.patch.object(self.controller.host_api,
+                           'service_get_by_compute_host',
+                           fake_service_get_by_compute_host)
+        def _test(self, compute_node_get_all):
+            req = self._get_request(True)
+            result = self.controller.detail(req)
+            self.assertTrue(1, len(result['hypervisors']))
+            expected = {
+                'id': compute_nodes[0].id,
+                'hypervisor_hostname': compute_nodes[0].hypervisor_hostname,
+                'state': 'up',
+                'status': 'enabled',
+            }
+            # we don't care about all of the details, just make sure we get
+            # the subset we care about and there are more keys than what index
+            # would return
+            hypervisor = result['hypervisors'][0]
+            self.assertTrue(
+                set(expected.keys()).issubset(set(hypervisor.keys())))
+            self.assertGreater(len(hypervisor.keys()), len(expected.keys()))
+            self.assertEqual(compute_nodes[0].hypervisor_hostname,
+                             hypervisor['hypervisor_hostname'])
+
+        _test(self)
+
     def test_show_noid(self):
         req = self._get_request(True)
         self.assertRaises(exc.HTTPNotFound, self.controller.show, req, '3')