diff --git a/manila/api/common.py b/manila/api/common.py index 015a7f242c..b7f51f0311 100644 --- a/manila/api/common.py +++ b/manila/api/common.py @@ -102,18 +102,9 @@ def _get_marker_param(request): return request.GET['marker'] -def limited(items, request, max_limit=CONF.osapi_max_limit): - """Return a slice of items according to requested offset and limit. +def _validate_pagination_query(request, max_limit=CONF.osapi_max_limit): + """Validate the given request query and return limit and offset.""" - :param items: A sliceable entity - :param request: ``wsgi.Request`` possibly containing 'offset' and 'limit' - GET variables. 'offset' is where to start in the list, - and 'limit' is the maximum number of items to return. If - 'limit' is not specified, 0, or > max_limit, we default - to max_limit. Negative values for either offset or limit - will cause exc.HTTPBadRequest() exceptions to be raised. - :kwarg max_limit: The maximum number of items to return from 'items' - """ try: offset = int(request.GET.get('offset', 0)) except ValueError: @@ -134,6 +125,23 @@ def limited(items, request, max_limit=CONF.osapi_max_limit): msg = _('offset param must be positive') raise webob.exc.HTTPBadRequest(explanation=msg) + return limit, offset + + +def limited(items, request, max_limit=CONF.osapi_max_limit): + """Return a slice of items according to requested offset and limit. + + :param items: A sliceable entity + :param request: ``wsgi.Request`` possibly containing 'offset' and 'limit' + GET variables. 'offset' is where to start in the list, + and 'limit' is the maximum number of items to return. If + 'limit' is not specified, 0, or > max_limit, we default + to max_limit. Negative values for either offset or limit + will cause exc.HTTPBadRequest() exceptions to be raised. + :kwarg max_limit: The maximum number of items to return from 'items' + """ + limit, offset = _validate_pagination_query(request, max_limit) + limit = min(max_limit, limit or max_limit) range_end = offset + limit return items[offset:range_end] diff --git a/manila/api/v1/shares.py b/manila/api/v1/shares.py index 0e8a8b4e1e..4c782cb23b 100644 --- a/manila/api/v1/shares.py +++ b/manila/api/v1/shares.py @@ -124,12 +124,12 @@ class ShareMixin(object): """Returns a list of shares, transformed through view builder.""" context = req.environ['manila.context'] + common._validate_pagination_query(req) + search_opts = {} search_opts.update(req.GET) # Remove keys that are not related to share attrs - search_opts.pop('limit', None) - search_opts.pop('offset', None) sort_key = search_opts.pop('sort_key', 'created_at') sort_dir = search_opts.pop('sort_dir', 'desc') @@ -174,14 +174,10 @@ class ShareMixin(object): if show_count: total_count = len(shares) - limited_list = common.limited(shares, req) - if is_detail: - shares = self._view_builder.detail_list(req, limited_list, - total_count) + shares = self._view_builder.detail_list(req, shares, total_count) else: - shares = self._view_builder.summary_list(req, limited_list, - total_count) + shares = self._view_builder.summary_list(req, shares, total_count) return shares def _get_share_search_options(self): @@ -196,7 +192,7 @@ class ShareMixin(object): 'is_public', 'metadata', 'extra_specs', 'sort_key', 'sort_dir', 'share_group_id', 'share_group_snapshot_id', 'export_location_id', 'export_location_path', 'display_name~', 'display_description~', - 'display_description' + 'display_description', 'limit', 'offset' ) @wsgi.Controller.authorize diff --git a/manila/db/sqlalchemy/api.py b/manila/db/sqlalchemy/api.py index 787918240e..ea009ef0b9 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -1899,6 +1899,10 @@ def _share_get_all_with_filters(context, project_id=None, share_server_id=None, msg = _("Wrong sorting key provided - '%s'.") % sort_key raise exception.InvalidInput(reason=msg) + if 'limit' in filters: + offset = filters.get('offset', 0) + query = query.limit(filters['limit']).offset(offset) + # Returns list of shares that satisfy filters. query = query.all() return query diff --git a/manila/share/api.py b/manila/share/api.py index ced4346b09..a8663adf23 100644 --- a/manila/share/api.py +++ b/manila/share/api.py @@ -1605,6 +1605,10 @@ class API(base.Base): msg = _("Wrong extra specs filter provided: " "%s.") % six.text_type(filters['extra_specs']) raise exception.InvalidInput(reason=msg) + if 'limit' in search_opts: + filters['limit'] = search_opts.pop('limit') + if 'offset' in search_opts: + filters['offset'] = search_opts.pop('offset') if not (isinstance(sort_key, six.string_types) and sort_key): msg = _("Wrong sort_key filter provided: " "'%s'.") % six.text_type(sort_key) diff --git a/manila/tests/api/v1/test_shares.py b/manila/tests/api/v1/test_shares.py index cc98089f81..161068deac 100644 --- a/manila/tests/api/v1/test_shares.py +++ b/manila/tests/api/v1/test_shares.py @@ -249,6 +249,7 @@ class ShareAPITest(test.TestCase): expected = self._get_expected_share_detailed_response(shr) expected['share'].pop('snapshot_support') self.assertEqual(expected, res_dict) + # pylint: disable=unsubscriptable-object self.assertEqual("fakenetid", create_mock.call_args[1]['share_network_id']) @@ -323,6 +324,7 @@ class ShareAPITest(test.TestCase): expected = self._get_expected_share_detailed_response(shr) expected['share'].pop('snapshot_support') self.assertEqual(expected, res_dict) + # pylint: disable=unsubscriptable-object self.assertEqual(parent_share_net, create_mock.call_args[1]['share_network_id']) @@ -367,6 +369,7 @@ class ShareAPITest(test.TestCase): expected = self._get_expected_share_detailed_response(shr) expected['share'].pop('snapshot_support') self.assertEqual(expected, res_dict) + # pylint: disable=unsubscriptable-object self.assertEqual(parent_share_net, create_mock.call_args[1]['share_network_id']) @@ -436,6 +439,7 @@ class ShareAPITest(test.TestCase): expected = self._get_expected_share_detailed_response(shr) expected['share'].pop('snapshot_support') self.assertDictEqual(expected, res_dict) + # pylint: disable=unsubscriptable-object self.assertEqual(parent_share_net, create_mock.call_args[1]['share_network_id']) @@ -581,7 +585,7 @@ class ShareAPITest(test.TestCase): {'id': 'id3', 'display_name': 'n3'}, ] self.mock_object(share_api.API, 'get_all', - mock.Mock(return_value=shares)) + mock.Mock(return_value=[shares[1]])) result = self.controller.index(req) @@ -595,7 +599,10 @@ class ShareAPITest(test.TestCase): 'metadata': {'k1': 'v1'}, 'extra_specs': {'k2': 'v2'}, 'is_public': 'False', + 'limit': '1', + 'offset': '1' } + if use_admin_context: search_opts_expected.update({'fake_key': 'fake_value'}) search_opts_expected['host'] = search_opts['host'] @@ -679,7 +686,7 @@ class ShareAPITest(test.TestCase): {'id': 'id3', 'display_name': 'n3'}, ] self.mock_object(share_api.API, 'get_all', - mock.Mock(return_value=shares)) + mock.Mock(return_value=[shares[1]])) result = self.controller.detail(req) @@ -693,6 +700,8 @@ class ShareAPITest(test.TestCase): 'metadata': {'k1': 'v1'}, 'extra_specs': {'k2': 'v2'}, 'is_public': 'False', + 'limit': '1', + 'offset': '1' } if use_admin_context: search_opts_expected.update({'fake_key': 'fake_value'}) diff --git a/manila/tests/api/v2/test_shares.py b/manila/tests/api/v2/test_shares.py index 4d98b3971a..e054fbdaad 100644 --- a/manila/tests/api/v2/test_shares.py +++ b/manila/tests/api/v2/test_shares.py @@ -674,6 +674,7 @@ class ShareAPITest(test.TestCase): expected = self._get_expected_share_detailed_response( shr, version='2.7') self.assertDictMatch(expected, res_dict) + # pylint: disable=unsubscriptable-object self.assertEqual("fakenetid", create_mock.call_args[1]['share_network_id']) @@ -1253,6 +1254,7 @@ class ShareAPITest(test.TestCase): expected = self._get_expected_share_detailed_response( shr, version='2.7') self.assertEqual(expected, res_dict) + # pylint: disable=unsubscriptable-object self.assertEqual(parent_share_net, create_mock.call_args[1]['share_network_id']) @@ -1293,6 +1295,7 @@ class ShareAPITest(test.TestCase): expected = self._get_expected_share_detailed_response( shr, version='2.7') self.assertDictMatch(expected, res_dict) + # pylint: disable=unsubscriptable-object self.assertEqual(parent_share_net, create_mock.call_args[1]['share_network_id']) @@ -1590,7 +1593,7 @@ class ShareAPITest(test.TestCase): {'id': 'id3', 'display_name': 'n3'}, ] self.mock_object(share_api.API, 'get_all', - mock.Mock(return_value=shares)) + mock.Mock(return_value=[shares[1]])) result = self.controller.index(req) @@ -1604,6 +1607,8 @@ class ShareAPITest(test.TestCase): 'metadata': {'k1': 'v1'}, 'extra_specs': {'k2': 'v2'}, 'is_public': 'False', + 'limit': '1', + 'offset': '1' } if (api_version.APIVersionRequest(version) >= api_version.APIVersionRequest('2.35')): @@ -1632,7 +1637,7 @@ class ShareAPITest(test.TestCase): shares[1]['display_name'], result['shares'][0]['name']) if (api_version.APIVersionRequest(version) >= api_version.APIVersionRequest('2.42')): - self.assertEqual(3, result['count']) + self.assertEqual(1, result['count']) def test_share_list_summary(self): self.mock_object(share_api.API, 'get_all', @@ -1715,7 +1720,7 @@ class ShareAPITest(test.TestCase): ] self.mock_object(share_api.API, 'get_all', - mock.Mock(return_value=shares)) + mock.Mock(return_value=[shares[1]])) result = self.controller.detail(req) @@ -1729,7 +1734,10 @@ class ShareAPITest(test.TestCase): 'metadata': {'k1': 'v1'}, 'extra_specs': {'k2': 'v2'}, 'is_public': 'False', + 'limit': '1', + 'offset': '1' } + if (api_version.APIVersionRequest(version) >= api_version.APIVersionRequest('2.35')): search_opts_expected['export_location_id'] = ( @@ -1767,7 +1775,7 @@ class ShareAPITest(test.TestCase): result['shares'][0]['share_network_id']) if (api_version.APIVersionRequest(version) >= api_version.APIVersionRequest('2.42')): - self.assertEqual(3, result['count']) + self.assertEqual(1, result['count']) def _list_detail_common_expected(self, admin=False): share_dict = { diff --git a/manila/tests/db/sqlalchemy/test_api.py b/manila/tests/db/sqlalchemy/test_api.py index d2040ed613..3c8d37633b 100644 --- a/manila/tests/db/sqlalchemy/test_api.py +++ b/manila/tests/db/sqlalchemy/test_api.py @@ -544,6 +544,27 @@ class ShareDatabaseAPITestCase(test.TestCase): self.assertEqual(0, len(actual_result)) + @ddt.data((10, 5), (20, 5)) + @ddt.unpack + def test_share_get_all_with_limit(self, limit, offset): + for i in range(limit + 5): + db_utils.create_share() + + filters = {'limit': offset, 'offset': 0} + shares_not_requested = db_api.share_get_all( + self.ctxt, filters=filters) + + filters = {'limit': limit, 'offset': offset} + shares_requested = db_api.share_get_all(self.ctxt, filters=filters) + + shares_not_requested_ids = [s['id'] for s in shares_not_requested] + shares_requested_ids = [s['id'] for s in shares_requested] + + self.assertEqual(offset, len(shares_not_requested_ids)) + self.assertEqual(limit, len(shares_requested_ids)) + self.assertEqual(0, len( + set(shares_requested_ids) & set(shares_not_requested_ids))) + @ddt.data(None, 'writable') def test_share_get_has_replicas_field(self, replication_type): share = db_utils.create_share(replication_type=replication_type) diff --git a/releasenotes/notes/bug-1795463-fix-pagination-slowness-8fcda3746aa13940.yaml b/releasenotes/notes/bug-1795463-fix-pagination-slowness-8fcda3746aa13940.yaml new file mode 100644 index 0000000000..205347120a --- /dev/null +++ b/releasenotes/notes/bug-1795463-fix-pagination-slowness-8fcda3746aa13940.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + When the OpenStack administrator has a busy environment that contains many + shares, the list operation with `--limit` parameter was taking too long to + respond. This lag has now been fixed. See the `launchpad bug 1795463 + `_ for more details.