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 57edcbd1da)
(cherry picked from commit 462b0e7461)
(cherry picked from commit bad03feea9)
This commit is contained in:
silvacarloss 2019-04-08 17:33:33 -03:00 committed by Goutham Pacha Ravi
parent c761bb559e
commit 8263f911be
8 changed files with 83 additions and 26 deletions

View File

@ -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]

View File

@ -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):

View File

@ -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

View File

@ -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)

View File

@ -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'})

View File

@ -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 = {

View File

@ -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)

View File

@ -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
<https://bugs.launchpad.net/manila/+bug/1795463>`_ for more details.