From 8263f911beaef26865d30c2cfee760c594175e5e Mon Sep 17 00:00:00 2001 From: silvacarloss Date: Mon, 8 Apr 2019 17:33:33 -0300 Subject: [PATCH] Fix pagination does not speed up queries bug This patch modifies the database api layer to fix the pagination slowness bug, which causes a delay while the administrator tries to list shares using the `--limit` option. This change was tested in a very busy environment with 800 shares. Before the change, the operation took about 25 seconds to be finished. Now, the operation takes about 3 seconds in the same environment. Change-Id: I89659452b0e033631f1318a2eabb7e120c9e5743 Closes-bug: #1795463 (cherry picked from commit 57edcbd1da4b61ce1a61db3be0d69755d968c36c) (cherry picked from commit 462b0e7461cdd00d51f15aacaa43854aeb0689f4) (cherry picked from commit bad03feea9a8470c9fe55f1a925d83e026353989) --- manila/api/common.py | 30 ++++++++++++------- manila/api/v1/shares.py | 14 ++++----- manila/db/sqlalchemy/api.py | 4 +++ manila/share/api.py | 4 +++ manila/tests/api/v1/test_shares.py | 13 ++++++-- manila/tests/api/v2/test_shares.py | 16 +++++++--- manila/tests/db/sqlalchemy/test_api.py | 21 +++++++++++++ ...-pagination-slowness-8fcda3746aa13940.yaml | 7 +++++ 8 files changed, 83 insertions(+), 26 deletions(-) create mode 100644 releasenotes/notes/bug-1795463-fix-pagination-slowness-8fcda3746aa13940.yaml diff --git a/manila/api/common.py b/manila/api/common.py index 4a6b8def8d..5bea7f9e55 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 362bc43a3e..a919220cff 100644 --- a/manila/api/v1/shares.py +++ b/manila/api/v1/shares.py @@ -123,12 +123,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') @@ -173,14 +173,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): @@ -195,7 +191,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 9b8942e83b..2da3922062 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 347b6e9354..362ae5eb3c 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 ffaa2f30cd..04c358f873 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 d58c458895..ae669cdba2 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 1e1e82c95f..530242dff9 100644 --- a/manila/tests/db/sqlalchemy/test_api.py +++ b/manila/tests/db/sqlalchemy/test_api.py @@ -473,6 +473,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.