Currently snapshot list does not provide pagination like volume does.
This patch adds pagination for snapshots in the same way that volume
does, using marker, limit, sort_keys and sort_dirs.
DocImpact
APIImpact: Use marker, limit, sort_keys and sort_dirs
Implements: snapshots-and-bakcup-support-pagination-query
Implements: blueprint extend-limit-implementations
Change-Id: I21edac6d29abf7fc2f446111a7d6888db9ab6eba
# Pop out non search_opts and create local variables
search_opts = req.GET.copy()
- search_opts.pop('limit', None)
- search_opts.pop('offset', None)
+ sort_keys, sort_dirs = common.get_sort_params(search_opts)
+ marker, limit, offset = common.get_pagination_params(search_opts)
# Filter out invalid options
allowed_search_options = ('status', 'volume_id', 'name')
del search_opts['name']
snapshots = self.volume_api.get_all_snapshots(context,
- search_opts=search_opts)
+ search_opts=search_opts,
+ marker=marker,
+ limit=limit,
+ sort_keys=sort_keys,
+ sort_dirs=sort_dirs,
+ offset=offset)
- limited_list = common.limited(snapshots.objects, req)
- req.cache_db_snapshots(limited_list)
- snapshot_count = len(snapshots)
+ req.cache_db_snapshots(snapshots.objects)
if is_detail:
- snapshots = self._view_builder.detail_list(req, limited_list,
- snapshot_count)
+ snapshots = self._view_builder.detail_list(req, snapshots.objects)
else:
- snapshots = self._view_builder.summary_list(req, limited_list,
- snapshot_count)
+ snapshots = self._view_builder.summary_list(req, snapshots.objects)
return snapshots
@wsgi.response(202)
"""Initialize view builder."""
super(ViewBuilder, self).__init__()
- def summary_list(self, request, snapshots, snapshot_count):
+ def summary_list(self, request, snapshots, snapshot_count=None):
"""Show a list of snapshots without many details."""
return self._list_view(self.summary, request, snapshots,
snapshot_count)
- def detail_list(self, request, snapshots, snapshot_count):
+ def detail_list(self, request, snapshots, snapshot_count=None):
"""Detailed view of a list of snapshots."""
return self._list_view(self.detail, request, snapshots, snapshot_count,
coll_name=self._collection_name + '/detail')
"""Provide a view for a list of snapshots."""
snapshots_list = [func(request, snapshot)['snapshot']
for snapshot in snapshots]
+ snapshots_links = self._get_collection_links(request,
+ snapshots,
+ coll_name,
+ snapshot_count)
snapshots_dict = {self._collection_name: snapshots_list}
+ if snapshots_links:
+ snapshots_dict[self._collection_name + '_links'] = snapshots_links
+
return snapshots_dict
return IMPL.snapshot_get(context, snapshot_id)
-def snapshot_get_all(context, filters=None):
+def snapshot_get_all(context, filters=None, marker=None, limit=None,
+ sort_keys=None, sort_dirs=None, offset=None):
"""Get all snapshots."""
- return IMPL.snapshot_get_all(context, filters)
+ return IMPL.snapshot_get_all(context, filters, marker, limit, sort_keys,
+ sort_dirs, offset)
-def snapshot_get_all_by_project(context, project_id, filters=None):
+def snapshot_get_all_by_project(context, project_id, filters=None, marker=None,
+ limit=None, sort_keys=None, sort_dirs=None,
+ offset=None):
"""Get all snapshots belonging to a project."""
- return IMPL.snapshot_get_all_by_project(context, project_id, filters)
+ return IMPL.snapshot_get_all_by_project(context, project_id, filters,
+ marker, limit, sort_keys,
+ sort_dirs, offset)
def snapshot_get_by_host(context, host, filters=None):
def _generate_paginate_query(context, session, marker, limit, sort_keys,
- sort_dirs, filters, offset=None):
+ sort_dirs, filters, offset=None,
+ paginate_type=models.Volume):
"""Generate the query to include the filters and the paginate options.
Returns a query with sorting / pagination criteria added or None
is used for other values, see _process_volume_filters
function for more information
:param offset: number of items to skip
+ :param paginate_type: type of pagination to generate
:returns: updated query or None
"""
+ get_query, process_filters, get = PAGINATION_HELPERS[paginate_type]
+
sort_keys, sort_dirs = process_sort_params(sort_keys,
sort_dirs,
default_dir='desc')
- query = _volume_get_query(context, session=session)
+ query = get_query(context, session=session)
if filters:
- query = _process_volume_filters(query, filters)
+ query = process_filters(query, filters)
if query is None:
return None
marker_volume = None
if marker is not None:
- marker_volume = _volume_get(context, marker, session)
+ marker_volume = get(context, marker, session)
- return sqlalchemyutils.paginate_query(query, models.Volume, limit,
+ return sqlalchemyutils.paginate_query(query, paginate_type, limit,
sort_keys,
marker=marker_volume,
sort_dirs=sort_dirs,
@require_admin_context
-def snapshot_get_all(context, filters=None):
- # Ensure that the filter value exists on the model
- if filters:
- for key in filters.keys():
- try:
- getattr(models.Snapshot, key)
- except AttributeError:
- LOG.debug("'%s' filter key is not valid.", key)
- return []
+def snapshot_get_all(context, filters=None, marker=None, limit=None,
+ sort_keys=None, sort_dirs=None, offset=None):
+ """Retrieves all snapshots.
+
+ If no sorting parameters are specified then returned snapshots are sorted
+ first by the 'created_at' key and then by the 'id' key in descending
+ order.
+
+ :param context: context to query under
+ :param filters: dictionary of filters; will do exact matching on values
+ :param marker: the last item of the previous page, used to determine the
+ next page of results to return
+ :param limit: maximum number of items to return
+ :param sort_keys: list of attributes by which results should be sorted,
+ paired with corresponding item in sort_dirs
+ :param sort_dirs: list of directions in which results should be sorted,
+ paired with corresponding item in sort_keys
+ :returns: list of matching snapshots
+ """
+ session = get_session()
+ with session.begin():
+ query = _generate_paginate_query(context, session, marker, limit,
+ sort_keys, sort_dirs, filters,
+ offset, models.Snapshot)
+
+ # No snapshots would match, return empty list
+ if not query:
+ return []
+ return query.all()
+
+
+def _snaps_get_query(context, session=None, project_only=False):
+ return model_query(context, models.Snapshot, session=session,
+ project_only=project_only).\
+ options(joinedload('snapshot_metadata'))
- query = model_query(context, models.Snapshot)
+def _process_snaps_filters(query, filters):
if filters:
+ # Ensure that filters' keys exist on the model
+ if not is_valid_model_filters(models.Snapshot, filters):
+ return None
query = query.filter_by(**filters)
-
- return query.options(joinedload('snapshot_metadata')).all()
+ return query
@require_context
@require_context
-def snapshot_get_all_by_project(context, project_id, filters=None):
+def snapshot_get_all_by_project(context, project_id, filters=None, marker=None,
+ limit=None, sort_keys=None, sort_dirs=None,
+ offset=None):
+ """"Retrieves all snapshots in a project.
+
+ If no sorting parameters are specified then returned snapshots are sorted
+ first by the 'created_at' key and then by the 'id' key in descending
+ order.
+
+ :param context: context to query under
+ :param project_id: project for all snapshots being retrieved
+ :param filters: dictionary of filters; will do exact matching on values
+ :param marker: the last item of the previous page, used to determine the
+ next page of results to return
+ :param limit: maximum number of items to return
+ :param sort_keys: list of attributes by which results should be sorted,
+ paired with corresponding item in sort_dirs
+ :param sort_dirs: list of directions in which results should be sorted,
+ paired with corresponding item in sort_keys
+ :returns: list of matching snapshots
+ """
authorize_project_context(context, project_id)
- query = model_query(context, models.Snapshot)
- if filters:
- query = query.filter_by(**filters)
+ # Add project_id to filters
+ filters = filters.copy() if filters else {}
+ filters['project_id'] = project_id
- return query.filter_by(project_id=project_id).\
- options(joinedload('snapshot_metadata')).all()
+ session = get_session()
+ with session.begin():
+ query = _generate_paginate_query(context, session, marker, limit,
+ sort_keys, sort_dirs, filters,
+ offset, models.Snapshot)
+
+ # No snapshots would match, return empty list
+ if not query:
+ return []
+ return query.all()
@require_context
filter_by(initiator=initiator).\
filter_by(namespace=namespace).\
all()
+
+
+###############################
+
+
+PAGINATION_HELPERS = {
+ models.Volume: (_volume_get_query, _process_volume_filters, _volume_get),
+ models.Snapshot: (_snaps_get_query, _process_snaps_filters, _snapshot_get)
+}
}
@base.remotable_classmethod
- def get_all(cls, context, search_opts):
- snapshots = db.snapshot_get_all(context, search_opts)
+ def get_all(cls, context, search_opts, marker=None, limit=None,
+ sort_keys=None, sort_dirs=None, offset=None):
+ snapshots = db.snapshot_get_all(context, search_opts, marker, limit,
+ sort_keys, sort_dirs, offset)
return base.obj_make_list(context, cls(), objects.Snapshot,
snapshots,
expected_attrs=['metadata'])
snapshots, expected_attrs=['metadata'])
@base.remotable_classmethod
- def get_all_by_project(cls, context, project_id, search_opts):
- snapshots = db.snapshot_get_all_by_project(context, project_id,
- search_opts)
+ def get_all_by_project(cls, context, project_id, search_opts, marker=None,
+ limit=None, sort_keys=None, sort_dirs=None,
+ offset=None):
+ snapshots = db.snapshot_get_all_by_project(
+ context, project_id, search_opts, marker, limit, sort_keys,
+ sort_dirs, offset)
return base.obj_make_list(context, cls(context), objects.Snapshot,
snapshots, expected_attrs=['metadata'])
return snapshot
-def stub_snapshot_get_all(self, search_opts=None):
+def stub_snapshot_get_all(context, filters=None, marker=None, limit=None,
+ sort_keys=None, sort_dirs=None, offset=None):
return [stub_snapshot(100, project_id='fake'),
stub_snapshot(101, project_id='superfake'),
stub_snapshot(102, project_id='superduperfake')]
-def stub_snapshot_get_all_by_project(self, context, search_opts=None):
+def stub_snapshot_get_all_by_project(context, project_id, filters=None,
+ marker=None, limit=None, sort_keys=None,
+ sort_dirs=None, offset=None):
return [stub_snapshot(1)]
snapshot_metadata_get):
def list_snapshots_with_limit_and_offset(is_admin):
def stub_snapshot_get_all_by_project(context, project_id,
- search_opts):
+ filters=None, marker=None,
+ limit=None, sort_keys=None,
+ sort_dirs=None, offset=None):
return [
stubs.stub_snapshot(1, display_name='backup1'),
stubs.stub_snapshot(2, display_name='backup2'),
return snapshot
-def stub_snapshot_get_all(self, search_opts=None):
+def stub_snapshot_get_all(context, filters=None, marker=None, limit=None,
+ sort_keys=None, sort_dirs=None, offset=None):
return [stub_snapshot(100, project_id='fake'),
stub_snapshot(101, project_id='superfake'),
stub_snapshot(102, project_id='superduperfake')]
-def stub_snapshot_get_all_by_project(self, context, search_opts=None):
+def stub_snapshot_get_all_by_project(context, project_id, filters=None,
+ marker=None, limit=None, sort_keys=None,
+ sort_dirs=None, offset=None):
return [stub_snapshot(1)]
from lxml import etree
import mock
+from oslo_config import cfg
from oslo_utils import timeutils
+from six.moves.urllib import parse as urllib
import webob
+from cinder.api import common
from cinder.api.v2 import snapshots
from cinder import context
from cinder import db
from cinder import volume
+CONF = cfg.CONF
+
UUID = '00000000-0000-0000-0000-000000000001'
INVALID_UUID = '00000000-0000-0000-0000-000000000002'
stubs.stub_snapshot_get_all_by_project)
self.stubs.Set(db, 'snapshot_get_all',
stubs.stub_snapshot_get_all)
+
self.ctx = context.RequestContext('admin', 'fakeproject', True)
@mock.patch(
@mock.patch('cinder.db.snapshot_metadata_get', return_value=dict())
def test_list_snapshots_with_limit_and_offset(self,
snapshot_metadata_get):
- def list_snapshots_with_limit_and_offset(is_admin):
- def stub_snapshot_get_all_by_project(context, project_id,
- search_opts):
- return [
- stubs.stub_snapshot(1, display_name='backup1'),
- stubs.stub_snapshot(2, display_name='backup2'),
- stubs.stub_snapshot(3, display_name='backup3'),
- ]
-
- self.stubs.Set(db, 'snapshot_get_all_by_project',
- stub_snapshot_get_all_by_project)
-
+ def list_snapshots_with_limit_and_offset(snaps, is_admin):
req = fakes.HTTPRequest.blank('/v2/fake/snapshots?limit=1\
&offset=1',
use_admin_context=is_admin)
self.assertIn('snapshots', res)
self.assertEqual(1, len(res['snapshots']))
- self.assertEqual('2', res['snapshots'][0]['id'])
+ self.assertEqual(snaps[1].id, res['snapshots'][0]['id'])
+ # Test that we get an empty list with an offset greater than the
+ # number of items
+ req = fakes.HTTPRequest.blank('/v2/snapshots?limit=1&offset=3')
+ self.assertEqual({'snapshots': []}, self.controller.index(req))
+
+ self.stubs.UnsetAll()
+ volume, snaps = self._create_db_snapshots(3)
# admin case
- list_snapshots_with_limit_and_offset(is_admin=True)
+ list_snapshots_with_limit_and_offset(snaps, is_admin=True)
# non-admin case
- list_snapshots_with_limit_and_offset(is_admin=False)
+ list_snapshots_with_limit_and_offset(snaps, is_admin=False)
+
+ @mock.patch.object(db, 'snapshot_get_all_by_project')
+ @mock.patch('cinder.db.snapshot_metadata_get', return_value=dict())
+ def test_list_snpashots_with_wrong_limit_and_offset(self,
+ mock_metadata_get,
+ mock_snapshot_get_all):
+ """Test list with negative and non numeric limit and offset."""
+ mock_snapshot_get_all.return_value = []
+
+ # Negative limit
+ req = fakes.HTTPRequest.blank('/v2/snapshots?limit=-1&offset=1')
+ self.assertRaises(webob.exc.HTTPBadRequest,
+ self.controller.index,
+ req)
+
+ # Non numeric limit
+ req = fakes.HTTPRequest.blank('/v2/snapshots?limit=a&offset=1')
+ self.assertRaises(webob.exc.HTTPBadRequest,
+ self.controller.index,
+ req)
+
+ # Negative offset
+ req = fakes.HTTPRequest.blank('/v2/snapshots?limit=1&offset=-1')
+ self.assertRaises(webob.exc.HTTPBadRequest,
+ self.controller.index,
+ req)
+
+ # Non numeric offset
+ req = fakes.HTTPRequest.blank('/v2/snapshots?limit=1&offset=a')
+ self.assertRaises(webob.exc.HTTPBadRequest,
+ self.controller.index,
+ req)
+
+ def _assert_list_next(self, expected_query=None, project='fakeproject',
+ **kwargs):
+ """Check a page of snapshots list."""
+ # Since we are accessing v2 api directly we don't need to specify
+ # v2 in the request path, if we did, we'd get /v2/v2 links back
+ request_path = '/%s/snapshots' % project
+ expected_path = '/v2' + request_path
+
+ # Construct the query if there are kwargs
+ if kwargs:
+ request_str = request_path + '?' + urllib.urlencode(kwargs)
+ else:
+ request_str = request_path
+
+ # Make the request
+ req = fakes.HTTPRequest.blank(request_str)
+ res = self.controller.index(req)
+
+ # We only expect to have a next link if there is an actual expected
+ # query.
+ if expected_query:
+ # We must have the links
+ self.assertIn('snapshots_links', res)
+ links = res['snapshots_links']
+
+ # Must be a list of links, even if we only get 1 back
+ self.assertTrue(list, type(links))
+ next_link = links[0]
+
+ # rel entry must be next
+ self.assertIn('rel', next_link)
+ self.assertIn('next', next_link['rel'])
+
+ # href entry must have the right path
+ self.assertIn('href', next_link)
+ href_parts = urllib.urlparse(next_link['href'])
+ self.assertEqual(expected_path, href_parts.path)
+
+ # And the query from the next link must match what we were
+ # expecting
+ params = urllib.parse_qs(href_parts.query)
+ self.assertDictEqual(expected_query, params)
+
+ # Make sure we don't have links if we were not expecting them
+ else:
+ self.assertNotIn('snapshots_links', res)
+
+ def _create_db_snapshots(self, num_snaps):
+ volume = utils.create_volume(self.ctx)
+ snaps = [utils.create_snapshot(self.ctx,
+ volume.id,
+ display_name='snap' + str(i))
+ for i in range(num_snaps)]
+
+ self.addCleanup(db.volume_destroy, self.ctx, volume.id)
+ for snap in snaps:
+ self.addCleanup(db.snapshot_destroy, self.ctx, snap.id)
+
+ snaps.reverse()
+ return volume, snaps
+
+ def test_list_snapshots_next_link_default_limit(self):
+ """Test that snapshot list pagination is limited by osapi_max_limit."""
+ self.stubs.UnsetAll()
+ volume, snaps = self._create_db_snapshots(3)
+
+ # NOTE(geguileo): Since cinder.api.common.limited has already been
+ # imported his argument max_limit already has a default value of 1000
+ # so it doesn't matter that we change it to 2. That's why we need to
+ # mock it and send it current value. We still need to set the default
+ # value because other sections of the code use it, for example
+ # _get_collection_links
+ CONF.set_default('osapi_max_limit', 2)
+
+ 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):
+ # The link from the first page should link to the second
+ self._assert_list_next({'marker': [snaps[1].id]})
+
+ # Second page should have no next link
+ self._assert_list_next(marker=snaps[1].id)
+
+ def test_list_snapshots_next_link_with_limit(self):
+ """Test snapshot list pagination with specific limit."""
+ self.stubs.UnsetAll()
+ volume, snaps = self._create_db_snapshots(2)
+
+ # The link from the first page should link to the second
+ self._assert_list_next({'limit': ['1'], 'marker': [snaps[0].id]},
+ limit=1)
+
+ # Even though there are no more elements, we should get a next element
+ # per specification.
+ expected = {'limit': ['1'], 'marker': [snaps[1].id]}
+ self._assert_list_next(expected, limit=1, marker=snaps[0].id)
+
+ # When we go beyond the number of elements there should be no more
+ # next links
+ self._assert_list_next(limit=1, marker=snaps[1].id)
@mock.patch('cinder.db.snapshot_metadata_get', return_value=dict())
def test_admin_list_snapshots_all_tenants(self, snapshot_metadata_get):
self.context, search_opts)
self.assertEqual(1, len(snapshots))
TestSnapshot._compare(self, fake_snapshot_obj, snapshots[0])
- snapshot_get_all.assert_called_once_with(self.context, search_opts)
+ snapshot_get_all.assert_called_once_with(self.context, search_opts,
+ None, None, None, None, None)
@mock.patch('cinder.objects.Volume.get_by_id')
@mock.patch('cinder.db.snapshot_get_by_host',
TestSnapshot._compare(self, fake_snapshot_obj, snapshots[0])
get_all_by_project.assert_called_once_with(self.context,
self.project_id,
- search_opts)
+ search_opts, None, None,
+ None, None, None)
@mock.patch('cinder.objects.volume.Volume.get_by_id')
@mock.patch('cinder.db.snapshot_get_all_for_volume',
snapshot_obj = copy.deepcopy(fake_snapshot_obj)
snapshot_obj['metadata'] = {'fake_key': 'fake_value'}
TestSnapshot._compare(self, snapshot_obj, snapshots[0])
- snapshot_get_all.assert_called_once_with(self.context, search_opts)
+ snapshot_get_all.assert_called_once_with(self.context, search_opts,
+ None, None, None, None, None)
LOG.info(_LI("Volume retrieved successfully."), resource=vref)
return dict(vref)
- def get_all_snapshots(self, context, search_opts=None):
+ def get_all_snapshots(self, context, search_opts=None, marker=None,
+ limit=None, sort_keys=None, sort_dirs=None,
+ offset=None):
check_policy(context, 'get_all_snapshots')
search_opts = search_opts or {}
- if (context.is_admin and 'all_tenants' in search_opts):
+ if context.is_admin and 'all_tenants' in search_opts:
# Need to remove all_tenants to pass the filtering below.
del search_opts['all_tenants']
- snapshots = objects.SnapshotList.get_all(context,
- search_opts)
+ snapshots = objects.SnapshotList.get_all(
+ context, search_opts, marker, limit, sort_keys, sort_dirs,
+ offset)
else:
snapshots = objects.SnapshotList.get_all_by_project(
- context, context.project_id, search_opts)
+ context, context.project_id, search_opts, marker, limit,
+ sort_keys, sort_dirs, offset)
LOG.info(_LI("Get all snaphsots completed successfully."))
return snapshots
Code Review / openstack-build / cinder-build.git / commitdiff