From c3bd31909d47d571d63e5a66672a4f895edccaec Mon Sep 17 00:00:00 2001 From: Chuan Miao Date: Tue, 11 Aug 2026 11:06:47 +0200 Subject: [PATCH] perf: improve share creation by using SQL aggregates in server limit checks When creating a share, the scheduler evaluates all available share servers to find one within configured per-server limits. Previously this fetched full ORM objects for every share instance and snapshot on each server, loading large result sets into Python just to compute a count and a size sum. On busy backends with many shares per server this adds significant latency to every share creation request. Replace those fetches with two SQL aggregate queries (COUNT + SUM) per share server, returning only the scalar values needed. - share_instance_count_and_size_sum_by_server: returns (count, size_sum) in a single query joining ShareInstance -> Share - share_snapshot_size_sum_by_server: returns total snapshot size using COALESCE(instance_size, snapshot.size), mirroring the ORM size property; share-group snapshot members (snapshot_id=NULL) are excluded as their size cannot be resolved - Missing ShareInstance.deleted filter added to the snapshot query to exclude snapshots belonging to soft-deleted instances Change-Id: Idcd378c654c2fb7fbf867e9eb62991cd2cbcb017 --- manila/db/api.py | 11 +++++++ manila/db/sqlalchemy/api.py | 51 ++++++++++++++++++++++++++++++ manila/share/manager.py | 23 ++++---------- manila/tests/share/test_manager.py | 32 ++++++++++--------- 4 files changed, 86 insertions(+), 31 deletions(-) diff --git a/manila/db/api.py b/manila/db/api.py index cd1ce2e047..fb37b0c179 100644 --- a/manila/db/api.py +++ b/manila/db/api.py @@ -396,6 +396,17 @@ def share_instance_sizes_sum_by_host(context, host): return IMPL.share_instance_sizes_sum_by_host(context, host) +def share_instance_count_and_size_sum_by_server(context, share_server_id): + """Returns (count, size_sum) of share instances on the given server.""" + return IMPL.share_instance_count_and_size_sum_by_server( + context, share_server_id) + + +def share_snapshot_size_sum_by_server(context, share_server_id): + """Returns total snapshot size for all snapshots on the given server.""" + return IMPL.share_snapshot_size_sum_by_server(context, share_server_id) + + def share_instance_purge(context, instance_id): """Removes share instance from database.""" return IMPL.share_instance_purge(context, instance_id) diff --git a/manila/db/sqlalchemy/api.py b/manila/db/sqlalchemy/api.py index cf12946134..c6f317f4e4 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -2181,6 +2181,57 @@ def share_instance_sizes_sum_by_host(context, host): return int(result[0] or 0) +@require_context +@context_manager.reader +def share_instance_count_and_size_sum_by_server(context, share_server_id): + """Return (count, size_sum) of share instances on the share server.""" + result = model_query( + context, models.ShareInstance, + func.count(models.ShareInstance.id), + func.sum(models.Share.size), + ).join( + models.ShareInstance.share, + ).filter( + models.ShareInstance.share_server_id == share_server_id, + ).first() + return int(result[0] or 0), int(result[1] or 0) + + +@require_context +@context_manager.reader +def share_snapshot_size_sum_by_server(context, share_server_id): + """Return total snapshot size for all snapshots on the given share server. + + Uses instance_size when set, otherwise falls back to ShareSnapshot.size, + mirroring the ShareSnapshotInstance.size property. + + Share-group snapshot members (snapshot_id=NULL) are excluded: their + snapshot relationship is NULL so their size cannot be resolved anyway. + """ + result = model_query( + context, models.ShareSnapshotInstance, + func.sum( + func.coalesce( + models.ShareSnapshotInstance.instance_size, + models.ShareSnapshot.size, + ) + ), + ).join( + models.ShareSnapshot, + models.ShareSnapshot.id == models.ShareSnapshotInstance.snapshot_id, + ).join( + models.ShareInstance, + models.ShareInstance.id == + models.ShareSnapshotInstance.share_instance_id, + ).filter( + models.ShareInstance.share_server_id == share_server_id, + models.ShareInstance.deleted == 'False', + models.ShareSnapshotInstance.deleted == 'False', + models.ShareSnapshot.deleted == 'False', + ).first() + return int(result[0] or 0) + + @require_context @context_manager.reader def share_instance_get_all_by_share_network(context, share_network_id): diff --git a/manila/share/manager.py b/manila/share/manager.py index 58fc7d5755..196d7afc28 100644 --- a/manila/share/manager.py +++ b/manila/share/manager.py @@ -881,24 +881,15 @@ def _check_share_server_backend_limits( return available_share_servers for ss in available_share_servers[:]: - share_instances = self.db.share_instance_get_all_by_share_server( - context, ss['id'], with_share_data=True) - if not share_instances: + num_instances, instances_size_sum = ( + self.db.share_instance_count_and_size_sum_by_server( + context, ss['id'])) + if num_instances == 0: continue - share_instance_ids = [si['id'] for si in share_instances] - share_snapshot_instances = ( - self.db.share_snapshot_instance_get_all_with_filters( - context, {"share_instance_ids": share_instance_ids}, - with_share_data=True)) - - server_instances_size_sum = 0 - num_instances = 0 + snapshot_size_sum = self.db.share_snapshot_size_sum_by_server( + context, ss['id']) - server_instances_size_sum += sum( - instance['size'] for instance in share_instances) - server_instances_size_sum += sum( - instance['size'] for instance in share_snapshot_instances) - num_instances += len(share_instances) + server_instances_size_sum = instances_size_sum + snapshot_size_sum # NOTE(carloss): If a share instance was not provided, means that # a share group is being requested and there aren't shares to diff --git a/manila/tests/share/test_manager.py b/manila/tests/share/test_manager.py index 0862dcd2c5..e34cfa8d68 100644 --- a/manila/tests/share/test_manager.py +++ b/manila/tests/share/test_manager.py @@ -3212,7 +3212,7 @@ def test__check_share_server_backend_limits( # Creates some snapshot instances to make sure they are being # accounted - snapshot_instances = [ + snapshots = [ db_utils.create_snapshot( size=resource_size, share_id=share['id'])['instance'] for i in range(3)] @@ -3226,12 +3226,15 @@ def test__check_share_server_backend_limits( driver_mock.max_shares_per_share_server = max_shares driver_mock.max_share_server_size = max_gigabytes self.share_manager.driver = driver_mock + num_instances = len(share_instances) + instances_size_sum = sum(si['size'] for si in share_instances) + snapshot_size_sum = sum(si['size'] for si in snapshots) self.mock_object( - db, 'share_instance_get_all_by_share_server', - mock.Mock(return_value=share_instances)) + db, 'share_instance_count_and_size_sum_by_server', + mock.Mock(return_value=(num_instances, instances_size_sum))) self.mock_object( - db, 'share_snapshot_instance_get_all_with_filters', - mock.Mock(return_value=snapshot_instances)) + db, 'share_snapshot_size_sum_by_server', + mock.Mock(return_value=snapshot_size_sum)) # NOTE(carloss): If with_share_instance, simulates the behavior where # the provide_share_server method call was not related to a request to @@ -3276,11 +3279,12 @@ def test__check_share_server_backend_limits_migrating_share(self): self.share_manager.driver = driver_mock self.mock_object( - db, 'share_instance_get_all_by_share_server', - mock.Mock(return_value=share_instances)) + db, 'share_instance_count_and_size_sum_by_server', + mock.Mock(return_value=(len(share_instances), + sum(s['size'] for s in share_instances)))) self.mock_object( - db, 'share_snapshot_instance_get_all_with_filters', - mock.Mock(return_value=[])) + db, 'share_snapshot_size_sum_by_server', + mock.Mock(return_value=0)) self.mock_object(db, 'share_get', mock.Mock(return_value=share)) self.mock_object(api.API, 'get_migrating_instances', mock.Mock(return_value=share_instance_ids)) @@ -3302,12 +3306,10 @@ def test__check_share_server_backend_limits_migrating_share(self): self.assertEqual( 1, len(available_share_servers)) - db.share_instance_get_all_by_share_server.assert_called_once_with( - self.context, share_servers[0]['id'], with_share_data=True) - (db.share_snapshot_instance_get_all_with_filters. - assert_called_once_with( - self.context, {"share_instance_ids": share_instance_ids}, - with_share_data=True)) + db.share_instance_count_and_size_sum_by_server.assert_called_once_with( + self.context, share_servers[0]['id']) + db.share_snapshot_size_sum_by_server.assert_called_once_with( + self.context, share_servers[0]['id']) db.share_get.assert_called_once_with(self.context, share['id']) api.API.get_migrating_instances.assert_called_once_with(share) db.share_instance_get.assert_called_once_with(