From 7f4c47c6423b121a5d8d79c192a4922500a749a1 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Thu, 24 Apr 2025 12:59:45 +0200 Subject: [PATCH] [quota]Refactor group counting to scatter-gather The legacy server group member counting logic fits well to use the existing scatter-gather logic instead of rolling its own thread handling. This replaces a direct eventlet dependency with an indirect, shared one, in the scatter-gather therefore making the eventlet removal work easier Change-Id: I6d1b5f9654df2a93bd3722a5813d5ad3a7d1c94a --- nova/quota.py | 48 +++++++---------- nova/tests/unit/test_quota.py | 98 +++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 28 deletions(-) diff --git a/nova/quota.py b/nova/quota.py index c8684792f04f..162c57df63e4 100644 --- a/nova/quota.py +++ b/nova/quota.py @@ -32,7 +32,6 @@ from nova.limit import local as local_limit from nova.limit import placement as placement_limit from nova import objects from nova.scheduler.client import report -from nova import utils LOG = logging.getLogger(__name__) CONF = nova.conf.CONF @@ -1208,38 +1207,31 @@ def _keypair_get_count_by_user(context, user_id): def _server_group_count_members_by_user_legacy(context, group, user_id): - # NOTE(melwitt): This is mostly duplicated from - # InstanceGroup.count_members_by_user() to query across multiple cells. - # We need to be able to pass the correct cell context to - # InstanceList.get_by_filters(). - # NOTE(melwitt): Counting across cells for instances means we will miss - # counting resources if a cell is down. - cell_mappings = objects.CellMappingList.get_all(context) - greenthreads = [] filters = {'deleted': False, 'user_id': user_id, 'uuid': group.members} - for cell_mapping in cell_mappings: - with nova_context.target_cell(context, cell_mapping) as cctxt: - greenthreads.append(utils.spawn( - objects.InstanceList.get_by_filters, cctxt, filters, - expected_attrs=[])) - instances = objects.InstanceList(objects=[]) - for greenthread in greenthreads: - found = greenthread.wait() - instances = instances + found - # Count build requests using the same filters to catch group members - # that are not yet created in a cell. - # NOTE(mriedem): BuildRequestList.get_by_filters is not very efficient for - # what we need and we can optimize this with a new query method. - build_requests = objects.BuildRequestList.get_by_filters(context, filters) + + def group_member_uuids(cctxt): + return {inst.uuid for inst in objects.InstanceList.get_by_filters( + cctxt, filters, expected_attrs=[])} + # Ignore any duplicates since build requests and instances can co-exist # for a short window of time after the instance is created in a cell but # before the build request is deleted. - instance_uuids = [inst.uuid for inst in instances] - count = len(instances) + instance_uuids = set() + + # NOTE(melwitt): Counting across cells for instances means we will miss + # counting resources if a cell is down. + per_cell = nova_context.scatter_gather_all_cells( + context, group_member_uuids) + for uuids in per_cell.values(): + instance_uuids |= uuids + + # Count build requests using the same filters to catch group members + # that are not yet created in a cell. + build_requests = objects.BuildRequestList.get_by_filters(context, filters) for build_request in build_requests: - if build_request.instance_uuid not in instance_uuids: - count += 1 - return {'user': {'server_group_members': count}} + instance_uuids.add(build_request.instance_uuid) + + return {'user': {'server_group_members': len(instance_uuids)}} def is_qfd_populated(context): diff --git a/nova/tests/unit/test_quota.py b/nova/tests/unit/test_quota.py index 7979d83e918b..7c2aa585ec64 100644 --- a/nova/tests/unit/test_quota.py +++ b/nova/tests/unit/test_quota.py @@ -2287,3 +2287,101 @@ class QuotaCountTestCase(test.NoDBTestCase): quota._instances_cores_ram_count(mock.sentinel.context, mock.sentinel.project_id) mock_uid_qfd_populated.assert_not_called() + + +class LegacyGroupMemberQuotaTestCase(test.NoDBTestCase): + @mock.patch('nova.objects.BuildRequestList.get_by_filters') + @mock.patch('nova.objects.InstanceList.get_by_filters') + @mock.patch('nova.objects.CellMappingList.get_all') + def test_no_cells_no_build_reqs( + self, mock_get_cell_mappings, mock_inst_get, mock_build_req_get + ): + mock_get_cell_mappings.return_value = [] + group = objects.InstanceGroup() + group.members = [] + + self.assertEqual( + {'user': {'server_group_members': 0}}, + quota._server_group_count_members_by_user_legacy( + mock.sentinel.context, group, mock.sentinel.user_id)) + + mock_get_cell_mappings.assert_called_once() + mock_inst_get.assert_not_called() + mock_build_req_get.assert_called_once_with( + mock.sentinel.context, + {'deleted': False, 'user_id': mock.sentinel.user_id, 'uuid': []}) + + @mock.patch('nova.objects.BuildRequestList.get_by_filters') + @mock.patch('nova.objects.InstanceList.get_by_filters') + @mock.patch('nova.objects.CellMappingList.get_all') + def test_no_cells_just_build_reqs( + self, mock_get_cell_mappings, mock_inst_get, mock_build_req_get + ): + mock_get_cell_mappings.return_value = [] + br = objects.BuildRequest() + br.instance_uuid = uuids.inst1 + mock_build_req_get.return_value = objects.BuildRequestList( + objects=[br]) + group = objects.InstanceGroup() + group.members = [] + + self.assertEqual( + {'user': {'server_group_members': 1}}, + quota._server_group_count_members_by_user_legacy( + mock.sentinel.context, group, mock.sentinel.user_id)) + + mock_get_cell_mappings.assert_called_once() + mock_inst_get.assert_not_called() + mock_build_req_get.assert_called_once_with( + mock.sentinel.context, + {'deleted': False, 'user_id': mock.sentinel.user_id, 'uuid': []}) + + @mock.patch('nova.objects.BuildRequestList.get_by_filters') + @mock.patch('nova.objects.InstanceList.get_by_filters') + @mock.patch('nova.objects.CellMappingList.get_all') + def test_cells_and_build_reqs( + self, mock_get_cell_mappings, mock_inst_get, mock_build_req_get + ): + cell1 = objects.CellMapping() + cell1.name = "cell1" + cell1.uuid = uuids.cell1 + cell2 = objects.CellMapping() + cell2.name = "cell2" + cell2.uuid = uuids.cell2 + mock_get_cell_mappings.return_value = objects.CellMappingList( + objects=[cell1, cell2]) + + br1 = objects.BuildRequest() + br1.instance_uuid = uuids.inst1 + br2 = objects.BuildRequest() + br2.instance_uuid = uuids.inst2 + mock_build_req_get.return_value = objects.BuildRequestList( + objects=[br1, br2]) + + inst1 = objects.Instance() + inst1.uuid = uuids.inst1 # same as br1 + + inst3 = objects.Instance() + inst3.uuid = uuids.inst3 + + mock_inst_get.side_effect = [ + # cell1 + objects.InstanceList(objects=[inst1]), + # cell2 + objects.InstanceList(objects=[inst3]), + ] + + group = objects.InstanceGroup() + group.members = [] + + # as br1 and inst1 counted only once as they are the same + self.assertEqual( + {'user': {'server_group_members': 3}}, + quota._server_group_count_members_by_user_legacy( + mock.sentinel.context, group, mock.sentinel.user_id)) + + mock_get_cell_mappings.assert_called_once() + self.assertEqual(2, len(mock_inst_get.mock_calls)) + mock_build_req_get.assert_called_once_with( + mock.sentinel.context, + {'deleted': False, 'user_id': mock.sentinel.user_id, 'uuid': []})