From 1d4dbfd4680d32d0619e84dbe563deed892e0506 Mon Sep 17 00:00:00 2001 From: melanie witt Date: Tue, 3 May 2022 01:48:36 +0000 Subject: [PATCH] Log the exception returned from a cell during API.get() When getting an instance using the compute.API we call scatter_gather_single_cell() to be able to capture details when we fail to retrieve a result from a cell such as timeouts and exceptions. Currently however, we aren't logging the content of an exception if scatter_gather_single_cell() returns an exception as the result. The scatter gather method itself logs exceptions that are not of type NovaException as these represent definite unexpected errors such as database errors but NovaException handling are left for the caller to decide whether they want to log it or re-raise it and so on. It can be difficult to debug a situation where a cell is returning a NovaException result so this adds logging of the exception content in the compute API when we encounter an unexpected NovaException. The existing log message has been updated to more accurately reflect what has happened (did not respond vs exception). The assignment of the exception object in scatter gather has also been updated to not unnecessarily construct a new exception object because it (a) wasn't necessary and (b) made asserting the LOG.exception() call argument in the unit test difficult. Related-Bug: #1970087 Change-Id: Iae1c61c72be5b6017b934293e3dc079a24eeb0e7 --- nova/compute/api.py | 6 ++++-- nova/context.py | 2 +- nova/tests/unit/compute/test_api.py | 12 +++++++++++- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/nova/compute/api.py b/nova/compute/api.py index 43a0f66a100e..8170fd8f24f8 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -2857,9 +2857,11 @@ class API: # spec has been archived is being queried. raise exception.InstanceNotFound(instance_id=uuid) else: + if isinstance(result[cell_uuid], exception.NovaException): + LOG.exception(result[cell_uuid]) raise exception.NovaException( - _("Cell %s is not responding and hence instance " - "info is not available.") % cell_uuid) + _("Cell %s is not responding or returned an exception, " + "hence instance info is not available.") % cell_uuid) def _get_instance(self, context, instance_uuid, expected_attrs, cell_down_support=False): diff --git a/nova/context.py b/nova/context.py index 619c39e212ae..dc42b38b5bd2 100644 --- a/nova/context.py +++ b/nova/context.py @@ -424,7 +424,7 @@ def scatter_gather_cells(context, cell_mappings, timeout, fn, *args, **kwargs): # Only log the exception traceback for non-nova exceptions. if not isinstance(e, exception.NovaException): LOG.exception('Error gathering result from cell %s', cell_uuid) - result = e.__class__(e.args) + result = e # The queue is already synchronized. queue.put((cell_uuid, result)) diff --git a/nova/tests/unit/compute/test_api.py b/nova/tests/unit/compute/test_api.py index 4297fcff667c..5a3f5d01b41d 100644 --- a/nova/tests/unit/compute/test_api.py +++ b/nova/tests/unit/compute/test_api.py @@ -6371,8 +6371,9 @@ class _ComputeAPIUnitTestMixIn(object): self.assertEqual(uuids.inst, result.uuid) mock_get_inst.assert_called_once() + @mock.patch('nova.compute.api.LOG.exception') @mock.patch.object(objects.Instance, 'get_by_uuid') - def test_get_instance_from_cell_failure(self, mock_get_inst): + def test_get_instance_from_cell_failure(self, mock_get_inst, mock_log_exp): # Make sure InstanceNotFound is bubbled up and not treated like # other errors mock_get_inst.side_effect = exception.InstanceNotFound( @@ -6385,6 +6386,15 @@ class _ComputeAPIUnitTestMixIn(object): self.compute_api._get_instance_from_cell, self.context, im, [], False) self.assertIn('could not be found', str(exp)) + # Make sure other unexpected NovaException are logged for debugging + mock_get_inst.side_effect = exception.NovaException() + exp = self.assertRaises( + exception.NovaException, self.compute_api._get_instance_from_cell, + self.context, im, [], False) + msg = (f'Cell {cell_mapping.uuid} is not responding or returned an ' + 'exception, hence instance info is not available.') + self.assertIn(msg, str(exp)) + mock_log_exp.assert_called_once_with(mock_get_inst.side_effect) @mock.patch('nova.compute.api.API._save_user_id_in_instance_mapping') @mock.patch.object(objects.RequestSpec, 'get_by_instance_uuid')