From 28879f9a78020a64992f278edd8692ecbbd4a56c Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Wed, 12 Aug 2015 19:09:53 +0200 Subject: [PATCH] On Volume list only retrieve needed data from DB Currently when there is no limit set on a volume list query we retrieve all volumes and then limit them locally using osapi_max_limit. Similar thing happens when we are using the marker for next pages, we get all volumes from that marker until the last volume and then limit it locally. We should be limiting it on the DB side so we only retrieve the data we are actually going to return to the API caller. This patch always limits the data retrieved from the DB and for the offset to keep working as it was before we need to do the offset on the DB side as well. For reference some tests were performed: On a deployment with 60,000 volumes, 370,000 volume_metadata items and 240,000 volume_glance_metadata items in cinder db. Before the patch this will use nearly 10G memory. With the patch we will just use about 500M. Co-Authored-By: wangxiyuan Closes-bug:#1483165i Change-Id: Ie903e546074fe118299e8e1acfb9c88c8a10d78c --- cinder/api/common.py | 144 ++++++------- cinder/api/v2/views/volumes.py | 4 +- cinder/api/v2/volumes.py | 17 +- cinder/common/sqlalchemyutils.py | 5 +- cinder/db/api.py | 11 +- cinder/db/sqlalchemy/api.py | 15 +- cinder/tests/unit/api/test_common.py | 205 +++++++++---------- cinder/tests/unit/api/v1/test_volumes.py | 3 +- cinder/tests/unit/api/v2/stubs.py | 4 +- cinder/tests/unit/api/v2/test_volumes.py | 247 +++++++++++------------ cinder/volume/api.py | 9 +- 11 files changed, 313 insertions(+), 351 deletions(-) diff --git a/cinder/api/common.py b/cinder/api/common.py index 5395ca10876..06fee0008cc 100644 --- a/cinder/api/common.py +++ b/cinder/api/common.py @@ -69,42 +69,64 @@ def validate_key_names(key_names_list): return True -def get_pagination_params(request): - """Return marker, limit tuple from request. - - :param request: `wsgi.Request` possibly containing 'marker' and 'limit' - GET variables. 'marker' is the id of the last element - the client has seen, 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 marker or limit will cause - exc.HTTPBadRequest() exceptions to be raised. +def get_pagination_params(params, max_limit=CONF.osapi_max_limit): + """Return marker, limit, offset tuple from request. + :param params: `wsgi.Request`'s GET dictionary, possibly containing + 'marker', 'limit', and 'offset' variables. 'marker' is the + id of the last element the client has seen, 'limit' is the + maximum number of items to return and 'offset' is the number + of items to skip from the marker or from the first element. + If 'limit' is not specified, or > max_limit, we default to + max_limit. Negative values for either offset or limit will + cause exc.HTTPBadRequest() exceptions to be raised. If no + offset is present we'll default to 0 and if no marker is + present we'll default to None. + :max_limit: Max value 'limit' return value can take + :returns: Tuple (marker, limit, offset) """ - params = {} - if 'limit' in request.GET: - params['limit'] = _get_limit_param(request) - if 'marker' in request.GET: - params['marker'] = _get_marker_param(request) - return params + limit = _get_limit_param(params, max_limit) + marker = _get_marker_param(params) + offset = _get_offset_param(params) + return marker, limit, offset -def _get_limit_param(request): - """Extract integer limit from request or fail.""" +def _get_limit_param(params, max_limit=CONF.osapi_max_limit): + """Extract integer limit from request's dictionary or fail. + + Defaults to max_limit if not present and returns max_limit if present + 'limit' is greater than max_limit. + """ try: - limit = int(request.GET['limit']) + limit = int(params.pop('limit', max_limit)) except ValueError: msg = _('limit param must be an integer') raise webob.exc.HTTPBadRequest(explanation=msg) if limit < 0: msg = _('limit param must be positive') raise webob.exc.HTTPBadRequest(explanation=msg) + limit = min(limit, max_limit) return limit -def _get_marker_param(request): - """Extract marker id from request or fail.""" - return request.GET['marker'] +def _get_marker_param(params): + """Extract marker id from request's dictionary (defaults to None).""" + return params.pop('marker', None) + + +def _get_offset_param(params): + """Extract offset id from request's dictionary (defaults to 0) or fail.""" + try: + offset = int(params.pop('offset', 0)) + except ValueError: + msg = _('offset param must be an integer') + raise webob.exc.HTTPBadRequest(explanation=msg) + + if offset < 0: + msg = _('offset param must be positive') + raise webob.exc.HTTPBadRequest(explanation=msg) + + return offset def limited(items, request, max_limit=CONF.osapi_max_limit): @@ -119,39 +141,16 @@ def limited(items, request, max_limit=CONF.osapi_max_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: - msg = _('offset param must be an integer') - raise webob.exc.HTTPBadRequest(explanation=msg) - - try: - limit = int(request.GET.get('limit', max_limit)) - except ValueError: - msg = _('limit param must be an integer') - raise webob.exc.HTTPBadRequest(explanation=msg) - - if limit < 0: - msg = _('limit param must be positive') - raise webob.exc.HTTPBadRequest(explanation=msg) - - if offset < 0: - msg = _('offset param must be positive') - raise webob.exc.HTTPBadRequest(explanation=msg) - - limit = min(max_limit, limit or max_limit) - range_end = offset + limit + marker, limit, offset = get_pagination_params(request.GET.copy(), + max_limit) + range_end = offset + (limit or max_limit) return items[offset:range_end] def limited_by_marker(items, request, max_limit=CONF.osapi_max_limit): """Return a slice of items according to the requested marker and limit.""" - params = get_pagination_params(request) + marker, limit, __ = get_pagination_params(request.GET.copy(), max_limit) - limit = params.get('limit', max_limit) - marker = params.get('marker') - - limit = min(max_limit, limit) start_index = 0 if marker: start_index = -1 @@ -289,19 +288,19 @@ class ViewBuilder(object): str(identifier)) def _get_collection_links(self, request, items, collection_name, - item_count, id_key="uuid"): + item_count=None, id_key="uuid"): """Retrieve 'next' link, if applicable. - The next link is included if: - 1) 'limit' param is specified and equal to or less than the - number of items. - 2) 'limit' param is specified and both the limit and the number - of items are greater than CONF.osapi_max_limit, even if limit - is greater than the number of items. - 3) 'limit' param is NOT specified and the number of items is - greater than CONF.osapi_max_limit. - Notes: The case limit equals to 0 or CONF.osapi_max_limit equals to 0 - is not included in the above conditions. + The next link is included if we are returning as many items as we can, + given the restrictions of limit optional request parameter and + osapi_max_limit configuration parameter as long as we are returning + some elements. + + So we return next link if: + + 1) 'limit' param is specified and equal to the number of items. + 2) 'limit' param is NOT specified and the number of items is + equal to CONF.osapi_max_limit. :param request: API request :param items: List of collection items @@ -313,25 +312,12 @@ class ViewBuilder(object): to generate the next link marker for a pagination query :returns links """ - limit = request.params.get("limit", None) - if limit is None or int(limit) > CONF.osapi_max_limit: - # If limit is not set in the request or greater than - # osapi_max_limit, len(items) < item_count means the items - # are limited by osapi_max_limit and we need to generate the - # next link. Otherwise, all the items will be returned in - # the response, no next link is generated. - if len(items) < item_count: - return self._generate_next_link(items, id_key, request, - collection_name) - else: - # If limit is set in the request and not more than - # osapi_max_limit, int(limit) == len(items) means it is possible - # that the DB still have more items left. In this case, - # we generate the next link. - limit = int(limit) - if limit and limit == len(items): - return self._generate_next_link(items, id_key, request, - collection_name) + item_count = item_count or len(items) + limit = _get_limit_param(request.GET.copy()) + if len(items) and limit <= item_count: + return self._generate_next_link(items, id_key, request, + collection_name) + return [] def _generate_next_link(self, items, id_key, request, diff --git a/cinder/api/v2/views/volumes.py b/cinder/api/v2/views/volumes.py index df453fd8b35..d428827d7de 100644 --- a/cinder/api/v2/views/volumes.py +++ b/cinder/api/v2/views/volumes.py @@ -30,12 +30,12 @@ class ViewBuilder(common.ViewBuilder): """Initialize view builder.""" super(ViewBuilder, self).__init__() - def summary_list(self, request, volumes, volume_count): + def summary_list(self, request, volumes, volume_count=None): """Show a list of volumes without many details.""" return self._list_view(self.summary, request, volumes, volume_count) - def detail_list(self, request, volumes, volume_count): + def detail_list(self, request, volumes, volume_count=None): """Detailed view of a list of volumes.""" return self._list_view(self.detail, request, volumes, volume_count, diff --git a/cinder/api/v2/volumes.py b/cinder/api/v2/volumes.py index 852515cc012..5287d54bd4f 100644 --- a/cinder/api/v2/volumes.py +++ b/cinder/api/v2/volumes.py @@ -227,10 +227,8 @@ class VolumeController(wsgi.Controller): context = req.environ['cinder.context'] params = req.params.copy() - marker = params.pop('marker', None) - limit = params.pop('limit', None) + marker, limit, offset = common.get_pagination_params(params) sort_keys, sort_dirs = common.get_sort_params(params) - params.pop('offset', None) filters = params utils.remove_invalid_filter_options(context, @@ -255,23 +253,20 @@ class VolumeController(wsgi.Controller): sort_keys=sort_keys, sort_dirs=sort_dirs, filters=filters, - viewable_admin_meta=True) + viewable_admin_meta=True, + offset=offset) volumes = [dict(vol) for vol in volumes] for volume in volumes: utils.add_visible_admin_metadata(volume) - limited_list = common.limited(volumes, req) - volume_count = len(volumes) - req.cache_db_volumes(limited_list) + req.cache_db_volumes(volumes) if is_detail: - volumes = self._view_builder.detail_list(req, limited_list, - volume_count) + volumes = self._view_builder.detail_list(req, volumes) else: - volumes = self._view_builder.summary_list(req, limited_list, - volume_count) + volumes = self._view_builder.summary_list(req, volumes) return volumes def _image_uuid_from_ref(self, image_ref, context): diff --git a/cinder/common/sqlalchemyutils.py b/cinder/common/sqlalchemyutils.py index 4ff438ce365..53faf927793 100644 --- a/cinder/common/sqlalchemyutils.py +++ b/cinder/common/sqlalchemyutils.py @@ -31,7 +31,7 @@ LOG = logging.getLogger(__name__) # copied from glance/db/sqlalchemy/api.py def paginate_query(query, model, limit, sort_keys, marker=None, - sort_dir=None, sort_dirs=None): + sort_dir=None, sort_dirs=None, offset=None): """Returns a query with sorting / pagination criteria added. Pagination works by requiring a unique sort_key, specified by sort_keys. @@ -125,4 +125,7 @@ def paginate_query(query, model, limit, sort_keys, marker=None, if limit is not None: query = query.limit(limit) + if offset: + query = query.offset(offset) + return query diff --git a/cinder/db/api.py b/cinder/db/api.py index 69a54a091ca..4e7e6969f04 100644 --- a/cinder/db/api.py +++ b/cinder/db/api.py @@ -206,10 +206,11 @@ def volume_get(context, volume_id): def volume_get_all(context, marker, limit, sort_keys=None, sort_dirs=None, - filters=None): + filters=None, offset=None): """Get all volumes.""" return IMPL.volume_get_all(context, marker, limit, sort_keys=sort_keys, - sort_dirs=sort_dirs, filters=filters) + sort_dirs=sort_dirs, filters=filters, + offset=offset) def volume_get_all_by_host(context, host, filters=None): @@ -223,12 +224,14 @@ def volume_get_all_by_group(context, group_id, filters=None): def volume_get_all_by_project(context, project_id, marker, limit, - sort_keys=None, sort_dirs=None, filters=None): + sort_keys=None, sort_dirs=None, filters=None, + offset=None): """Get all volumes belonging to a project.""" return IMPL.volume_get_all_by_project(context, project_id, marker, limit, sort_keys=sort_keys, sort_dirs=sort_dirs, - filters=filters) + filters=filters, + offset=offset) def volume_get_iscsi_target_num(context, volume_id): diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index da6f1494093..e331e427606 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -1339,7 +1339,7 @@ def volume_get(context, volume_id): @require_admin_context def volume_get_all(context, marker, limit, sort_keys=None, sort_dirs=None, - filters=None): + filters=None, offset=None): """Retrieves all volumes. If no sort parameters are specified then the returned volumes are sorted @@ -1364,7 +1364,7 @@ def volume_get_all(context, marker, limit, sort_keys=None, sort_dirs=None, with session.begin(): # Generate the query query = _generate_paginate_query(context, session, marker, limit, - sort_keys, sort_dirs, filters) + sort_keys, sort_dirs, filters, offset) # No volumes would match, return empty list if query is None: return [] @@ -1429,7 +1429,8 @@ def volume_get_all_by_group(context, group_id, filters=None): @require_context def volume_get_all_by_project(context, project_id, marker, limit, - sort_keys=None, sort_dirs=None, filters=None): + sort_keys=None, sort_dirs=None, filters=None, + offset=None): """Retrieves all volumes in a project. If no sort parameters are specified then the returned volumes are sorted @@ -1459,7 +1460,7 @@ def volume_get_all_by_project(context, project_id, marker, limit, filters['project_id'] = project_id # Generate the query query = _generate_paginate_query(context, session, marker, limit, - sort_keys, sort_dirs, filters) + sort_keys, sort_dirs, filters, offset) # No volumes would match, return empty list if query is None: return [] @@ -1467,7 +1468,7 @@ def volume_get_all_by_project(context, project_id, marker, limit, def _generate_paginate_query(context, session, marker, limit, sort_keys, - sort_dirs, filters): + sort_dirs, filters, offset=None): """Generate the query to include the filters and the paginate options. Returns a query with sorting / pagination criteria added or None @@ -1486,6 +1487,7 @@ def _generate_paginate_query(context, session, marker, limit, sort_keys, or sets cause an 'IN' operation, while exact matching is used for other values, see _process_volume_filters function for more information + :param offset: number of items to skip :returns: updated query or None """ sort_keys, sort_dirs = process_sort_params(sort_keys, @@ -1505,7 +1507,8 @@ def _generate_paginate_query(context, session, marker, limit, sort_keys, return sqlalchemyutils.paginate_query(query, models.Volume, limit, sort_keys, marker=marker_volume, - sort_dirs=sort_dirs) + sort_dirs=sort_dirs, + offset=offset) def _process_volume_filters(query, filters): diff --git a/cinder/tests/unit/api/test_common.py b/cinder/tests/unit/api/test_common.py index daddc7ac5b9..5b31b538b8a 100644 --- a/cinder/tests/unit/api/test_common.py +++ b/cinder/tests/unit/api/test_common.py @@ -22,12 +22,15 @@ from testtools import matchers import webob import webob.exc +from oslo_config import cfg + from cinder.api import common from cinder import test NS = "{http://docs.openstack.org/compute/api/v1.1}" ATOMNS = "{http://www.w3.org/2005/Atom}" +CONF = cfg.CONF class LimiterTest(test.TestCase): @@ -172,37 +175,45 @@ class PaginationParamsTest(test.TestCase): """Test nonnumerical limit param.""" req = webob.Request.blank('/?limit=hello') self.assertRaises( - webob.exc.HTTPBadRequest, common.get_pagination_params, req) + webob.exc.HTTPBadRequest, common.get_pagination_params, + req.GET.copy()) def test_no_params(self): """Test no params.""" req = webob.Request.blank('/') - self.assertEqual({}, common.get_pagination_params(req)) + expected = (None, CONF.osapi_max_limit, 0) + self.assertEqual(expected, + common.get_pagination_params(req.GET.copy())) def test_valid_marker(self): """Test valid marker param.""" - req = webob.Request.blank( - '/?marker=263abb28-1de6-412f-b00b-f0ee0c4333c2') - self.assertEqual({'marker': '263abb28-1de6-412f-b00b-f0ee0c4333c2'}, - common.get_pagination_params(req)) + marker = '263abb28-1de6-412f-b00b-f0ee0c4333c2' + req = webob.Request.blank('/?marker=' + marker) + expected = (marker, CONF.osapi_max_limit, 0) + self.assertEqual(expected, + common.get_pagination_params(req.GET.copy())) def test_valid_limit(self): """Test valid limit param.""" req = webob.Request.blank('/?limit=10') - self.assertEqual({'limit': 10}, common.get_pagination_params(req)) + expected = (None, 10, 0) + self.assertEqual(expected, + common.get_pagination_params(req.GET.copy())) def test_invalid_limit(self): """Test invalid limit param.""" req = webob.Request.blank('/?limit=-2') self.assertRaises( - webob.exc.HTTPBadRequest, common.get_pagination_params, req) + webob.exc.HTTPBadRequest, common.get_pagination_params, + req.GET.copy()) def test_valid_limit_and_marker(self): """Test valid limit and marker parameters.""" marker = '263abb28-1de6-412f-b00b-f0ee0c4333c2' req = webob.Request.blank('/?limit=20&marker=%s' % marker) - self.assertEqual({'marker': marker, 'limit': 20}, - common.get_pagination_params(req)) + expected = (marker, 20, 0) + self.assertEqual(expected, + common.get_pagination_params(req.GET.copy())) class SortParamUtilsTest(test.TestCase): @@ -354,25 +365,35 @@ class MiscFunctionsTest(test.TestCase): class TestCollectionLinks(test.TestCase): """Tests the _get_collection_links method.""" - def _validate_next_link(self, href_link_mock, item_count, - osapi_max_limit, limit, should_link_exist): - req = mock.MagicMock() - href_link_mock.return_value = [{"rel": "next", - "href": "fake_link"}] + def _validate_next_link(self, item_count, osapi_max_limit, limit, + should_link_exist): + req = webob.Request.blank('/?limit=%s' % limit if limit else '/') + link_return = [{"rel": "next", "href": "fake_link"}] self.flags(osapi_max_limit=osapi_max_limit) if limit is None: - params = mock.PropertyMock(return_value=dict()) limited_list_size = min(item_count, osapi_max_limit) else: - params = mock.PropertyMock(return_value=dict(limit=limit)) - limited_list_size = min(item_count, osapi_max_limit, - limit) + limited_list_size = min(item_count, osapi_max_limit, limit) limited_list = [{"uuid": str(i)} for i in range(limited_list_size)] - type(req).params = params builder = common.ViewBuilder() - results = builder._get_collection_links(req, limited_list, - mock.sentinel.coll_key, - item_count, "uuid") + + def get_pagination_params(params, max_limit=CONF.osapi_max_limit, + original_call=common.get_pagination_params): + return original_call(params, max_limit) + + def _get_limit_param(params, max_limit=CONF.osapi_max_limit, + original_call=common._get_limit_param): + return original_call(params, max_limit) + + with mock.patch.object(common, 'get_pagination_params', + get_pagination_params), \ + mock.patch.object(common, '_get_limit_param', + _get_limit_param), \ + mock.patch.object(common.ViewBuilder, '_generate_next_link', + return_value=link_return) as href_link_mock: + results = builder._get_collection_links(req, limited_list, + mock.sentinel.coll_key, + item_count, "uuid") if should_link_exist: href_link_mock.assert_called_once_with(limited_list, "uuid", req, @@ -382,171 +403,133 @@ class TestCollectionLinks(test.TestCase): self.assertFalse(href_link_mock.called) self.assertThat(results, matchers.HasLength(0)) - @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') - def test_items_equals_osapi_max_no_limit(self, href_link_mock): + def test_items_equals_osapi_max_no_limit(self): item_count = 5 osapi_max_limit = 5 limit = None - should_link_exist = False - self._validate_next_link(href_link_mock, item_count, - osapi_max_limit, - limit, should_link_exist) + should_link_exist = True + self._validate_next_link(item_count, osapi_max_limit, limit, + should_link_exist) - @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') - def test_items_equals_osapi_max_greater_than_limit(self, - href_link_mock): + def test_items_equals_osapi_max_greater_than_limit(self): item_count = 5 osapi_max_limit = 5 limit = 4 should_link_exist = True - self._validate_next_link(href_link_mock, item_count, - osapi_max_limit, - limit, should_link_exist) + self._validate_next_link(item_count, osapi_max_limit, limit, + should_link_exist) - @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') - def test_items_equals_osapi_max_equals_limit(self, href_link_mock): + def test_items_equals_osapi_max_equals_limit(self): item_count = 5 osapi_max_limit = 5 limit = 5 should_link_exist = True - self._validate_next_link(href_link_mock, item_count, - osapi_max_limit, - limit, should_link_exist) + self._validate_next_link(item_count, osapi_max_limit, limit, + should_link_exist) - @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') - def test_items_equals_osapi_max_less_than_limit(self, href_link_mock): + def test_items_equals_osapi_max_less_than_limit(self): item_count = 5 osapi_max_limit = 5 limit = 6 - should_link_exist = False - self._validate_next_link(href_link_mock, item_count, - osapi_max_limit, - limit, should_link_exist) + should_link_exist = True + self._validate_next_link(item_count, osapi_max_limit, limit, + should_link_exist) - @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') - def test_items_less_than_osapi_max_no_limit(self, href_link_mock): + def test_items_less_than_osapi_max_no_limit(self): item_count = 5 osapi_max_limit = 7 limit = None should_link_exist = False - self._validate_next_link(href_link_mock, item_count, - osapi_max_limit, - limit, should_link_exist) + self._validate_next_link(item_count, osapi_max_limit, limit, + should_link_exist) - @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') - def test_limit_less_than_items_less_than_osapi_max(self, href_link_mock): + def test_limit_less_than_items_less_than_osapi_max(self): item_count = 5 osapi_max_limit = 7 limit = 4 should_link_exist = True - self._validate_next_link(href_link_mock, item_count, - osapi_max_limit, - limit, should_link_exist) + self._validate_next_link(item_count, osapi_max_limit, limit, + should_link_exist) - @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') - def test_limit_equals_items_less_than_osapi_max(self, href_link_mock): + def test_limit_equals_items_less_than_osapi_max(self): item_count = 5 osapi_max_limit = 7 limit = 5 should_link_exist = True - self._validate_next_link(href_link_mock, item_count, - osapi_max_limit, - limit, should_link_exist) + self._validate_next_link(item_count, osapi_max_limit, limit, + should_link_exist) - @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') - def test_items_less_than_limit_less_than_osapi_max(self, href_link_mock): + def test_items_less_than_limit_less_than_osapi_max(self): item_count = 5 osapi_max_limit = 7 limit = 6 should_link_exist = False - self._validate_next_link(href_link_mock, item_count, - osapi_max_limit, - limit, should_link_exist) + self._validate_next_link(item_count, osapi_max_limit, limit, + should_link_exist) - @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') - def test_items_less_than_osapi_max_equals_limit(self, href_link_mock): + def test_items_less_than_osapi_max_equals_limit(self): item_count = 5 osapi_max_limit = 7 limit = 7 should_link_exist = False - self._validate_next_link(href_link_mock, item_count, - osapi_max_limit, - limit, should_link_exist) + self._validate_next_link(item_count, osapi_max_limit, limit, + should_link_exist) - @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') - def test_items_less_than_osapi_max_less_than_limit(self, href_link_mock): + def test_items_less_than_osapi_max_less_than_limit(self): item_count = 5 osapi_max_limit = 7 limit = 8 should_link_exist = False - self._validate_next_link(href_link_mock, item_count, - osapi_max_limit, - limit, should_link_exist) + self._validate_next_link(item_count, osapi_max_limit, limit, + should_link_exist) - @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') - def test_items_greater_than_osapi_max_no_limit(self, href_link_mock): + def test_items_greater_than_osapi_max_no_limit(self): item_count = 5 osapi_max_limit = 3 limit = None should_link_exist = True - self._validate_next_link(href_link_mock, item_count, - osapi_max_limit, - limit, should_link_exist) + self._validate_next_link(item_count, osapi_max_limit, limit, + should_link_exist) - @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') - def test_limit_less_than_items_greater_than_osapi_max(self, - href_link_mock): + def test_limit_less_than_items_greater_than_osapi_max(self): item_count = 5 osapi_max_limit = 3 limit = 2 should_link_exist = True - self._validate_next_link(href_link_mock, item_count, - osapi_max_limit, - limit, should_link_exist) + self._validate_next_link(item_count, osapi_max_limit, limit, + should_link_exist) - @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') - def test_items_greater_than_osapi_max_equals_limit(self, - href_link_mock): + def test_items_greater_than_osapi_max_equals_limit(self): item_count = 5 osapi_max_limit = 3 limit = 3 should_link_exist = True - self._validate_next_link(href_link_mock, item_count, - osapi_max_limit, - limit, should_link_exist) + self._validate_next_link(item_count, osapi_max_limit, limit, + should_link_exist) - @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') - def test_items_greater_than_limit_greater_than_osapi_max(self, - href_link_mock): + def test_items_greater_than_limit_greater_than_osapi_max(self): item_count = 5 osapi_max_limit = 3 limit = 4 should_link_exist = True - self._validate_next_link(href_link_mock, item_count, - osapi_max_limit, - limit, should_link_exist) + self._validate_next_link(item_count, osapi_max_limit, limit, + should_link_exist) - @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') - def test_items_equals_limit_greater_than_osapi_max(self, - href_link_mock): + def test_items_equals_limit_greater_than_osapi_max(self): item_count = 5 osapi_max_limit = 3 limit = 5 should_link_exist = True - self._validate_next_link(href_link_mock, item_count, - osapi_max_limit, - limit, should_link_exist) + self._validate_next_link(item_count, osapi_max_limit, limit, + should_link_exist) - @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') - def test_limit_greater_than_items_greater_than_osapi_max(self, - href_link_mock): + def test_limit_greater_than_items_greater_than_osapi_max(self): item_count = 5 osapi_max_limit = 3 limit = 6 should_link_exist = True - self._validate_next_link(href_link_mock, item_count, - osapi_max_limit, - limit, should_link_exist) + self._validate_next_link(item_count, osapi_max_limit, limit, + should_link_exist) class LinkPrefixTest(test.TestCase): diff --git a/cinder/tests/unit/api/v1/test_volumes.py b/cinder/tests/unit/api/v1/test_volumes.py index 40b051aa7dc..1dc435466ef 100644 --- a/cinder/tests/unit/api/v1/test_volumes.py +++ b/cinder/tests/unit/api/v1/test_volumes.py @@ -620,7 +620,8 @@ class VolumeApiTest(test.TestCase): def stub_volume_get_all_by_project(context, project_id, marker, limit, sort_keys=None, sort_dirs=None, filters=None, - viewable_admin_meta=False): + viewable_admin_meta=False, + offset=None): return [ stubs.stub_volume(1, display_name='vol1'), stubs.stub_volume(2, display_name='vol2'), diff --git a/cinder/tests/unit/api/v2/stubs.py b/cinder/tests/unit/api/v2/stubs.py index 89235b33da7..ef4184fdb90 100644 --- a/cinder/tests/unit/api/v2/stubs.py +++ b/cinder/tests/unit/api/v2/stubs.py @@ -139,7 +139,7 @@ def stub_volume_get_db(context, volume_id): def stub_volume_get_all(context, search_opts=None, marker=None, limit=None, sort_keys=None, sort_dirs=None, filters=None, - viewable_admin_meta=False): + viewable_admin_meta=False, offset=None): return [stub_volume(100, project_id='fake'), stub_volume(101, project_id='superfake'), stub_volume(102, project_id='superduperfake')] @@ -148,7 +148,7 @@ def stub_volume_get_all(context, search_opts=None, marker=None, limit=None, def stub_volume_get_all_by_project(self, context, marker, limit, sort_keys=None, sort_dirs=None, filters=None, - viewable_admin_meta=False): + viewable_admin_meta=False, offset=None): filters = filters or {} return [stub_volume_get(self, context, '1')] diff --git a/cinder/tests/unit/api/v2/test_volumes.py b/cinder/tests/unit/api/v2/test_volumes.py index 4e272c5b46e..69e1422a5a9 100644 --- a/cinder/tests/unit/api/v2/test_volumes.py +++ b/cinder/tests/unit/api/v2/test_volumes.py @@ -25,6 +25,7 @@ from six.moves import range from six.moves import urllib import webob +from cinder.api import common from cinder.api import extensions from cinder.api.v2 import volumes from cinder import consistencygroup as consistencygroupAPI @@ -36,6 +37,7 @@ from cinder.tests.unit.api import fakes from cinder.tests.unit.api.v2 import stubs from cinder.tests.unit import fake_notifier from cinder.tests.unit.image import fake as fake_image +from cinder.tests.unit import utils from cinder.volume import api as volume_api CONF = cfg.CONF @@ -60,6 +62,7 @@ class VolumeApiTest(test.TestCase): self.stubs.Set(db, 'service_get_all_by_topic', stubs.stub_service_get_all_by_topic) self.maxDiff = None + self.ctxt = context.RequestContext('admin', 'fakeproject', True) def test_volume_create(self): self.stubs.Set(volume_api.API, 'get', stubs.stub_volume_get) @@ -731,7 +734,8 @@ class VolumeApiTest(test.TestCase): def stub_volume_get_all_by_project(context, project_id, marker, limit, sort_keys=None, sort_dirs=None, filters=None, - viewable_admin_meta=False): + viewable_admin_meta=False, + offset=0): return [ stubs.stub_volume(1, display_name='vol1'), stubs.stub_volume(2, display_name='vol2'), @@ -776,13 +780,13 @@ class VolumeApiTest(test.TestCase): def test_volume_index_limit_negative(self): req = fakes.HTTPRequest.blank('/v2/volumes?limit=-1') - self.assertRaises(exception.Invalid, + self.assertRaises(webob.exc.HTTPBadRequest, self.controller.index, req) def test_volume_index_limit_non_int(self): req = fakes.HTTPRequest.blank('/v2/volumes?limit=a') - self.assertRaises(exception.Invalid, + self.assertRaises(webob.exc.HTTPBadRequest, self.controller.index, req) @@ -797,32 +801,29 @@ class VolumeApiTest(test.TestCase): self.assertEqual(1, len(volumes)) self.assertEqual('1', volumes[0]['id']) - def test_volume_index_limit_offset(self): - def stub_volume_get_all_by_project(context, project_id, marker, limit, - sort_keys=None, sort_dirs=None, - filters=None, - viewable_admin_meta=False): - return [ - stubs.stub_volume(1, display_name='vol1'), - stubs.stub_volume(2, display_name='vol2'), - ] - self.stubs.Set(db, 'volume_get_all_by_project', - stub_volume_get_all_by_project) - self.stubs.Set(volume_api.API, 'get', stubs.stub_volume_get) + def _create_db_volumes(self, num_volumes): + volumes = [utils.create_volume(self.ctxt, display_name='vol%s' % i) + for i in range(num_volumes)] + for vol in volumes: + self.addCleanup(db.volume_destroy, self.ctxt, vol.id) + volumes.reverse() + return volumes + def test_volume_index_limit_offset(self): + created_volumes = self._create_db_volumes(2) req = fakes.HTTPRequest.blank('/v2/volumes?limit=2&offset=1') res_dict = self.controller.index(req) volumes = res_dict['volumes'] self.assertEqual(1, len(volumes)) - self.assertEqual(2, volumes[0]['id']) + self.assertEqual(created_volumes[1].id, volumes[0]['id']) req = fakes.HTTPRequest.blank('/v2/volumes?limit=-1&offset=1') - self.assertRaises(exception.InvalidInput, + self.assertRaises(webob.exc.HTTPBadRequest, self.controller.index, req) req = fakes.HTTPRequest.blank('/v2/volumes?limit=a&offset=1') - self.assertRaises(exception.InvalidInput, + self.assertRaises(webob.exc.HTTPBadRequest, self.controller.index, req) @@ -830,7 +831,8 @@ class VolumeApiTest(test.TestCase): def stub_volume_get_all_by_project(context, project_id, marker, limit, sort_keys=None, sort_dirs=None, filters=None, - viewable_admin_meta=False): + viewable_admin_meta=False, + offset=0): return [ stubs.stub_volume(1, display_name='vol1'), stubs.stub_volume(2, display_name='vol2'), @@ -867,13 +869,13 @@ class VolumeApiTest(test.TestCase): def test_volume_detail_limit_negative(self): req = fakes.HTTPRequest.blank('/v2/volumes/detail?limit=-1') - self.assertRaises(exception.Invalid, + self.assertRaises(webob.exc.HTTPBadRequest, self.controller.detail, req) def test_volume_detail_limit_non_int(self): req = fakes.HTTPRequest.blank('/v2/volumes/detail?limit=a') - self.assertRaises(exception.Invalid, + self.assertRaises(webob.exc.HTTPBadRequest, self.controller.detail, req) @@ -889,38 +891,27 @@ class VolumeApiTest(test.TestCase): self.assertEqual('1', volumes[0]['id']) def test_volume_detail_limit_offset(self): - def stub_volume_get_all_by_project(context, project_id, marker, limit, - sort_keys=None, sort_dirs=None, - filters=None, - viewable_admin_meta=False): - return [ - stubs.stub_volume(1, display_name='vol1'), - stubs.stub_volume(2, display_name='vol2'), - ] - self.stubs.Set(db, 'volume_get_all_by_project', - stub_volume_get_all_by_project) - self.stubs.Set(volume_api.API, 'get', stubs.stub_volume_get) - + created_volumes = self._create_db_volumes(2) req = fakes.HTTPRequest.blank('/v2/volumes/detail?limit=2&offset=1') res_dict = self.controller.detail(req) volumes = res_dict['volumes'] self.assertEqual(1, len(volumes)) - self.assertEqual(2, volumes[0]['id']) + self.assertEqual(created_volumes[1].id, volumes[0]['id']) req = fakes.HTTPRequest.blank('/v2/volumes/detail?limit=2&offset=1', use_admin_context=True) res_dict = self.controller.detail(req) volumes = res_dict['volumes'] self.assertEqual(1, len(volumes)) - self.assertEqual(2, volumes[0]['id']) + self.assertEqual(created_volumes[1].id, volumes[0]['id']) req = fakes.HTTPRequest.blank('/v2/volumes/detail?limit=-1&offset=1') - self.assertRaises(exception.InvalidInput, + self.assertRaises(webob.exc.HTTPBadRequest, self.controller.detail, req) req = fakes.HTTPRequest.blank('/v2/volumes/detail?limit=a&offset=1') - self.assertRaises(exception.InvalidInput, + self.assertRaises(webob.exc.HTTPBadRequest, self.controller.detail, req) @@ -933,84 +924,71 @@ class VolumeApiTest(test.TestCase): expected = {'volumes': []} self.assertEqual(expected, res_dict) - def test_volume_default_limit(self): - self.stubs.Set(volume_api.API, 'get', stubs.stub_volume_get) + def _validate_next_link(self, detailed, item_count, osapi_max_limit, limit, + should_link_exist): + keys_fns = (('volumes', self.controller.index), + ('volumes/detail', self.controller.detail)) + key, fn = keys_fns[detailed] - def _verify_links(links, url_key): - """Verify next link and url.""" - self.assertEqual('next', links[0]['rel']) - href_parts = urllib.parse.urlparse(links[0]['href']) - self.assertEqual('/v2/fakeproject/%s' % key, href_parts.path) + req_string = '/v2/%s?all_tenants=1' % key + if limit: + req_string += '&limit=%s' % limit + req = fakes.HTTPRequest.blank(req_string, use_admin_context=True) + + link_return = [{"rel": "next", "href": "fake_link"}] + self.flags(osapi_max_limit=osapi_max_limit) + + def get_pagination_params(params, max_limit=CONF.osapi_max_limit, + original_call=common.get_pagination_params): + return original_call(params, max_limit) + + def _get_limit_param(params, max_limit=CONF.osapi_max_limit, + original_call=common._get_limit_param): + return original_call(params, max_limit) + + with mock.patch.object(common, 'get_pagination_params', + get_pagination_params), \ + mock.patch.object(common, '_get_limit_param', + _get_limit_param), \ + mock.patch.object(common.ViewBuilder, '_generate_next_link', + return_value=link_return): + res_dict = fn(req) + self.assertEqual(item_count, len(res_dict['volumes'])) + self.assertEqual(should_link_exist, 'volumes_links' in res_dict) + + def test_volume_default_limit(self): + self.stubs.UnsetAll() + self._create_db_volumes(3) # Verify both the index and detail queries - api_keys = ['volumes', 'volumes/detail'] - fns = [self.controller.index, self.controller.detail] + for detailed in (True, False): + # Number of volumes less than max, do not include + self._validate_next_link(detailed, item_count=3, osapi_max_limit=4, + limit=None, should_link_exist=False) - # Number of volumes equals the max, next link not included - def stub_volume_get_all(context, marker, limit, - sort_keys=None, sort_dirs=None, - filters=None, - viewable_admin_meta=False): - vols = [stubs.stub_volume(i) - for i in range(CONF.osapi_max_limit)] - if limit is None or limit >= len(vols): - return vols - return vols[:limit] - self.stubs.Set(db, 'volume_get_all', stub_volume_get_all) - for key, fn in zip(api_keys, fns): - req = fakes.HTTPRequest.blank('/v2/%s?all_tenants=1' % key, - use_admin_context=True) - res_dict = fn(req) - self.assertEqual(len(res_dict['volumes']), CONF.osapi_max_limit) - self.assertFalse('volumes_links' in res_dict) + # Number of volumes equals the max, next link will be included + self._validate_next_link(detailed, item_count=3, osapi_max_limit=3, + limit=None, should_link_exist=True) - # Number of volumes less than max, do not include - def stub_volume_get_all2(context, marker, limit, - sort_keys=None, sort_dirs=None, - filters=None, - viewable_admin_meta=False): - vols = [stubs.stub_volume(i) - for i in range(100)] - if limit is None or limit >= len(vols): - return vols - return vols[:limit] - self.stubs.Set(db, 'volume_get_all', stub_volume_get_all2) - for key, fn in zip(api_keys, fns): - req = fakes.HTTPRequest.blank('/v2/%s?all_tenants=1' % key, - use_admin_context=True) - res_dict = fn(req) - self.assertEqual(100, len(res_dict['volumes'])) - self.assertFalse('volumes_links' in res_dict) + # Number of volumes more than the max, include next link + self._validate_next_link(detailed, item_count=2, osapi_max_limit=2, + limit=None, should_link_exist=True) - # Number of volumes more than the max, include next link - def stub_volume_get_all3(context, marker, limit, - sort_keys=None, sort_dirs=None, - filters=None, - viewable_admin_meta=False): - vols = [stubs.stub_volume(i) - for i in range(CONF.osapi_max_limit + 100)] - if limit is None or limit >= len(vols): - return vols - return vols[:limit] - self.stubs.Set(db, 'volume_get_all', stub_volume_get_all3) - for key, fn in zip(api_keys, fns): - req = fakes.HTTPRequest.blank('/v2/%s?all_tenants=1' % key, - use_admin_context=True) - res_dict = fn(req) - self.assertEqual(CONF.osapi_max_limit, len(res_dict['volumes'])) - volumes_links = res_dict['volumes_links'] - _verify_links(volumes_links, key) - # Pass a limit that is greater than the max and the total number of - # volumes, ensure only the maximum is returned and that the next - # link is present. - for key, fn in zip(api_keys, fns): - req = fakes.HTTPRequest.blank('/v2/%s?all_tenants=1&limit=%d' - % (key, CONF.osapi_max_limit * 2), - use_admin_context=True) - res_dict = fn(req) - self.assertEqual(CONF.osapi_max_limit, len(res_dict['volumes'])) - volumes_links = res_dict['volumes_links'] - _verify_links(volumes_links, key) + # Limit lower than max but doesn't limit, no next link + self._validate_next_link(detailed, item_count=3, osapi_max_limit=5, + limit=4, should_link_exist=False) + + # Limit lower than max and limits, we have next link + self._validate_next_link(detailed, item_count=2, osapi_max_limit=4, + limit=2, should_link_exist=True) + + # Limit higher than max and max limits, we have next link + self._validate_next_link(detailed, item_count=2, osapi_max_limit=2, + limit=4, should_link_exist=True) + + # Limit higher than max but none of them limiting, no next link + self._validate_next_link(detailed, item_count=3, osapi_max_limit=4, + limit=5, should_link_exist=False) def test_volume_list_default_filters(self): """Tests that the default filters from volume.api.API.get_all are set. @@ -1027,7 +1005,8 @@ class VolumeApiTest(test.TestCase): def stub_volume_get_all_by_project(context, project_id, marker, limit, sort_keys=None, sort_dirs=None, filters=None, - viewable_admin_meta=False): + viewable_admin_meta=False, + offset=0): self.assertEqual(True, filters['no_migration_targets']) self.assertFalse('all_tenants' in filters) return [stubs.stub_volume(1, display_name='vol1')] @@ -1035,7 +1014,7 @@ class VolumeApiTest(test.TestCase): def stub_volume_get_all(context, marker, limit, sort_keys=None, sort_dirs=None, filters=None, - viewable_admin_meta=False): + viewable_admin_meta=False, offset=0): return [] self.stubs.Set(db, 'volume_get_all_by_project', stub_volume_get_all_by_project) @@ -1053,14 +1032,15 @@ class VolumeApiTest(test.TestCase): def stub_volume_get_all_by_project2(context, project_id, marker, limit, sort_keys=None, sort_dirs=None, filters=None, - viewable_admin_meta=False): + viewable_admin_meta=False, + offset=0): self.assertFalse('no_migration_targets' in filters) return [stubs.stub_volume(1, display_name='vol2')] def stub_volume_get_all2(context, marker, limit, sort_keys=None, sort_dirs=None, filters=None, - viewable_admin_meta=False): + viewable_admin_meta=False, offset=0): return [] self.stubs.Set(db, 'volume_get_all_by_project', stub_volume_get_all_by_project2) @@ -1076,13 +1056,14 @@ class VolumeApiTest(test.TestCase): def stub_volume_get_all_by_project3(context, project_id, marker, limit, sort_keys=None, sort_dirs=None, filters=None, - viewable_admin_meta=False): + viewable_admin_meta=False, + offset=0): return [] def stub_volume_get_all3(context, marker, limit, sort_keys=None, sort_dirs=None, filters=None, - viewable_admin_meta=False): + viewable_admin_meta=False, offset=0): self.assertFalse('no_migration_targets' in filters) self.assertFalse('all_tenants' in filters) return [stubs.stub_volume(1, display_name='vol3')] @@ -1279,10 +1260,10 @@ class VolumeApiTest(test.TestCase): self.controller._view_builder.detail_list = mock.Mock() self.controller._get_volumes(req, True) get_all.assert_called_once_with( - context, None, None, + context, None, CONF.osapi_max_limit, sort_keys=['created_at'], sort_dirs=['desc'], filters={'display_name': 'Volume-573108026'}, - viewable_admin_meta=True) + viewable_admin_meta=True, offset=0) @mock.patch('cinder.volume.api.API.get_all') def test_get_volumes_filter_with_list(self, get_all): @@ -1293,9 +1274,10 @@ class VolumeApiTest(test.TestCase): self.controller._view_builder.detail_list = mock.Mock() self.controller._get_volumes(req, True) get_all.assert_called_once_with( - context, None, None, + context, None, CONF.osapi_max_limit, sort_keys=['created_at'], sort_dirs=['desc'], - filters={'id': ['1', '2', '3']}, viewable_admin_meta=True) + filters={'id': ['1', '2', '3']}, viewable_admin_meta=True, + offset=0) @mock.patch('cinder.volume.api.API.get_all') def test_get_volumes_filter_with_expression(self, get_all): @@ -1306,9 +1288,9 @@ class VolumeApiTest(test.TestCase): self.controller._view_builder.detail_list = mock.Mock() self.controller._get_volumes(req, True) get_all.assert_called_once_with( - context, None, None, + context, None, CONF.osapi_max_limit, sort_keys=['created_at'], sort_dirs=['desc'], - filters={'display_name': 'd-'}, viewable_admin_meta=True) + filters={'display_name': 'd-'}, viewable_admin_meta=True, offset=0) @mock.patch('cinder.volume.api.API.get_all') def test_get_volumes_filter_with_status(self, get_all): @@ -1319,9 +1301,10 @@ class VolumeApiTest(test.TestCase): self.controller._view_builder.detail_list = mock.Mock() self.controller._get_volumes(req, True) get_all.assert_called_once_with( - ctxt, None, None, + ctxt, None, CONF.osapi_max_limit, sort_keys=['created_at'], sort_dirs=['desc'], - filters={'status': 'available'}, viewable_admin_meta=True) + filters={'status': 'available'}, viewable_admin_meta=True, + offset=0) @mock.patch('cinder.volume.api.API.get_all') def test_get_volumes_filter_with_metadata(self, get_all): @@ -1332,10 +1315,10 @@ class VolumeApiTest(test.TestCase): self.controller._view_builder.detail_list = mock.Mock() self.controller._get_volumes(req, True) get_all.assert_called_once_with( - ctxt, None, None, + ctxt, None, CONF.osapi_max_limit, sort_keys=['created_at'], sort_dirs=['desc'], filters={'metadata': {'fake_key': 'fake_value'}}, - viewable_admin_meta=True) + viewable_admin_meta=True, offset=0) @mock.patch('cinder.volume.api.API.get_all') def test_get_volumes_filter_with_availability_zone(self, get_all): @@ -1346,9 +1329,10 @@ class VolumeApiTest(test.TestCase): self.controller._view_builder.detail_list = mock.Mock() self.controller._get_volumes(req, True) get_all.assert_called_once_with( - ctxt, None, None, + ctxt, None, CONF.osapi_max_limit, sort_keys=['created_at'], sort_dirs=['desc'], - filters={'availability_zone': 'nova'}, viewable_admin_meta=True) + filters={'availability_zone': 'nova'}, viewable_admin_meta=True, + offset=0) @mock.patch('cinder.volume.api.API.get_all') def test_get_volumes_filter_with_invalid_filter(self, get_all): @@ -1360,9 +1344,10 @@ class VolumeApiTest(test.TestCase): self.controller._view_builder.detail_list = mock.Mock() self.controller._get_volumes(req, True) get_all.assert_called_once_with( - ctxt, None, None, + ctxt, None, CONF.osapi_max_limit, sort_keys=['created_at'], sort_dirs=['desc'], - filters={'availability_zone': 'nova'}, viewable_admin_meta=True) + filters={'availability_zone': 'nova'}, viewable_admin_meta=True, + offset=0) @mock.patch('cinder.volume.api.API.get_all') def test_get_volumes_sort_by_name(self, get_all): @@ -1375,9 +1360,9 @@ class VolumeApiTest(test.TestCase): self.controller._view_builder.detail_list = mock.Mock() self.controller._get_volumes(req, True) get_all.assert_called_once_with( - ctxt, None, None, + ctxt, None, CONF.osapi_max_limit, sort_dirs=['desc'], viewable_admin_meta=True, - sort_keys=['display_name'], filters={}) + sort_keys=['display_name'], filters={}, offset=0) def test_get_volume_filter_options_using_config(self): self.override_config('query_volume_filters', ['name', 'status', diff --git a/cinder/volume/api.py b/cinder/volume/api.py index b44095f5658..c79a0637fd5 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -437,7 +437,8 @@ class API(base.Base): return b def get_all(self, context, marker=None, limit=None, sort_keys=None, - sort_dirs=None, filters=None, viewable_admin_meta=False): + sort_dirs=None, filters=None, viewable_admin_meta=False, + offset=None): check_policy(context, 'get_all') if filters is None: @@ -471,7 +472,8 @@ class API(base.Base): volumes = self.db.volume_get_all(context, marker, limit, sort_keys=sort_keys, sort_dirs=sort_dirs, - filters=filters) + filters=filters, + offset=offset) else: if viewable_admin_meta: context = context.elevated() @@ -480,7 +482,8 @@ class API(base.Base): marker, limit, sort_keys=sort_keys, sort_dirs=sort_dirs, - filters=filters) + filters=filters, + offset=offset) LOG.info(_LI("Get all volumes completed successfully.")) return volumes