openstack/deb-cinder
Code Issues Proposed changes

On Volume list only retrieve needed data from DB

Browse Source 
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 <wangxiyuan@huawei.com>
Closes-bug:#1483165i
Change-Id: Ie903e546074fe118299e8e1acfb9c88c8a10d78c
This commit is contained in:
Gorka Eguileor 2015-08-12 19:09:53 +02:00
parent 7e84f149e8
commit 28879f9a78
11 changed files with 313 additions and 351 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

144
cinder/api/common.py
View File

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

4
cinder/api/v2/views/volumes.py
View File

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

17
cinder/api/v2/volumes.py
View File

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

5
cinder/common/sqlalchemyutils.py
View File

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

11
cinder/db/api.py
View File

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

15
cinder/db/sqlalchemy/api.py
View File

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

205
cinder/tests/unit/api/test_common.py
View File

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

3
cinder/tests/unit/api/v1/test_volumes.py
View File

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

4
cinder/tests/unit/api/v2/stubs.py
View File

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

247
cinder/tests/unit/api/v2/test_volumes.py
View File

@ -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',

9
cinder/volume/api.py
View File

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