Browse Source

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

tags/6.3.2
Zuul 4 months ago
parent
commit
fc10c99ef1
8 changed files with 83 additions and 26 deletions
  1. +19
    -11
      manila/api/common.py
  2. +5
    -9
      manila/api/v1/shares.py
  3. +4
    -0
      manila/db/sqlalchemy/api.py
  4. +4
    -0
      manila/share/api.py
  5. +11
    -2
      manila/tests/api/v1/test_shares.py
  6. +12
    -4
      manila/tests/api/v2/test_shares.py
  7. +21
    -0
      manila/tests/db/sqlalchemy/test_api.py
  8. +7
    -0
      releasenotes/notes/bug-1795463-fix-pagination-slowness-8fcda3746aa13940.yaml

+ 19
- 11
manila/api/common.py 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]

+ 5
- 9
manila/api/v1/shares.py 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):

+ 4
- 0
manila/db/sqlalchemy/api.py 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

+ 4
- 0
manila/share/api.py 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)

+ 11
- 2
manila/tests/api/v1/test_shares.py 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'})

+ 12
- 4
manila/tests/api/v2/test_shares.py 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 = {

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

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

Loading…
Cancel
Save