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

manila/api/common.py

@@ -102,18 +102,9 @@ def _get_marker_param(request):
     return request.GET['marker']
 
 
-def limited(items, request, max_limit=CONF.osapi_max_limit):
+def _validate_pagination_query(request, max_limit=CONF.osapi_max_limit):
-    """Return a slice of items according to requested offset and 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:
@@ -134,6 +125,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]

manila/api/v1/shares.py

@@ -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,
+            shares = self._view_builder.detail_list(req, shares, total_count)
-                                                    total_count)
         else:
-            shares = self._view_builder.summary_list(req, limited_list,
+            shares = self._view_builder.summary_list(req, shares, total_count)
-                                                     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

manila/db/sqlalchemy/api.py

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

manila/share/api.py

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

manila/tests/api/v1/test_shares.py

@@ -249,6 +249,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'])
 
@@ -323,6 +324,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'])
 
@@ -367,6 +369,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'])
 
@@ -436,6 +439,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'])
 
@@ -581,7 +585,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)
 
@@ -595,7 +599,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']
@@ -679,7 +686,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)
 
@@ -693,6 +700,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'})

manila/tests/api/v2/test_shares.py

@@ -674,6 +674,7 @@ class ShareAPITest(test.TestCase):
         expected = self._get_expected_share_detailed_response(
             shr, version='2.7')
         self.assertDictMatch(expected, res_dict)
+        # pylint: disable=unsubscriptable-object
         self.assertEqual("fakenetid",
                          create_mock.call_args[1]['share_network_id'])
 
@@ -1253,6 +1254,7 @@ class ShareAPITest(test.TestCase):
         expected = self._get_expected_share_detailed_response(
             shr, version='2.7')
         self.assertEqual(expected, res_dict)
+        # pylint: disable=unsubscriptable-object
         self.assertEqual(parent_share_net,
                          create_mock.call_args[1]['share_network_id'])
 
@@ -1293,6 +1295,7 @@ class ShareAPITest(test.TestCase):
         expected = self._get_expected_share_detailed_response(
             shr, version='2.7')
         self.assertDictMatch(expected, res_dict)
+        # pylint: disable=unsubscriptable-object
         self.assertEqual(parent_share_net,
                          create_mock.call_args[1]['share_network_id'])
 
@@ -1590,7 +1593,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)
 
@@ -1604,6 +1607,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')):
@@ -1632,7 +1637,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',
@@ -1715,7 +1720,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)
 
@@ -1729,7 +1734,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'] = (
@@ -1767,7 +1775,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 = {

manila/tests/db/sqlalchemy/test_api.py

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

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