Merge "Fix pagination does not speed up queries bug" into stable/rocky

This commit is contained in:
Zuul 2019-10-18 02:04:33 +00:00 committed by Gerrit Code Review
commit 7914f90885
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

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

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

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

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.