From a176378361744aecaa8287ed4ca7e7c5fb6ba03f Mon Sep 17 00:00:00 2001 From: Steven Kaufer Date: Mon, 15 Dec 2014 20:18:42 +0000 Subject: [PATCH] GET volumes API sorting REST/volume/DB updates This change is to support updating the v2 /volumes and /volumes/detail APIs to support multiple sort keys and sort directions. Contains: * Updates to the v2 REST API to pass multiple sort keys and directions to the volume API * Volume API updates to accept and pass the sort information the DB layer * API signature updates on the DB layer to accept a list of sort keys and directions Note that the defaulting of the sort keys and directions is done in the dependent patch set in the new 'process_sort_params' function (invoked in db.sqlalchemy.api); by default, the sort keys are 'created_at' and 'id' in the 'desc' direction. Partially Implements: blueprint cinder-pagination APIImpact Change-Id: I02bd8104b501b496148f95b0a5045fa2b64c8802 --- cinder/api/v1/volumes.py | 5 +- cinder/api/v2/volumes.py | 9 +- cinder/db/api.py | 14 +- cinder/db/sqlalchemy/api.py | 47 ++++-- cinder/tests/api/v1/test_volumes.py | 16 +- cinder/tests/api/v2/stubs.py | 7 +- cinder/tests/api/v2/test_volumes.py | 69 +++++--- cinder/tests/test_db_api.py | 234 ++++++++++++++++++++++------ cinder/volume/api.py | 13 +- cinder/volume/manager.py | 3 +- 10 files changed, 303 insertions(+), 114 deletions(-) diff --git a/cinder/api/v1/volumes.py b/cinder/api/v1/volumes.py index 4798f5f4de6..b2ff3d6d679 100644 --- a/cinder/api/v1/volumes.py +++ b/cinder/api/v1/volumes.py @@ -282,8 +282,9 @@ class VolumeController(wsgi.Controller): self._get_volume_search_options()) volumes = self.volume_api.get_all(context, marker=None, limit=None, - sort_key='created_at', - sort_dir='desc', filters=search_opts, + sort_keys=['created_at'], + sort_dirs=['desc'], + filters=search_opts, viewable_admin_meta=True) volumes = [dict(vol.iteritems()) for vol in volumes] diff --git a/cinder/api/v2/volumes.py b/cinder/api/v2/volumes.py index 0844e7acb9f..ddc2d351c46 100644 --- a/cinder/api/v2/volumes.py +++ b/cinder/api/v2/volumes.py @@ -216,8 +216,7 @@ class VolumeController(wsgi.Controller): params = req.params.copy() marker = params.pop('marker', None) limit = params.pop('limit', None) - sort_key = params.pop('sort_key', 'created_at') - sort_dir = params.pop('sort_dir', 'desc') + sort_keys, sort_dirs = common.get_sort_params(params) params.pop('offset', None) filters = params @@ -236,8 +235,10 @@ class VolumeController(wsgi.Controller): except (ValueError, SyntaxError): LOG.debug('Could not evaluate value %s, assuming string', v) - volumes = self.volume_api.get_all(context, marker, limit, sort_key, - sort_dir, filters, + volumes = self.volume_api.get_all(context, marker, limit, + sort_keys=sort_keys, + sort_dirs=sort_dirs, + filters=filters, viewable_admin_meta=True) volumes = [dict(vol.iteritems()) for vol in volumes] diff --git a/cinder/db/api.py b/cinder/db/api.py index 9d36132dd9e..84d62571b13 100644 --- a/cinder/db/api.py +++ b/cinder/db/api.py @@ -179,11 +179,11 @@ def volume_get(context, volume_id): return IMPL.volume_get(context, volume_id) -def volume_get_all(context, marker, limit, sort_key, sort_dir, +def volume_get_all(context, marker, limit, sort_keys=None, sort_dirs=None, filters=None): """Get all volumes.""" - return IMPL.volume_get_all(context, marker, limit, sort_key, sort_dir, - filters=filters) + return IMPL.volume_get_all(context, marker, limit, sort_keys=sort_keys, + sort_dirs=sort_dirs, filters=filters) def volume_get_all_by_host(context, host, filters=None): @@ -196,11 +196,13 @@ def volume_get_all_by_group(context, group_id, filters=None): return IMPL.volume_get_all_by_group(context, group_id, filters=filters) -def volume_get_all_by_project(context, project_id, marker, limit, sort_key, - sort_dir, filters=None): +def volume_get_all_by_project(context, project_id, marker, limit, + sort_keys=None, sort_dirs=None, filters=None): """Get all volumes belonging to a project.""" return IMPL.volume_get_all_by_project(context, project_id, marker, limit, - sort_key, sort_dir, filters=filters) + sort_keys=sort_keys, + sort_dirs=sort_dirs, + filters=filters) def volume_get_iscsi_target_num(context, volume_id): diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index 62beae875a3..b0c51eb6349 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -1184,16 +1184,22 @@ def volume_get(context, volume_id): @require_admin_context -def volume_get_all(context, marker, limit, sort_key, sort_dir, +def volume_get_all(context, marker, limit, sort_keys=None, sort_dirs=None, filters=None): """Retrieves all volumes. + If no sort parameters are specified then the returned volumes are sorted + first by the 'created_at' key and then by the 'id' key in descending + order. + :param context: context to query under :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_key: single attributes by which results should be sorted - :param sort_dir: direction in which results should be sorted (asc, desc) + :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 :param filters: dictionary of filters; values that are in lists, tuples, or sets cause an 'IN' operation, while exact matching is used for other values, see _process_volume_filters @@ -1204,7 +1210,7 @@ def volume_get_all(context, marker, limit, sort_key, sort_dir, with session.begin(): # Generate the query query = _generate_paginate_query(context, session, marker, limit, - sort_key, sort_dir, filters) + sort_keys, sort_dirs, filters) # No volumes would match, return empty list if query is None: return [] @@ -1268,17 +1274,23 @@ 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_key, - sort_dir, filters=None): +def volume_get_all_by_project(context, project_id, marker, limit, + sort_keys=None, sort_dirs=None, filters=None): """"Retrieves all volumes in a project. + If no sort parameters are specified then the returned volumes 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 volumes being retrieved :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_key: single attributes by which results should be sorted - :param sort_dir: direction in which results should be sorted (asc, desc) + :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 :param filters: dictionary of filters; values that are in lists, tuples, or sets cause an 'IN' operation, while exact matching is used for other values, see _process_volume_filters @@ -1293,15 +1305,15 @@ def volume_get_all_by_project(context, project_id, marker, limit, sort_key, filters['project_id'] = project_id # Generate the query query = _generate_paginate_query(context, session, marker, limit, - sort_key, sort_dir, filters) + sort_keys, sort_dirs, filters) # No volumes would match, return empty list if query is None: return [] return query.all() -def _generate_paginate_query(context, session, marker, limit, sort_key, - sort_dir, filters): +def _generate_paginate_query(context, session, marker, limit, sort_keys, + sort_dirs, filters): """Generate the query to include the filters and the paginate options. Returns a query with sorting / pagination criteria added or None @@ -1312,14 +1324,19 @@ def _generate_paginate_query(context, session, marker, limit, sort_key, :param marker: the last item of the previous page; we returns the next results after this value. :param limit: maximum number of items to return - :param sort_key: single attributes by which results should be sorted - :param sort_dir: direction in which results should be sorted (asc, desc) + :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 :param filters: dictionary of filters; values that are in lists, tuples, or sets cause an 'IN' operation, while exact matching is used for other values, see _process_volume_filters function for more information :returns: updated query or None """ + sort_keys, sort_dirs = process_sort_params(sort_keys, + sort_dirs, + default_dir='desc') query = _volume_get_query(context, session=session) if filters: @@ -1332,9 +1349,9 @@ def _generate_paginate_query(context, session, marker, limit, sort_key, marker_volume = _volume_get(context, marker, session) return sqlalchemyutils.paginate_query(query, models.Volume, limit, - [sort_key, 'created_at', 'id'], + sort_keys, marker=marker_volume, - sort_dir=sort_dir) + sort_dirs=sort_dirs) def _process_volume_filters(query, filters): diff --git a/cinder/tests/api/v1/test_volumes.py b/cinder/tests/api/v1/test_volumes.py index ef1226704a0..5cbb089850f 100644 --- a/cinder/tests/api/v1/test_volumes.py +++ b/cinder/tests/api/v1/test_volumes.py @@ -625,8 +625,8 @@ class VolumeApiTest(test.TestCase): def test_volume_detail_limit_offset(self): def volume_detail_limit_offset(is_admin): def stub_volume_get_all_by_project(context, project_id, marker, - limit, sort_key, sort_dir, - filters=None, + limit, sort_keys=None, + sort_dirs=None, filters=None, viewable_admin_meta=False): return [ stubs.stub_volume(1, display_name='vol1'), @@ -770,8 +770,8 @@ class VolumeApiTest(test.TestCase): req.environ = {'cinder.context': context} self.controller._items(req, mock.Mock) get_all.assert_called_once_with( - context, sort_dir='desc', viewable_admin_meta=True, - sort_key='created_at', limit=None, + context, sort_dirs=['desc'], viewable_admin_meta=True, + sort_keys=['created_at'], limit=None, filters={'display_name': 'Volume-573108026'}, marker=None) @mock.patch('cinder.volume.api.API.get_all') @@ -782,8 +782,8 @@ class VolumeApiTest(test.TestCase): req.environ = {'cinder.context': context} self.controller._items(req, mock.Mock) get_all.assert_called_once_with( - context, sort_dir='desc', viewable_admin_meta=True, - sort_key='created_at', limit=None, + context, sort_dirs=['desc'], viewable_admin_meta=True, + sort_keys=['created_at'], limit=None, filters={'id': ['1', '2', '3']}, marker=None) @mock.patch('cinder.volume.api.API.get_all') @@ -794,8 +794,8 @@ class VolumeApiTest(test.TestCase): req.environ = {'cinder.context': context} self.controller._items(req, mock.Mock) get_all.assert_called_once_with( - context, sort_dir='desc', viewable_admin_meta=True, - sort_key='created_at', limit=None, filters={'id': 'd+'}, + context, sort_dirs=['desc'], viewable_admin_meta=True, + sort_keys=['created_at'], limit=None, filters={'id': 'd+'}, marker=None) diff --git a/cinder/tests/api/v2/stubs.py b/cinder/tests/api/v2/stubs.py index f0c1e36a419..f3a5b79ee5a 100644 --- a/cinder/tests/api/v2/stubs.py +++ b/cinder/tests/api/v2/stubs.py @@ -124,15 +124,16 @@ def stub_volume_get_db(context, volume_id): def stub_volume_get_all(context, search_opts=None, marker=None, limit=None, - sort_key='created_at', sort_dir='desc', filters=None, + sort_keys=None, sort_dirs=None, filters=None, viewable_admin_meta=False): return [stub_volume(100, project_id='fake'), stub_volume(101, project_id='superfake'), stub_volume(102, project_id='superduperfake')] -def stub_volume_get_all_by_project(self, context, marker, limit, sort_key, - sort_dir, filters=None, +def stub_volume_get_all_by_project(self, context, marker, limit, + sort_keys=None, sort_dirs=None, + filters=None, viewable_admin_meta=False): filters = filters or {} return [stub_volume_get(self, context, '1')] diff --git a/cinder/tests/api/v2/test_volumes.py b/cinder/tests/api/v2/test_volumes.py index c200e6ade86..9f51d02b9ac 100644 --- a/cinder/tests/api/v2/test_volumes.py +++ b/cinder/tests/api/v2/test_volumes.py @@ -928,7 +928,8 @@ class VolumeApiTest(test.TestCase): def test_volume_index_with_marker(self): def stub_volume_get_all_by_project(context, project_id, marker, limit, - sort_key, sort_dir, filters=None, + sort_keys=None, sort_dirs=None, + filters=None, viewable_admin_meta=False): return [ stubs.stub_volume(1, display_name='vol1'), @@ -950,19 +951,27 @@ class VolumeApiTest(test.TestCase): stubs.stub_volume_get_all_by_project) self.stubs.Set(volume_api.API, 'get', stubs.stub_volume_get) - req = fakes.HTTPRequest.blank('/v2/volumes?limit=1') + req = fakes.HTTPRequest.blank('/v2/volumes' + '?limit=1&name=foo' + '&sort=id1:asc') res_dict = self.controller.index(req) volumes = res_dict['volumes'] self.assertEqual(len(volumes), 1) - # Ensure that the next link is correctly formatted + # Ensure that the next link is correctly formatted, it should + # contain the same limit, filter, and sort information as the + # original request as well as a marker; this ensures that the + # caller can simply use the "next" link and that they do not + # need to manually insert the limit and sort information. links = res_dict['volumes_links'] self.assertEqual(links[0]['rel'], 'next') href_parts = urlparse.urlparse(links[0]['href']) self.assertEqual('/v2/fakeproject/volumes', href_parts.path) params = urlparse.parse_qs(href_parts.query) - self.assertTrue('marker' in params) + self.assertEqual(str(volumes[0]['id']), params['marker'][0]) self.assertEqual('1', params['limit'][0]) + self.assertEqual('foo', params['name'][0]) + self.assertEqual('id1:asc', params['sort'][0]) def test_volume_index_limit_negative(self): req = fakes.HTTPRequest.blank('/v2/volumes?limit=-1') @@ -989,7 +998,8 @@ class VolumeApiTest(test.TestCase): def test_volume_index_limit_offset(self): def stub_volume_get_all_by_project(context, project_id, marker, limit, - sort_key, sort_dir, filters=None, + sort_keys=None, sort_dirs=None, + filters=None, viewable_admin_meta=False): return [ stubs.stub_volume(1, display_name='vol1'), @@ -1017,7 +1027,8 @@ class VolumeApiTest(test.TestCase): def test_volume_detail_with_marker(self): def stub_volume_get_all_by_project(context, project_id, marker, limit, - sort_key, sort_dir, filters=None, + sort_keys=None, sort_dirs=None, + filters=None, viewable_admin_meta=False): return [ stubs.stub_volume(1, display_name='vol1'), @@ -1078,7 +1089,8 @@ class VolumeApiTest(test.TestCase): def test_volume_detail_limit_offset(self): def stub_volume_get_all_by_project(context, project_id, marker, limit, - sort_key, sort_dir, filters=None, + sort_keys=None, sort_dirs=None, + filters=None, viewable_admin_meta=False): return [ stubs.stub_volume(1, display_name='vol1'), @@ -1112,8 +1124,7 @@ class VolumeApiTest(test.TestCase): req) def test_volume_with_limit_zero(self): - def stub_volume_get_all(context, marker, limit, - sort_key, sort_dir, **kwargs): + def stub_volume_get_all(context, marker, limit, **kwargs): return [] self.stubs.Set(db, 'volume_get_all', stub_volume_get_all) req = fakes.HTTPRequest.blank('/v2/volumes?limit=0') @@ -1136,7 +1147,7 @@ class VolumeApiTest(test.TestCase): # Number of volumes equals the max, include next link def stub_volume_get_all(context, marker, limit, - sort_key, sort_dir, + sort_keys=None, sort_dirs=None, filters=None, viewable_admin_meta=False): vols = [stubs.stub_volume(i) @@ -1155,7 +1166,7 @@ class VolumeApiTest(test.TestCase): # Number of volumes less than max, do not include def stub_volume_get_all2(context, marker, limit, - sort_key, sort_dir, + sort_keys=None, sort_dirs=None, filters=None, viewable_admin_meta=False): vols = [stubs.stub_volume(i) @@ -1173,7 +1184,7 @@ class VolumeApiTest(test.TestCase): # Number of volumes more than the max, include next link def stub_volume_get_all3(context, marker, limit, - sort_key, sort_dir, + sort_keys=None, sort_dirs=None, filters=None, viewable_admin_meta=False): vols = [stubs.stub_volume(i) @@ -1214,14 +1225,16 @@ class VolumeApiTest(test.TestCase): """ # Non-admin, project function should be called with no_migration_status def stub_volume_get_all_by_project(context, project_id, marker, limit, - sort_key, sort_dir, filters=None, + sort_keys=None, sort_dirs=None, + filters=None, viewable_admin_meta=False): self.assertEqual(filters['no_migration_targets'], True) self.assertFalse('all_tenants' in filters) return [stubs.stub_volume(1, display_name='vol1')] def stub_volume_get_all(context, marker, limit, - sort_key, sort_dir, filters=None, + sort_keys=None, sort_dirs=None, + filters=None, viewable_admin_meta=False): return [] self.stubs.Set(db, 'volume_get_all_by_project', @@ -1238,13 +1251,15 @@ class VolumeApiTest(test.TestCase): # Admin, all_tenants is not set, project function should be called # without no_migration_status def stub_volume_get_all_by_project2(context, project_id, marker, limit, - sort_key, sort_dir, filters=None, + sort_keys=None, sort_dirs=None, + filters=None, viewable_admin_meta=False): self.assertFalse('no_migration_targets' in filters) return [stubs.stub_volume(1, display_name='vol2')] def stub_volume_get_all2(context, marker, limit, - sort_key, sort_dir, filters=None, + sort_keys=None, sort_dirs=None, + filters=None, viewable_admin_meta=False): return [] self.stubs.Set(db, 'volume_get_all_by_project', @@ -1259,12 +1274,14 @@ class VolumeApiTest(test.TestCase): # Admin, all_tenants is set, get_all function should be called # without no_migration_status def stub_volume_get_all_by_project3(context, project_id, marker, limit, - sort_key, sort_dir, filters=None, + sort_keys=None, sort_dirs=None, + filters=None, viewable_admin_meta=False): return [] def stub_volume_get_all3(context, marker, limit, - sort_key, sort_dir, filters=None, + sort_keys=None, sort_dirs=None, + filters=None, viewable_admin_meta=False): self.assertFalse('no_migration_targets' in filters) self.assertFalse('all_tenants' in filters) @@ -1551,8 +1568,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, 'created_at', 'desc', - {'display_name': 'Volume-573108026'}, viewable_admin_meta=True) + context, None, None, + sort_keys=['created_at'], sort_dirs=['desc'], + filters={'display_name': 'Volume-573108026'}, + viewable_admin_meta=True) @mock.patch('cinder.volume.api.API.get_all') def test_get_volumes_filter_with_list(self, get_all): @@ -1563,8 +1582,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, 'created_at', 'desc', - {'id': ['1', '2', '3']}, viewable_admin_meta=True) + context, None, None, + sort_keys=['created_at'], sort_dirs=['desc'], + filters={'id': ['1', '2', '3']}, viewable_admin_meta=True) @mock.patch('cinder.volume.api.API.get_all') def test_get_volumes_filter_with_expression(self, get_all): @@ -1575,8 +1595,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, 'created_at', 'desc', - {'display_name': 'd-'}, viewable_admin_meta=True) + context, None, None, + sort_keys=['created_at'], sort_dirs=['desc'], + filters={'display_name': 'd-'}, viewable_admin_meta=True) class VolumeSerializerTest(test.TestCase): diff --git a/cinder/tests/test_db_api.py b/cinder/tests/test_db_api.py index e2d3f5f6443..5678039df0b 100644 --- a/cinder/tests/test_db_api.py +++ b/cinder/tests/test_db_api.py @@ -315,7 +315,7 @@ class DBAPIVolumeTestCase(BaseTest): {'host': 'h%d' % i, 'size': i}) for i in xrange(3)] self._assertEqualListsOfObjects(volumes, db.volume_get_all( - self.ctxt, None, None, 'host', None)) + self.ctxt, None, None, ['host'], None)) def test_volume_get_all_marker_passed(self): volumes = [ @@ -326,7 +326,7 @@ class DBAPIVolumeTestCase(BaseTest): ] self._assertEqualListsOfObjects(volumes[2:], db.volume_get_all( - self.ctxt, 2, 2, 'id', None)) + self.ctxt, 2, 2, ['id'], ['asc'])) def test_volume_get_all_by_host(self): volumes = [] @@ -433,7 +433,7 @@ class DBAPIVolumeTestCase(BaseTest): self._assertEqualListsOfObjects(volumes[i], db.volume_get_all_by_project( self.ctxt, 'p%d' % i, None, - None, 'host', None)) + None, ['host'], None)) def test_volume_get_by_name(self): db.volume_create(self.ctxt, {'display_name': 'vol1'}) @@ -441,17 +441,17 @@ class DBAPIVolumeTestCase(BaseTest): db.volume_create(self.ctxt, {'display_name': 'vol3'}) # no name filter - volumes = db.volume_get_all(self.ctxt, None, None, 'created_at', - 'asc') + volumes = db.volume_get_all(self.ctxt, None, None, ['created_at'], + ['asc']) self.assertEqual(len(volumes), 3) # filter on name - volumes = db.volume_get_all(self.ctxt, None, None, 'created_at', - 'asc', {'display_name': 'vol2'}) + volumes = db.volume_get_all(self.ctxt, None, None, ['created_at'], + ['asc'], {'display_name': 'vol2'}) self.assertEqual(len(volumes), 1) self.assertEqual(volumes[0]['display_name'], 'vol2') # filter no match - volumes = db.volume_get_all(self.ctxt, None, None, 'created_at', - 'asc', {'display_name': 'vol4'}) + volumes = db.volume_get_all(self.ctxt, None, None, ['created_at'], + ['asc'], {'display_name': 'vol4'}) self.assertEqual(len(volumes), 0) def test_volume_list_by_status(self): @@ -463,47 +463,52 @@ class DBAPIVolumeTestCase(BaseTest): 'status': 'in-use'}) # no status filter - volumes = db.volume_get_all(self.ctxt, None, None, 'created_at', - 'asc') + volumes = db.volume_get_all(self.ctxt, None, None, ['created_at'], + ['asc']) self.assertEqual(len(volumes), 3) # single match - volumes = db.volume_get_all(self.ctxt, None, None, 'created_at', - 'asc', {'status': 'in-use'}) + volumes = db.volume_get_all(self.ctxt, None, None, ['created_at'], + ['asc'], {'status': 'in-use'}) self.assertEqual(len(volumes), 1) self.assertEqual(volumes[0]['status'], 'in-use') # multiple match - volumes = db.volume_get_all(self.ctxt, None, None, 'created_at', - 'asc', {'status': 'available'}) + volumes = db.volume_get_all(self.ctxt, None, None, ['created_at'], + ['asc'], {'status': 'available'}) self.assertEqual(len(volumes), 2) for volume in volumes: self.assertEqual(volume['status'], 'available') # multiple filters - volumes = db.volume_get_all(self.ctxt, None, None, 'created_at', - 'asc', {'status': 'available', - 'display_name': 'vol1'}) + volumes = db.volume_get_all(self.ctxt, None, None, ['created_at'], + ['asc'], {'status': 'available', + 'display_name': 'vol1'}) self.assertEqual(len(volumes), 1) self.assertEqual(volumes[0]['display_name'], 'vol1') self.assertEqual(volumes[0]['status'], 'available') # no match - volumes = db.volume_get_all(self.ctxt, None, None, 'created_at', - 'asc', {'status': 'in-use', - 'display_name': 'vol1'}) + volumes = db.volume_get_all(self.ctxt, None, None, ['created_at'], + ['asc'], {'status': 'in-use', + 'display_name': 'vol1'}) self.assertEqual(len(volumes), 0) def _assertEqualsVolumeOrderResult(self, correct_order, limit=None, - sort_key='created_at', sort_dir='asc', + sort_keys=None, sort_dirs=None, filters=None, project_id=None, + marker=None, match_keys=['id', 'display_name', 'volume_metadata', 'created_at']): """"Verifies that volumes are returned in the correct order.""" if project_id: - result = db.volume_get_all_by_project(self.ctxt, project_id, None, - limit, sort_key, - sort_dir, filters=filters) + result = db.volume_get_all_by_project(self.ctxt, project_id, + marker, limit, + sort_keys=sort_keys, + sort_dirs=sort_dirs, + filters=filters) else: - result = db.volume_get_all(self.ctxt, None, limit, sort_key, - sort_dir, filters=filters) + result = db.volume_get_all(self.ctxt, marker, limit, + sort_keys=sort_keys, + sort_dirs=sort_dirs, + filters=filters) self.assertEqual(len(correct_order), len(result)) for vol1, vol2 in zip(result, correct_order): for key in match_keys: @@ -517,6 +522,7 @@ class DBAPIVolumeTestCase(BaseTest): self.assertDictMatch(val1_dict, val2_dict) else: self.assertEqual(val1, val2) + return result def test_volume_get_by_filter(self): """Verifies that all filtering is done at the DB layer.""" @@ -543,7 +549,7 @@ class DBAPIVolumeTestCase(BaseTest): # By project, filter on size and name filters = {'size': '1'} - correct_order = [vols[0], vols[1]] + correct_order = [vols[1], vols[0]] self._assertEqualsVolumeOrderResult(correct_order, filters=filters, project_id='g1') filters = {'size': '1', 'display_name': 'name_1'} @@ -553,18 +559,18 @@ class DBAPIVolumeTestCase(BaseTest): # Remove project scope filters = {'size': '1'} - correct_order = [vols[0], vols[1], vols[6], vols[7]] + correct_order = [vols[7], vols[6], vols[1], vols[0]] self._assertEqualsVolumeOrderResult(correct_order, filters=filters) filters = {'size': '1', 'display_name': 'name_1'} - correct_order = [vols[1], vols[7]] + correct_order = [vols[7], vols[1]] self._assertEqualsVolumeOrderResult(correct_order, filters=filters) # Remove size constraint filters = {'display_name': 'name_1'} - correct_order = [vols[1], vols[3], vols[5]] + correct_order = [vols[5], vols[3], vols[1]] self._assertEqualsVolumeOrderResult(correct_order, filters=filters, project_id='g1') - correct_order = [vols[1], vols[3], vols[5], vols[7]] + correct_order = [vols[7], vols[5], vols[3], vols[1]] self._assertEqualsVolumeOrderResult(correct_order, filters=filters) # Verify bogus values return nothing @@ -598,7 +604,7 @@ class DBAPIVolumeTestCase(BaseTest): db.volume_admin_metadata_update(self.ctxt, vol5.id, {"readonly": "True"}, False) - vols = [vol1, vol2, vol3, vol4, vol5] + vols = [vol5, vol4, vol3, vol2, vol1] # Ensure we have 5 total instances self._assertEqualsVolumeOrderResult(vols) @@ -609,20 +615,20 @@ class DBAPIVolumeTestCase(BaseTest): # Just the test2 volumes filters = {'display_name': 'test2'} - self._assertEqualsVolumeOrderResult([vol2, vol3], filters=filters) - self._assertEqualsVolumeOrderResult([vol2], limit=1, + self._assertEqualsVolumeOrderResult([vol3, vol2], filters=filters) + self._assertEqualsVolumeOrderResult([vol3], limit=1, filters=filters) - self._assertEqualsVolumeOrderResult([vol2, vol3], limit=2, + self._assertEqualsVolumeOrderResult([vol3, vol2], limit=2, filters=filters) - self._assertEqualsVolumeOrderResult([vol2, vol3], limit=100, + self._assertEqualsVolumeOrderResult([vol3, vol2], limit=100, filters=filters) # metadata filters filters = {'metadata': {'key1': 'val1'}} - self._assertEqualsVolumeOrderResult([vol3, vol4], filters=filters) - self._assertEqualsVolumeOrderResult([vol3], limit=1, + self._assertEqualsVolumeOrderResult([vol4, vol3], filters=filters) + self._assertEqualsVolumeOrderResult([vol4], limit=1, filters=filters) - self._assertEqualsVolumeOrderResult([vol3, vol4], limit=10, + self._assertEqualsVolumeOrderResult([vol4, vol3], limit=10, filters=filters) filters = {'metadata': {'readonly': 'True'}} @@ -675,22 +681,160 @@ class DBAPIVolumeTestCase(BaseTest): 'migration_status': 'btarget:'}) vol4 = db.volume_create(self.ctxt, {'display_name': 'test4', 'migration_status': 'target:'}) - vols = [vol1, vol2, vol3, vol4] - # Ensure we have 4 total instances - self._assertEqualsVolumeOrderResult(vols) + # Ensure we have 4 total instances, default sort of created_at (desc) + self._assertEqualsVolumeOrderResult([vol4, vol3, vol2, vol1]) # Apply the unique filter filters = {'no_migration_targets': True} - self._assertEqualsVolumeOrderResult([vol1, vol2, vol3], + self._assertEqualsVolumeOrderResult([vol3, vol2, vol1], filters=filters) - self._assertEqualsVolumeOrderResult([vol1, vol2], limit=2, + self._assertEqualsVolumeOrderResult([vol3, vol2], limit=2, filters=filters) filters = {'no_migration_targets': True, 'display_name': 'test4'} self._assertEqualsVolumeOrderResult([], filters=filters) + def test_volume_get_all_by_filters_sort_keys(self): + # Volumes that will reply to the query + test_h1_avail = db.volume_create(self.ctxt, {'display_name': 'test', + 'status': 'available', + 'host': 'h1'}) + test_h1_error = db.volume_create(self.ctxt, {'display_name': 'test', + 'status': 'error', + 'host': 'h1'}) + test_h1_error2 = db.volume_create(self.ctxt, {'display_name': 'test', + 'status': 'error', + 'host': 'h1'}) + test_h2_avail = db.volume_create(self.ctxt, {'display_name': 'test', + 'status': 'available', + 'host': 'h2'}) + test_h2_error = db.volume_create(self.ctxt, {'display_name': 'test', + 'status': 'error', + 'host': 'h2'}) + test_h2_error2 = db.volume_create(self.ctxt, {'display_name': 'test', + 'status': 'error', + 'host': 'h2'}) + # Other volumes in the DB, will not match name filter + other_error = db.volume_create(self.ctxt, {'display_name': 'other', + 'status': 'error', + 'host': 'a'}) + other_active = db.volume_create(self.ctxt, {'display_name': 'other', + 'status': 'available', + 'host': 'a'}) + filters = {'display_name': 'test'} + + # Verify different sort key/direction combinations + sort_keys = ['host', 'status', 'created_at'] + sort_dirs = ['asc', 'asc', 'asc'] + correct_order = [test_h1_avail, test_h1_error, test_h1_error2, + test_h2_avail, test_h2_error, test_h2_error2] + self._assertEqualsVolumeOrderResult(correct_order, filters=filters, + sort_keys=sort_keys, + sort_dirs=sort_dirs) + + sort_dirs = ['asc', 'desc', 'asc'] + correct_order = [test_h1_error, test_h1_error2, test_h1_avail, + test_h2_error, test_h2_error2, test_h2_avail] + self._assertEqualsVolumeOrderResult(correct_order, filters=filters, + sort_keys=sort_keys, + sort_dirs=sort_dirs) + + sort_dirs = ['desc', 'desc', 'asc'] + correct_order = [test_h2_error, test_h2_error2, test_h2_avail, + test_h1_error, test_h1_error2, test_h1_avail] + self._assertEqualsVolumeOrderResult(correct_order, filters=filters, + sort_keys=sort_keys, + sort_dirs=sort_dirs) + + # created_at is added by default if not supplied, descending order + sort_keys = ['host', 'status'] + sort_dirs = ['desc', 'desc'] + correct_order = [test_h2_error2, test_h2_error, test_h2_avail, + test_h1_error2, test_h1_error, test_h1_avail] + self._assertEqualsVolumeOrderResult(correct_order, filters=filters, + sort_keys=sort_keys, + sort_dirs=sort_dirs) + + sort_dirs = ['asc', 'asc'] + correct_order = [test_h1_avail, test_h1_error, test_h1_error2, + test_h2_avail, test_h2_error, test_h2_error2] + self._assertEqualsVolumeOrderResult(correct_order, filters=filters, + sort_keys=sort_keys, + sort_dirs=sort_dirs) + + # Remove name filter + correct_order = [other_active, other_error, + test_h1_avail, test_h1_error, test_h1_error2, + test_h2_avail, test_h2_error, test_h2_error2] + self._assertEqualsVolumeOrderResult(correct_order, sort_keys=sort_keys, + sort_dirs=sort_dirs) + + # No sort data, default sort of created_at, id (desc) + correct_order = [other_active, other_error, + test_h2_error2, test_h2_error, test_h2_avail, + test_h1_error2, test_h1_error, test_h1_avail] + self._assertEqualsVolumeOrderResult(correct_order) + + def test_volume_get_all_by_filters_sort_keys_paginate(self): + '''Verifies sort order with pagination.''' + # Volumes that will reply to the query + test1_avail = db.volume_create(self.ctxt, {'display_name': 'test', + 'size': 1, + 'status': 'available'}) + test1_error = db.volume_create(self.ctxt, {'display_name': 'test', + 'size': 1, + 'status': 'error'}) + test1_error2 = db.volume_create(self.ctxt, {'display_name': 'test', + 'size': 1, + 'status': 'error'}) + test2_avail = db.volume_create(self.ctxt, {'display_name': 'test', + 'size': 2, + 'status': 'available'}) + test2_error = db.volume_create(self.ctxt, {'display_name': 'test', + 'size': 2, + 'status': 'error'}) + test2_error2 = db.volume_create(self.ctxt, {'display_name': 'test', + 'size': 2, + 'status': 'error'}) + + # Other volumes in the DB, will not match name filter + db.volume_create(self.ctxt, {'display_name': 'other'}) + db.volume_create(self.ctxt, {'display_name': 'other'}) + filters = {'display_name': 'test'} + # Common sort information for every query + sort_keys = ['size', 'status', 'created_at'] + sort_dirs = ['asc', 'desc', 'asc'] + # Overall correct volume order based on the sort keys + correct_order = [test1_error, test1_error2, test1_avail, + test2_error, test2_error2, test2_avail] + + # Limits of 1, 2, and 3, verify that the volumes returned are in the + # correct sorted order, update the marker to get the next correct page + for limit in range(1, 4): + marker = None + # Include the maximum number of volumes (ie, 6) to ensure that + # the last query (with marker pointing to the last volume) + # returns 0 servers + for i in range(0, 7, limit): + if i == len(correct_order): + correct = [] + else: + correct = correct_order[i:i + limit] + vols = self._assertEqualsVolumeOrderResult( + correct, filters=filters, + sort_keys=sort_keys, sort_dirs=sort_dirs, + limit=limit, marker=marker) + if correct: + marker = vols[-1]['id'] + self.assertEqual(correct[-1]['id'], marker) + + def test_volume_get_all_invalid_sort_key(self): + for keys in (['foo'], ['display_name', 'foo']): + self.assertRaises(exception.InvalidInput, db.volume_get_all, + self.ctxt, None, None, sort_keys=keys) + def test_volume_get_iscsi_target_num(self): db.iscsi_target_create_safe(self.ctxt, {'volume_id': 42, 'target_num': 43}) diff --git a/cinder/volume/api.py b/cinder/volume/api.py index 35ec0b86f04..6db92b7017c 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -375,8 +375,8 @@ class API(base.Base): return b - def get_all(self, context, marker=None, limit=None, sort_key='created_at', - sort_dir='desc', filters=None, viewable_admin_meta=False): + def get_all(self, context, marker=None, limit=None, sort_keys=None, + sort_dirs=None, filters=None, viewable_admin_meta=False): check_policy(context, 'get_all') if filters is None: @@ -407,15 +407,18 @@ class API(base.Base): if context.is_admin and allTenants: # Need to remove all_tenants to pass the filtering below. del filters['all_tenants'] - volumes = self.db.volume_get_all(context, marker, limit, sort_key, - sort_dir, filters=filters) + volumes = self.db.volume_get_all(context, marker, limit, + sort_keys=sort_keys, + sort_dirs=sort_dirs, + filters=filters) else: if viewable_admin_meta: context = context.elevated() volumes = self.db.volume_get_all_by_project(context, context.project_id, marker, limit, - sort_key, sort_dir, + sort_keys=sort_keys, + sort_dirs=sort_dirs, filters=filters) return volumes diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index 5033c0689ba..c9c2813a86c 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -258,8 +258,7 @@ class VolumeManager(manager.SchedulerDependentManager): :param ctxt: our working context """ - vol_entries = self.db.volume_get_all(ctxt, None, 1, 'created_at', - None, filters=None) + vol_entries = self.db.volume_get_all(ctxt, None, 1, filters=None) if len(vol_entries) == 0: LOG.info(_LI("Determined volume DB was empty at startup."))