From fb661ec597e326b70829246cfdd4309dc7273bfa Mon Sep 17 00:00:00 2001 From: Ivaylo Mitev Date: Mon, 16 Dec 2019 02:32:12 -0800 Subject: [PATCH] Faults from cell DB missing in GET /servers/detail Field is empty in the response of API GET /servers/detail if the instance (hence instace_faults DB entry) is in nova cell DB. Unlike that, for API /servers/:id fault is retrieved correctly no matter in which nova cell the instance belongs. Closes-Bug: #1856329 Change-Id: I1726f53cfeac0a67a5dacdddda2af2cc1db0af0f Signed-off-by: Marius Leustean --- nova/objects/instance.py | 18 +- nova/tests/unit/objects/test_instance.py | 170 +++++++++++++++--- .../notes/bug-1856329-32d7f65bf08257b3.yaml | 11 ++ 3 files changed, 174 insertions(+), 25 deletions(-) create mode 100644 releasenotes/notes/bug-1856329-32d7f65bf08257b3.yaml diff --git a/nova/objects/instance.py b/nova/objects/instance.py index b387b8e0fe15..179fb2ceb737 100644 --- a/nova/objects/instance.py +++ b/nova/objects/instance.py @@ -1570,11 +1570,21 @@ class InstanceList(base.ObjectListBase, base.NovaObject): :returns: A list of instance uuids for which faults were found. """ uuids = [inst.uuid for inst in self] - faults = objects.InstanceFaultList.get_latest_by_instance_uuids( - self._context, uuids) + results = nova_context.scatter_gather_all_cells( + self._context, + objects.InstanceFaultList.get_latest_by_instance_uuids, + uuids) + faults_by_uuid = {} - for fault in faults: - faults_by_uuid[fault.instance_uuid] = fault + for cell_uuid, faults in results.items(): + if faults is nova_context.did_not_respond_sentinel: + LOG.warning('Cell %s did not respond when getting faults', + cell_uuid) + elif isinstance(faults, Exception): + LOG.warning('Failed to get faults for cell %s', cell_uuid) + else: + for fault in faults: + faults_by_uuid[fault.instance_uuid] = fault for instance in self: if instance.uuid in faults_by_uuid: diff --git a/nova/tests/unit/objects/test_instance.py b/nova/tests/unit/objects/test_instance.py index b97964c222d6..c41a15808949 100644 --- a/nova/tests/unit/objects/test_instance.py +++ b/nova/tests/unit/objects/test_instance.py @@ -1937,42 +1937,170 @@ class _TestInstanceListObject(object): mock_fault_get.assert_called_once_with(self.context, [x['uuid'] for x in fake_insts]) - @mock.patch.object(db, 'instance_fault_get_by_instance_uuids') - def test_fill_faults(self, mock_fault_get): + @mock.patch('nova.context.scatter_gather_all_cells') + def test_fill_faults(self, mock_sg): inst1 = objects.Instance(uuid=uuids.db_fault_1) inst2 = objects.Instance(uuid=uuids.db_fault_2) insts = [inst1, inst2] for inst in insts: inst.obj_reset_changes() - db_faults = { - 'uuid1': [{'id': 123, - 'instance_uuid': uuids.db_fault_1, - 'code': 456, - 'message': 'Fake message', - 'details': 'No details', - 'host': 'foo', - 'deleted': False, - 'deleted_at': None, - 'updated_at': None, - 'created_at': None, - } - ]} - mock_fault_get.return_value = db_faults + + # Create fake faults + fault = objects.InstanceFault( + id=123, + instance_uuid=uuids.db_fault_1, + code=456, + message='Fake message', + details='No details', + host='foo' + ) + fault.obj_reset_changes() + + # scatter_gather_all_cells returns a dict of {cell_uuid: result} + mock_sg.return_value = { + uuids.cell1: objects.InstanceFaultList(objects=[fault]) + } inst_list = objects.InstanceList() inst_list._context = self.context inst_list.objects = insts faulty = inst_list.fill_faults() self.assertEqual([uuids.db_fault_1], list(faulty)) - self.assertEqual(db_faults['uuid1'][0]['message'], - inst_list[0].fault.message) + self.assertEqual('Fake message', inst_list[0].fault.message) self.assertIsNone(inst_list[1].fault) for inst in inst_list: self.assertEqual(set(), inst.obj_what_changed()) - mock_fault_get.assert_called_once_with(self.context, - [x.uuid for x in insts], - latest=True) + mock_sg.assert_called_once_with( + self.context, + objects.InstanceFaultList.get_latest_by_instance_uuids, + [x.uuid for x in insts]) + + @mock.patch('nova.context.scatter_gather_all_cells') + def test_fill_faults_multicell(self, mock_sg): + # Prepare list with 2 instances + inst_list = objects.InstanceList() + inst_list._context = self.context + inst_list.objects = [ + objects.Instance(uuid=uuids.inst_1), + objects.Instance(uuid=uuids.inst_2) + ] + for inst in inst_list.objects: + inst.obj_reset_changes() + + # Create faults for instances in different cells + fault1 = objects.InstanceFault( + id=123, + instance_uuid=uuids.inst_1, + code=456, + message='Fake message %s' % uuids.inst_1, + details='No details', + host='foo' + ) + fault2 = objects.InstanceFault( + id=124, + instance_uuid=uuids.inst_2, + code=456, + message='Fake message %s' % uuids.inst_2, + details='No details', + host='foo' + ) + + # scatter_gather_all_cells returns a dict of {cell_uuid: result} + # Simulate faults from different cells + mock_sg.return_value = { + uuids.cell1: objects.InstanceFaultList(objects=[fault1]), + uuids.cell2: objects.InstanceFaultList(objects=[fault2]) + } + + inst_list.fill_faults() + # Verify multicell fill_faults by comparing combined list of faults + # from different cell DBs to the list of instance faults as filled + self.assertEqual(['Fake message %s' % uuids.inst_1, + 'Fake message %s' % uuids.inst_2], + [getattr(inst.fault, 'message', None) + for inst in inst_list]) + + mock_sg.assert_called_once_with( + self.context, + objects.InstanceFaultList.get_latest_by_instance_uuids, + [uuids.inst_1, uuids.inst_2]) + + @mock.patch('nova.context.scatter_gather_all_cells') + def test_fill_faults_cell_no_response(self, mock_sg): + """Test fill_faults when a cell doesn't respond.""" + inst1 = objects.Instance(uuid=uuids.db_fault_1) + inst2 = objects.Instance(uuid=uuids.db_fault_2) + insts = [inst1, inst2] + for inst in insts: + inst.obj_reset_changes() + + # Create a fault for one cell, another cell doesn't respond + fault = objects.InstanceFault( + id=123, + instance_uuid=uuids.db_fault_1, + code=456, + message='Fake message', + details='No details', + host='foo' + ) + fault.obj_reset_changes() + + # One cell returns a fault, another doesn't respond + mock_sg.return_value = { + uuids.cell1: objects.InstanceFaultList(objects=[fault]), + uuids.cell2: context.did_not_respond_sentinel + } + + inst_list = objects.InstanceList() + inst_list._context = self.context + inst_list.objects = insts + faulty = inst_list.fill_faults() + + # Should still get the fault from the responsive cell + self.assertEqual([uuids.db_fault_1], list(faulty)) + self.assertEqual('Fake message', inst_list[0].fault.message) + self.assertIsNone(inst_list[1].fault) + for inst in inst_list: + self.assertEqual(set(), inst.obj_what_changed()) + + @mock.patch('nova.context.scatter_gather_all_cells') + def test_fill_faults_cell_exception(self, mock_sg): + """Test fill_faults when getting faults raises an exception.""" + inst1 = objects.Instance(uuid=uuids.db_fault_1) + inst2 = objects.Instance(uuid=uuids.db_fault_2) + insts = [inst1, inst2] + for inst in insts: + inst.obj_reset_changes() + + # Create a fault for one cell, another cell raises exception + fault = objects.InstanceFault( + id=123, + instance_uuid=uuids.db_fault_1, + code=456, + message='Fake message', + details='No details', + host='foo' + ) + fault.obj_reset_changes() + + # One cell returns a fault, another raises an exception + mock_sg.return_value = { + uuids.cell1: objects.InstanceFaultList(objects=[fault]), + uuids.cell2: Exception('Database connection failed') + } + + inst_list = objects.InstanceList() + inst_list._context = self.context + inst_list.objects = insts + faulty = inst_list.fill_faults() + + # Should still get the fault from the successful cell + self.assertEqual([uuids.db_fault_1], list(faulty)) + self.assertEqual('Fake message', inst_list[0].fault.message) + self.assertIsNone(inst_list[1].fault) + for inst in inst_list: + self.assertEqual(set(), inst.obj_what_changed()) @mock.patch('nova.db.main.api.instance_get_all_uuids_by_hosts') def test_get_uuids_by_host_no_match(self, mock_get_all): diff --git a/releasenotes/notes/bug-1856329-32d7f65bf08257b3.yaml b/releasenotes/notes/bug-1856329-32d7f65bf08257b3.yaml new file mode 100644 index 000000000000..4e4a4609308d --- /dev/null +++ b/releasenotes/notes/bug-1856329-32d7f65bf08257b3.yaml @@ -0,0 +1,11 @@ +--- +fixes: + - | + The `bug 1856329`_ is fixed where the ``fault`` field was empty in the + response of ``GET /servers/detail`` when instances were located in a nova + cell DB. The ``InstanceList.fill_faults()`` method has been updated to use + ``scatter_gather_all_cells()`` to properly retrieve faults from all cells, + ensuring fault information is consistently returned regardless of which + cell contains the instance. + + .. _bug 1856329: https://bugs.launchpad.net/nova/+bug/1856329