From 0e7812657b3ca90e618087b9fbb26816e60a90c3 Mon Sep 17 00:00:00 2001 From: Kiran Pawar Date: Tue, 27 Jun 2023 15:40:46 +0000 Subject: [PATCH] Add count info in 'snapshot list' API Added support for display count info in share snapshot list&detail APIs: 1. /v2/snapshots?with_count=True 2. /v2/snapshots/detail?with_count=True New microversion added 2.79 Closes-bug: #2024556 Change-Id: I37d8ca9022e2ea2c107c6695e20e951d7950043a --- api-ref/source/parameters.yaml | 9 ++- api-ref/source/snapshots.inc | 2 + manila/api/openstack/api_version_request.py | 4 +- .../openstack/rest_api_version_history.rst | 4 + manila/api/v1/share_snapshots.py | 32 +++++--- manila/api/v2/share_snapshots.py | 4 + manila/api/views/share_snapshots.py | 12 +-- manila/db/api.py | 19 +++++ manila/db/sqlalchemy/api.py | 51 ++++++++++-- manila/share/api.py | 36 ++++++++- manila/tests/api/v2/test_share_snapshots.py | 77 ++++++++++++++----- manila/tests/db/sqlalchemy/test_api.py | 36 +++++++++ ...fo-in-share-snapshot-eee90f1471f7a5c4.yaml | 4 + 13 files changed, 242 insertions(+), 48 deletions(-) create mode 100644 releasenotes/notes/add-count-info-in-share-snapshot-eee90f1471f7a5c4.yaml diff --git a/api-ref/source/parameters.yaml b/api-ref/source/parameters.yaml index 6f2f519793..b61a870a9a 100644 --- a/api-ref/source/parameters.yaml +++ b/api-ref/source/parameters.yaml @@ -653,11 +653,18 @@ user_id_query: type: string with_count_query: description: | - Whether to show ``count`` in API response or not, default is ``False``. + Whether to show ``count`` in share list API response or not, default is ``False``. in: query required: false type: boolean min_version: 2.42 +with_count_snapshot_query: + description: | + Whether to show ``count`` in share snapshot list API response or not, default is ``False``. + in: query + required: false + type: boolean + min_version: 2.79 # variables in body accepted: diff --git a/api-ref/source/snapshots.inc b/api-ref/source/snapshots.inc index c25ab229bc..7a6a182481 100644 --- a/api-ref/source/snapshots.inc +++ b/api-ref/source/snapshots.inc @@ -70,6 +70,7 @@ Request - all_tenants: all_tenants_query - name~: name_inexact_query - description~: description_inexact_query + - with_count: with_count_snapshot_query Response parameters ------------------- @@ -114,6 +115,7 @@ Request - all_tenants: all_tenants_query - name~: name_inexact_query - description~: description_inexact_query + - with_count: with_count_snapshot_query Response parameters ------------------- diff --git a/manila/api/openstack/api_version_request.py b/manila/api/openstack/api_version_request.py index 12c13ef888..4e7b09553c 100644 --- a/manila/api/openstack/api_version_request.py +++ b/manila/api/openstack/api_version_request.py @@ -195,6 +195,8 @@ REST_API_VERSION_HISTORY = """ * 2.76 - Added 'default_ad_site' field in security service object. * 2.77 - Added support for share transfer between different projects. * 2.78 - Added Share Network Subnet Metadata to Metadata API. + * 2.79 - Added ``with_count`` in share snapshot list API to get total + count info. """ @@ -202,7 +204,7 @@ REST_API_VERSION_HISTORY = """ # The default api version request is defined to be the # minimum version of the API supported. _MIN_API_VERSION = "2.0" -_MAX_API_VERSION = "2.78" +_MAX_API_VERSION = "2.79" DEFAULT_API_VERSION = _MIN_API_VERSION diff --git a/manila/api/openstack/rest_api_version_history.rst b/manila/api/openstack/rest_api_version_history.rst index 3aae24faf8..96182b8f1a 100644 --- a/manila/api/openstack/rest_api_version_history.rst +++ b/manila/api/openstack/rest_api_version_history.rst @@ -427,3 +427,7 @@ ____ --------------------------------- Added Metadata API methods (GET, PUT, POST, DELETE) to Share Network Subnets. + +2.79 +------------------------ + Added ``with_count`` in share snapshot list API to get total count info. diff --git a/manila/api/v1/share_snapshots.py b/manila/api/v1/share_snapshots.py index 5f6a906068..700fa280ca 100644 --- a/manila/api/v1/share_snapshots.py +++ b/manila/api/v1/share_snapshots.py @@ -30,6 +30,7 @@ from manila import db from manila import exception from manila.i18n import _ from manila import share +from manila import utils LOG = log.getLogger(__name__) @@ -100,6 +101,13 @@ class ShareSnapshotMixin(object): # Remove keys that are not related to share attrs search_opts.pop('limit', None) search_opts.pop('offset', None) + + show_count = False + if 'with_count' in search_opts: + show_count = utils.get_bool_from_api_params( + 'with_count', search_opts) + search_opts.pop('with_count') + sort_key, sort_dir = common.get_sort_params(search_opts) key_dict = {"name": "display_name", "description": "display_description"} @@ -137,19 +145,23 @@ class ShareSnapshotMixin(object): common.remove_invalid_options(context, search_opts, self._get_snapshots_search_options()) - snapshots = self.share_api.get_all_snapshots( - context, - search_opts=search_opts, - limit=limit, - offset=offset, - sort_key=sort_key, - sort_dir=sort_dir, - ) + total_count = None + if show_count: + count, snapshots = self.share_api.get_all_snapshots_with_count( + context, search_opts=search_opts, limit=limit, offset=offset, + sort_key=sort_key, sort_dir=sort_dir) + total_count = count + else: + snapshots = self.share_api.get_all_snapshots( + context, search_opts=search_opts, limit=limit, offset=offset, + sort_key=sort_key, sort_dir=sort_dir) if is_detail: - snapshots = self._view_builder.detail_list(req, snapshots) + snapshots = self._view_builder.detail_list( + req, snapshots, total_count) else: - snapshots = self._view_builder.summary_list(req, snapshots) + snapshots = self._view_builder.summary_list( + req, snapshots, total_count) return snapshots def _get_snapshots_search_options(self): diff --git a/manila/api/v2/share_snapshots.py b/manila/api/v2/share_snapshots.py index 257bcd3a22..9ff1b7ee3c 100644 --- a/manila/api/v2/share_snapshots.py +++ b/manila/api/v2/share_snapshots.py @@ -336,6 +336,10 @@ class ShareSnapshotsController(share_snapshots.ShareSnapshotMixin, req.GET.pop('name~', None) req.GET.pop('description~', None) req.GET.pop('description', None) + + if req.api_version_request < api_version.APIVersionRequest("2.79"): + req.GET.pop('with_count', None) + return self._get_snapshots(req, is_detail=False) @wsgi.Controller.api_version("2.0") diff --git a/manila/api/views/share_snapshots.py b/manila/api/views/share_snapshots.py index 41cd91aa35..795ecacccc 100644 --- a/manila/api/views/share_snapshots.py +++ b/manila/api/views/share_snapshots.py @@ -26,13 +26,13 @@ class ViewBuilder(common.ViewBuilder): "add_metadata" ] - def summary_list(self, request, snapshots): + def summary_list(self, request, snapshots, count=None): """Show a list of share snapshots without many details.""" - return self._list_view(self.summary, request, snapshots) + return self._list_view(self.summary, request, snapshots, count) - def detail_list(self, request, snapshots): + def detail_list(self, request, snapshots, count=None): """Detailed view of a list of share snapshots.""" - return self._list_view(self.detail, request, snapshots) + return self._list_view(self.detail, request, snapshots, count) def summary(self, request, snapshot): """Generic, non-detailed view of an share snapshot.""" @@ -84,7 +84,7 @@ class ViewBuilder(common.ViewBuilder): metadata = {} snapshot_dict['metadata'] = metadata - def _list_view(self, func, request, snapshots): + def _list_view(self, func, request, snapshots, count=None): """Provide a view for a list of share snapshots.""" snapshots_list = [func(request, snapshot)['snapshot'] for snapshot in snapshots] @@ -93,6 +93,8 @@ class ViewBuilder(common.ViewBuilder): self._collection_name) snapshots_dict = {self._collection_name: snapshots_list} + if count is not None: + snapshots_dict['count'] = count if snapshots_links: snapshots_dict['share_snapshots_links'] = snapshots_links diff --git a/manila/db/api.py b/manila/db/api.py index 58a6cfac1f..ddae316691 100644 --- a/manila/db/api.py +++ b/manila/db/api.py @@ -692,6 +692,15 @@ def share_snapshot_get_all(context, filters=None, limit=None, offset=None, sort_key=sort_key, sort_dir=sort_dir) +def share_snapshot_get_all_with_count(context, filters=None, limit=None, + offset=None, sort_key=None, + sort_dir=None): + """Get all snapshots.""" + return IMPL.share_snapshot_get_all_with_count( + context, filters=filters, limit=limit, offset=offset, + sort_key=sort_key, sort_dir=sort_dir) + + def share_snapshot_get_all_by_project(context, project_id, filters=None, limit=None, offset=None, sort_key=None, sort_dir=None): @@ -701,6 +710,16 @@ def share_snapshot_get_all_by_project(context, project_id, filters=None, sort_key=sort_key, sort_dir=sort_dir) +def share_snapshot_get_all_by_project_with_count(context, project_id, + filters=None, limit=None, + offset=None, sort_key=None, + sort_dir=None): + """Get all snapshots belonging to a project.""" + return IMPL.share_snapshot_get_all_by_project_with_count( + context, project_id, filters=filters, limit=limit, offset=offset, + sort_key=sort_key, sort_dir=sort_dir) + + def share_snapshot_get_all_for_share(context, share_id, filters=None, sort_key=None, sort_dir=None): """Get all snapshots for a share.""" diff --git a/manila/db/sqlalchemy/api.py b/manila/db/sqlalchemy/api.py index ed0de99b22..7add5378d3 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -3319,7 +3319,8 @@ def share_snapshot_get(context, snapshot_id, project_only=True, session=None): def _share_snapshot_get_all_with_filters(context, project_id=None, share_id=None, filters=None, limit=None, offset=None, - sort_key=None, sort_dir=None): + sort_key=None, sort_dir=None, + show_count=False): """Retrieves all snapshots. If no sorting parameters are specified then returned snapshots are sorted @@ -3389,13 +3390,25 @@ def _share_snapshot_get_all_with_filters(context, project_id=None, query = exact_filter(query, models.ShareSnapshot, filters, legal_filter_keys) - query = utils.paginate_query(query, models.ShareSnapshot, limit, - sort_key=sort_key, - sort_dir=sort_dir, - offset=offset) + query = apply_sorting(models.ShareSnapshot, query, sort_key, sort_dir) - # Returns list of shares that satisfy filters - return query.all() + count = None + if show_count: + count = query.count() + + if limit is not None: + query = query.limit(limit) + + if offset: + query = query.offset(offset) + + # Returns list of share snapshots that satisfy filters + query = query.all() + + if show_count: + return count, query + + return query @require_admin_context @@ -3406,6 +3419,17 @@ def share_snapshot_get_all(context, filters=None, limit=None, offset=None, offset=offset, sort_key=sort_key, sort_dir=sort_dir) +@require_admin_context +def share_snapshot_get_all_with_count(context, filters=None, limit=None, + offset=None, sort_key=None, + sort_dir=None): + count, query = _share_snapshot_get_all_with_filters( + context, filters=filters, limit=limit, + offset=offset, sort_key=sort_key, sort_dir=sort_dir, + show_count=True) + return count, query + + @require_context def share_snapshot_get_all_by_project(context, project_id, filters=None, limit=None, offset=None, @@ -3416,6 +3440,19 @@ def share_snapshot_get_all_by_project(context, project_id, filters=None, offset=offset, sort_key=sort_key, sort_dir=sort_dir) +@require_context +def share_snapshot_get_all_by_project_with_count(context, project_id, + filters=None, limit=None, + offset=None, sort_key=None, + sort_dir=None): + authorize_project_context(context, project_id) + count, query = _share_snapshot_get_all_with_filters( + context, project_id=project_id, filters=filters, limit=limit, + offset=offset, sort_key=sort_key, sort_dir=sort_dir, + show_count=True) + return count, query + + @require_context def share_snapshot_get_all_for_share(context, share_id, filters=None, sort_key=None, sort_dir=None): diff --git a/manila/share/api.py b/manila/share/api.py index f9e6951256..87218a30c8 100644 --- a/manila/share/api.py +++ b/manila/share/api.py @@ -2177,6 +2177,21 @@ class API(base.Base): def get_all_snapshots(self, context, search_opts=None, limit=None, offset=None, sort_key='share_id', sort_dir='desc'): + return self._get_all_snapshots(context, search_opts=search_opts, + limit=limit, offset=offset, + sort_key=sort_key, sort_dir=sort_dir) + + def get_all_snapshots_with_count(self, context, search_opts=None, + limit=None, offset=None, + sort_key='share_id', sort_dir='desc'): + return self._get_all_snapshots(context, search_opts=search_opts, + limit=limit, offset=offset, + sort_key=sort_key, sort_dir=sort_dir, + show_count=True) + + def _get_all_snapshots(self, context, search_opts=None, limit=None, + offset=None, sort_key='share_id', sort_dir='desc', + show_count=False): policy.check_policy(context, 'share_snapshot', 'get_all_snapshots') search_opts = search_opts or {} @@ -2193,17 +2208,32 @@ class API(base.Base): "'%(v)s'.") % {'k': k, 'v': string_args[k]} raise exception.InvalidInput(reason=msg) + get_methods = { + 'get_all': ( + self.db.share_snapshot_get_all_with_count + if show_count else self.db.share_snapshot_get_all), + 'get_all_by_project': ( + self.db.share_snapshot_get_all_by_project_with_count + if show_count else self.db.share_snapshot_get_all_by_project)} + if context.is_admin and all_tenants: - snapshots = self.db.share_snapshot_get_all( + result = get_methods['get_all']( context, filters=search_opts, limit=limit, offset=offset, sort_key=sort_key, sort_dir=sort_dir) else: - snapshots = self.db.share_snapshot_get_all_by_project( + result = get_methods['get_all_by_project']( context, context.project_id, filters=search_opts, limit=limit, offset=offset, sort_key=sort_key, sort_dir=sort_dir) - return snapshots + if show_count: + count = result[0] + snapshots = result[1] + else: + snapshots = result + + result = (count, snapshots) if show_count else snapshots + return result def get_latest_snapshot_for_share(self, context, share_id): """Get the newest snapshot of a share.""" diff --git a/manila/tests/api/v2/test_share_snapshots.py b/manila/tests/api/v2/test_share_snapshots.py index c3694a2b1a..d9eb773420 100644 --- a/manila/tests/api/v2/test_share_snapshots.py +++ b/manila/tests/api/v2/test_share_snapshots.py @@ -196,6 +196,10 @@ class ShareSnapshotAPITest(test.TestCase): api_version.APIVersionRequest('2.36')): search_opts.pop('name') search_opts['display_name~'] = 'fake_name' + if (api_version.APIVersionRequest(version) >= + api_version.APIVersionRequest('2.79')): + search_opts.update({'with_count': 'true'}) + # fake_key should be filtered for non-admin url = '/v2/fake/snapshots?fake_key=fake_value' for k, v in search_opts.items(): @@ -203,14 +207,21 @@ class ShareSnapshotAPITest(test.TestCase): req = fakes.HTTPRequest.blank( url, use_admin_context=use_admin_context, version=version) + method = 'get_all_snapshots' db_snapshots = [ {'id': 'id1', 'display_name': 'n1', 'status': 'fake_status', }, {'id': 'id2', 'display_name': 'n2', 'status': 'fake_status', }, {'id': 'id3', 'display_name': 'n3', 'status': 'fake_status', }, ] - snapshots = [db_snapshots[1]] - self.mock_object(share_api.API, 'get_all_snapshots', - mock.Mock(return_value=snapshots)) + + mock_action = {'return_value': [db_snapshots[1]]} + if (api_version.APIVersionRequest(version) >= + api_version.APIVersionRequest('2.79')): + method = 'get_all_snapshots_with_count' + mock_action = {'side_effect': [(1, [db_snapshots[1]])]} + + mock_get_all_snapshots = ( + self.mock_object(share_api.API, method, mock.Mock(**mock_action))) result = self.controller.index(req) @@ -225,7 +236,8 @@ class ShareSnapshotAPITest(test.TestCase): search_opts_expected['display_name'] = search_opts['name'] if use_admin_context: search_opts_expected.update({'fake_key': 'fake_value'}) - share_api.API.get_all_snapshots.assert_called_once_with( + + mock_get_all_snapshots.assert_called_once_with( req.environ['manila.context'], limit=int(search_opts['limit']), offset=int(search_opts['offset']), @@ -234,14 +246,19 @@ class ShareSnapshotAPITest(test.TestCase): search_opts=search_opts_expected, ) self.assertEqual(1, len(result['snapshots'])) - self.assertEqual(snapshots[0]['id'], result['snapshots'][0]['id']) + self.assertEqual(db_snapshots[1]['id'], result['snapshots'][0]['id']) self.assertEqual( - snapshots[0]['display_name'], result['snapshots'][0]['name']) + db_snapshots[1]['display_name'], result['snapshots'][0]['name']) + if (api_version.APIVersionRequest(version) >= + api_version.APIVersionRequest('2.79')): + self.assertEqual(1, result['count']) @ddt.data({'version': '2.35', 'use_admin_context': True}, {'version': '2.36', 'use_admin_context': True}, + {'version': '2.79', 'use_admin_context': True}, {'version': '2.35', 'use_admin_context': False}, - {'version': '2.36', 'use_admin_context': False}) + {'version': '2.36', 'use_admin_context': False}, + {'version': '2.79', 'use_admin_context': False}) @ddt.unpack def test_snapshot_list_summary_with_search_opts(self, version, use_admin_context): @@ -288,14 +305,20 @@ class ShareSnapshotAPITest(test.TestCase): self.assertEqual(1, len(result['snapshots'])) self.assertEqual(snapshots[0]['id'], result['snapshots'][0]['id']) - def _snapshot_list_detail_with_search_opts(self, use_admin_context): + def _snapshot_list_detail_with_search_opts(self, version, + use_admin_context): search_opts = fake_share.search_opts() + if (api_version.APIVersionRequest(version) >= + api_version.APIVersionRequest('2.79')): + search_opts.update({'with_count': 'true'}) + # fake_key should be filtered for non-admin url = '/v2/fake/shares/detail?fake_key=fake_value' for k, v in search_opts.items(): url = url + '&' + k + '=' + v req = fakes.HTTPRequest.blank(url, use_admin_context=use_admin_context) + method = 'get_all_snapshots' db_snapshots = [ { 'id': 'id1', @@ -317,10 +340,14 @@ class ShareSnapshotAPITest(test.TestCase): 'aggregate_status': 'fake_status', }, ] - snapshots = [db_snapshots[1]] + mock_action = {'return_value': [db_snapshots[1]]} + if (api_version.APIVersionRequest(version) >= + api_version.APIVersionRequest('2.79')): + method = 'get_all_snapshots_with_count' + mock_action = {'side_effect': [(1, [db_snapshots[1]])]} - self.mock_object(share_api.API, 'get_all_snapshots', - mock.Mock(return_value=snapshots)) + mock_get_all_snapshots = ( + self.mock_object(share_api.API, method, mock.Mock(**mock_action))) result = self.controller.detail(req) @@ -331,7 +358,7 @@ class ShareSnapshotAPITest(test.TestCase): } if use_admin_context: search_opts_expected.update({'fake_key': 'fake_value'}) - share_api.API.get_all_snapshots.assert_called_once_with( + mock_get_all_snapshots.assert_called_once_with( req.environ['manila.context'], limit=int(search_opts['limit']), offset=int(search_opts['offset']), @@ -340,19 +367,27 @@ class ShareSnapshotAPITest(test.TestCase): search_opts=search_opts_expected, ) self.assertEqual(1, len(result['snapshots'])) - self.assertEqual(snapshots[0]['id'], result['snapshots'][0]['id']) + self.assertEqual(db_snapshots[1]['id'], result['snapshots'][0]['id']) self.assertEqual( - snapshots[0]['display_name'], result['snapshots'][0]['name']) + db_snapshots[1]['display_name'], result['snapshots'][0]['name']) self.assertEqual( - snapshots[0]['aggregate_status'], result['snapshots'][0]['status']) + db_snapshots[1]['aggregate_status'], + result['snapshots'][0]['status']) self.assertEqual( - snapshots[0]['share_id'], result['snapshots'][0]['share_id']) + db_snapshots[1]['share_id'], result['snapshots'][0]['share_id']) + if (api_version.APIVersionRequest(version) >= + api_version.APIVersionRequest('2.79')): + self.assertEqual(1, result['count']) - def test_snapshot_list_detail_with_search_opts_by_non_admin(self): - self._snapshot_list_detail_with_search_opts(use_admin_context=False) - - def test_snapshot_list_detail_with_search_opts_by_admin(self): - self._snapshot_list_detail_with_search_opts(use_admin_context=True) + @ddt.data({'version': '2.78', 'use_admin_context': True}, + {'version': '2.78', 'use_admin_context': False}, + {'version': '2.79', 'use_admin_context': True}, + {'version': '2.79', 'use_admin_context': False}) + @ddt.unpack + def test_snapshot_list_detail_with_search_opts(self, version, + use_admin_context): + self._snapshot_list_detail_with_search_opts( + version=version, use_admin_context=use_admin_context) @ddt.data('2.0', '2.16', '2.17') def test_snapshot_list_detail(self, version): diff --git a/manila/tests/db/sqlalchemy/test_api.py b/manila/tests/db/sqlalchemy/test_api.py index 0fc5065e34..ad7f354e9c 100644 --- a/manila/tests/db/sqlalchemy/test_api.py +++ b/manila/tests/db/sqlalchemy/test_api.py @@ -1732,6 +1732,42 @@ class ShareSnapshotDatabaseAPITestCase(test.TestCase): self.assertEqual(1, len(actual_result.instances)) self.assertSubDictMatch(values, actual_result.to_dict()) + @ddt.data( + ({'with_count': True}, 3, 3), + ({'with_count': True, 'limit': 2}, 3, 2) + ) + @ddt.unpack + def test_share_snapshot_get_all_with_count(self, filters, + amount_of_share_snapshots, + expected_share_snapshots_len): + share = db_utils.create_share(size=1) + values = { + 'share_id': share['id'], + 'size': share['size'], + 'user_id': share['user_id'], + 'project_id': share['project_id'], + 'status': constants.STATUS_CREATING, + 'progress': '0%', + 'share_size': share['size'], + 'display_description': 'fake_count_test', + 'share_proto': share['share_proto'], + } + + # consider only shares created in this function + filters.update({'share_id': share['id']}) + + for i in range(amount_of_share_snapshots): + tmp_values = copy.deepcopy(values) + tmp_values['display_name'] = 'fake_name_%s' % str(i) + db_api.share_snapshot_create(self.ctxt, tmp_values) + + limit = filters.get('limit') + count, share_snapshots = db_api.share_snapshot_get_all_with_count( + self.ctxt, filters=filters, limit=limit) + + self.assertEqual(count, amount_of_share_snapshots) + self.assertEqual(expected_share_snapshots_len, len(share_snapshots)) + def test_share_snapshot_get_all_with_filters_some(self): expected_status = constants.STATUS_AVAILABLE filters = { diff --git a/releasenotes/notes/add-count-info-in-share-snapshot-eee90f1471f7a5c4.yaml b/releasenotes/notes/add-count-info-in-share-snapshot-eee90f1471f7a5c4.yaml new file mode 100644 index 0000000000..837bc7137e --- /dev/null +++ b/releasenotes/notes/add-count-info-in-share-snapshot-eee90f1471f7a5c4.yaml @@ -0,0 +1,4 @@ +--- +features: + - | + Added total count info in Manila's /snapshots and /snapshots/detail APIs.