diff --git a/nova/db/api.py b/nova/db/api.py index 1102268210..73e6581e54 100644 --- a/nova/db/api.py +++ b/nova/db/api.py @@ -1901,9 +1901,11 @@ def instance_fault_create(context, values): return IMPL.instance_fault_create(context, values) -def instance_fault_get_by_instance_uuids(context, instance_uuids): +def instance_fault_get_by_instance_uuids(context, instance_uuids, + latest=False): """Get all instance faults for the provided instance_uuids.""" - return IMPL.instance_fault_get_by_instance_uuids(context, instance_uuids) + return IMPL.instance_fault_get_by_instance_uuids(context, instance_uuids, + latest=latest) #################### diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index 2b61952e58..1d8b338c23 100644 --- a/nova/db/sqlalchemy/api.py +++ b/nova/db/sqlalchemy/api.py @@ -6036,24 +6036,63 @@ def instance_fault_create(context, values): @pick_context_manager_reader -def instance_fault_get_by_instance_uuids(context, instance_uuids): - """Get all instance faults for the provided instance_uuids.""" +def instance_fault_get_by_instance_uuids(context, instance_uuids, + latest=False): + """Get all instance faults for the provided instance_uuids. + + :param instance_uuids: List of UUIDs of instances to grab faults for + :param latest: Optional boolean indicating we should only return the latest + fault for the instance + """ if not instance_uuids: return {} - rows = model_query(context, models.InstanceFault, read_deleted='no').\ - filter(models.InstanceFault.instance_uuid.in_( - instance_uuids)).\ - order_by(desc("created_at"), desc("id")).\ - all() + faults_tbl = models.InstanceFault.__table__ + # NOTE(rpodolyaka): filtering by instance_uuids is performed in both + # code branches below for the sake of a better query plan. On change, + # make sure to update the other one as well. + query = model_query(context, models.InstanceFault, + [faults_tbl], + read_deleted='no') + + if latest: + # NOTE(jaypipes): We join instance_faults to a derived table of the + # latest faults per instance UUID. The SQL produced below looks like + # this: + # + # SELECT instance_faults.* + # FROM instance_faults + # JOIN ( + # SELECT instance_uuid, MAX(id) AS max_id + # FROM instance_faults + # WHERE instance_uuid IN ( ... ) + # AND deleted = 0 + # GROUP BY instance_uuid + # ) AS latest_faults + # ON instance_faults.id = latest_faults.max_id; + latest_faults = model_query( + context, models.InstanceFault, + [faults_tbl.c.instance_uuid, + sql.func.max(faults_tbl.c.id).label('max_id')], + read_deleted='no' + ).filter( + faults_tbl.c.instance_uuid.in_(instance_uuids) + ).group_by( + faults_tbl.c.instance_uuid + ).subquery(name="latest_faults") + + query = query.join(latest_faults, + faults_tbl.c.id == latest_faults.c.max_id) + else: + query = query.filter(models.InstanceFault.instance_uuid.in_( + instance_uuids)).order_by(desc("id")) output = {} for instance_uuid in instance_uuids: output[instance_uuid] = [] - for row in rows: - data = dict(row) - output[row['instance_uuid']].append(data) + for row in query: + output[row.instance_uuid].append(row._asdict()) return output diff --git a/nova/objects/instance.py b/nova/objects/instance.py index 5436800f49..c1a0381bd9 100644 --- a/nova/objects/instance.py +++ b/nova/objects/instance.py @@ -1342,12 +1342,11 @@ 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_by_instance_uuids( + faults = objects.InstanceFaultList.get_latest_by_instance_uuids( self._context, uuids) faults_by_uuid = {} for fault in faults: - if fault.instance_uuid not in faults_by_uuid: - faults_by_uuid[fault.instance_uuid] = fault + faults_by_uuid[fault.instance_uuid] = fault for instance in self: if instance.uuid in faults_by_uuid: diff --git a/nova/objects/instance_fault.py b/nova/objects/instance_fault.py index 60af162869..33ed4b4eb6 100644 --- a/nova/objects/instance_fault.py +++ b/nova/objects/instance_fault.py @@ -96,12 +96,22 @@ class InstanceFaultList(base.ObjectListBase, base.NovaObject): # Version 1.0: Initial version # InstanceFault <= version 1.1 # Version 1.1: InstanceFault version 1.2 - VERSION = '1.1' + # Version 1.2: Added get_latest_by_instance_uuids() method + VERSION = '1.2' fields = { 'objects': fields.ListOfObjectsField('InstanceFault'), } + @base.remotable_classmethod + def get_latest_by_instance_uuids(cls, context, instance_uuids): + db_faultdict = db.instance_fault_get_by_instance_uuids(context, + instance_uuids, + latest=True) + db_faultlist = itertools.chain(*db_faultdict.values()) + return base.obj_make_list(context, cls(context), objects.InstanceFault, + db_faultlist) + @base.remotable_classmethod def get_by_instance_uuids(cls, context, instance_uuids): db_faultdict = db.instance_fault_get_by_instance_uuids(context, diff --git a/nova/tests/unit/db/test_db_api.py b/nova/tests/unit/db/test_db_api.py index 97a06b7f94..70bb15d14a 100644 --- a/nova/tests/unit/db/test_db_api.py +++ b/nova/tests/unit/db/test_db_api.py @@ -4187,6 +4187,33 @@ class InstanceFaultTestCase(test.TestCase, ModelsObjectComparatorMixin): for uuid in uuids: self._assertEqualListsOfObjects(expected[uuid], faults[uuid]) + def test_instance_fault_get_latest_by_instance(self): + """Ensure we can retrieve only latest faults for instance.""" + uuids = [uuidsentinel.uuid1, uuidsentinel.uuid2] + fault_codes = [404, 500] + expected = {} + + # Create faults + for uuid in uuids: + db.instance_create(self.ctxt, {'uuid': uuid}) + + expected[uuid] = [] + for code in fault_codes: + fault_values = self._create_fault_values(uuid, code) + fault = db.instance_fault_create(self.ctxt, fault_values) + expected[uuid].append(fault) + + # We are only interested in the latest fault for each instance + for uuid in expected: + expected[uuid] = expected[uuid][-1:] + + # Ensure faults are saved + faults = db.instance_fault_get_by_instance_uuids(self.ctxt, uuids, + latest=True) + self.assertEqual(len(expected), len(faults)) + for uuid in uuids: + self._assertEqualListsOfObjects(expected[uuid], faults[uuid]) + def test_instance_faults_get_by_instance_uuids_no_faults(self): uuid = uuidsentinel.uuid1 # None should be returned when no faults exist. diff --git a/nova/tests/unit/objects/test_instance.py b/nova/tests/unit/objects/test_instance.py index 175bce0751..a47f0117ad 100644 --- a/nova/tests/unit/objects/test_instance.py +++ b/nova/tests/unit/objects/test_instance.py @@ -1834,7 +1834,8 @@ class _TestInstanceListObject(object): self.assertEqual(set(), inst.obj_what_changed()) mock_fault_get.assert_called_once_with(self.context, - [x.uuid for x in insts]) + [x.uuid for x in insts], + latest=True) @mock.patch('nova.objects.instance.Instance.obj_make_compatible') def test_get_by_security_group(self, mock_compat): diff --git a/nova/tests/unit/objects/test_objects.py b/nova/tests/unit/objects/test_objects.py index 292bac50cd..9b33c02cda 100644 --- a/nova/tests/unit/objects/test_objects.py +++ b/nova/tests/unit/objects/test_objects.py @@ -1104,7 +1104,7 @@ object_data = { 'InstanceDeviceMetadata': '1.0-74d78dd36aa32d26d2769a1b57caf186', 'InstanceExternalEvent': '1.1-6e446ceaae5f475ead255946dd443417', 'InstanceFault': '1.2-7ef01f16f1084ad1304a513d6d410a38', - 'InstanceFaultList': '1.1-f8ec07cbe3b60f5f07a8b7a06311ac0d', + 'InstanceFaultList': '1.2-6bb72de2872fe49ded5eb937a93f2451', 'InstanceGroup': '1.10-1a0c8c7447dc7ecb9da53849430c4a5f', 'InstanceGroupList': '1.7-be18078220513316abd0ae1b2d916873', 'InstanceInfoCache': '1.5-cd8b96fefe0fc8d4d337243ba0bf0e1e',