Skip to content

Commit

Permalink
api: Normalize exception handling for os-hypervisors
Browse files Browse the repository at this point in the history 
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 <[email protected]>
Related-Bug: #1646255
  • Loading branch information
@stephenfin
stephenfin committed Jan 7, 2021
1 parent 4689996 commit ef7598a
Show file tree
Hide file tree
Showing 2 changed files with 178 additions and 52 deletions.
126 changes: 95 additions & 31 deletions nova/api/openstack/compute/hypervisors.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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))

Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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):
Expand Down
104 changes: 83 additions & 21 deletions nova/tests/unit/api/openstack/compute/test_hypervisors.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 = [
Expand Down

0 comments on commit ef7598a

Please sign in to comment.