openstack/manila
Code Issues Proposed changes

Merge "Fix pagination does not speed up queries bug"

Browse Source
This commit is contained in:
Zuul 2019-09-26 16:50:53 +00:00 committed by Gerrit Code Review
commit b6b2eb5cf3
8 changed files with 76 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

30
manila/api/common.py
View File

@ -103,18 +103,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:
@ -135,6 +126,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]

14
manila/api/v1/shares.py
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'
)
@wsgi.Controller.authorize

4
manila/db/sqlalchemy/api.py
View File

@ -1911,6 +1911,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

4
manila/share/api.py
View File

@ -1670,6 +1670,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)

9
manila/tests/api/v1/test_shares.py
View File

@ -616,7 +616,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)
@ -630,7 +630,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']
@ -714,7 +717,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)
@ -728,6 +731,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'})

13
manila/tests/api/v2/test_shares.py
View File

@ -1599,7 +1599,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)
@ -1613,6 +1613,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')):
@ -1641,7 +1643,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'])
@ddt.data({'use_admin_context': True, 'version': '2.42'},
{'use_admin_context': False, 'version': '2.42'})
@ -1763,7 +1765,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)
@ -1777,7 +1779,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'] = (
@ -1815,7 +1820,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 = {

21
manila/tests/db/sqlalchemy/test_api.py
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)

7
releasenotes/notes/bug-1795463-fix-pagination-slowness-8fcda3746aa13940.yaml Normal file
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.