diff --git a/manila/api/common.py b/manila/api/common.py index 5091bc1e54..a007523ab8 100644 --- a/manila/api/common.py +++ b/manila/api/common.py @@ -99,18 +99,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: @@ -131,6 +122,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 ed713ed242..2d8fd9e8d6 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' ) def update(self, req, id, body): diff --git a/manila/db/sqlalchemy/api.py b/manila/db/sqlalchemy/api.py index 5b4c9ec912..63d732ba06 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -1877,6 +1877,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 d811f2fcea..94065030ee 100644 --- a/manila/share/api.py +++ b/manila/share/api.py @@ -1497,6 +1497,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 0a8a8bad5c..e73e443d7f 100644 --- a/manila/tests/api/v1/test_shares.py +++ b/manila/tests/api/v1/test_shares.py @@ -228,6 +228,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']) @@ -294,6 +295,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']) @@ -334,6 +336,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']) @@ -398,6 +401,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']) @@ -531,7 +535,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) @@ -545,7 +549,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'] @@ -629,7 +636,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) @@ -643,6 +650,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 0c992da57c..f4aceadcb1 100644 --- a/manila/tests/api/v2/test_shares.py +++ b/manila/tests/api/v2/test_shares.py @@ -650,6 +650,7 @@ class ShareAPITest(test.TestCase): expected = self._get_expected_share_detailed_response(shr) self.assertDictMatch(expected, res_dict) + # pylint: disable=unsubscriptable-object self.assertEqual("fakenetid", create_mock.call_args[1]['share_network_id']) @@ -1178,6 +1179,7 @@ class ShareAPITest(test.TestCase): res_dict = self.controller.create(req, body) expected = self._get_expected_share_detailed_response(shr) self.assertEqual(expected, res_dict) + # pylint: disable=unsubscriptable-object self.assertEqual(parent_share_net, create_mock.call_args[1]['share_network_id']) @@ -1217,6 +1219,7 @@ class ShareAPITest(test.TestCase): res_dict = self.controller.create(req, body) expected = self._get_expected_share_detailed_response(shr) self.assertDictMatch(expected, res_dict) + # pylint: disable=unsubscriptable-object self.assertEqual(parent_share_net, create_mock.call_args[1]['share_network_id']) @@ -1554,7 +1557,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) @@ -1568,6 +1571,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')): @@ -1596,7 +1601,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', @@ -1679,7 +1684,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) @@ -1693,7 +1698,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'] = ( @@ -1731,7 +1739,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 e0d2aa43b1..1e17de76bc 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.