From 4a230e04eaeb8041f64b46d00497aab0614a8219 Mon Sep 17 00:00:00 2001 From: wanghao Date: Wed, 22 Jul 2015 16:30:00 +0800 Subject: [PATCH] Add pagination to backups Backups list/detail does not support pagination now. Add pagination support like volume does by using marker, limit, sort_key and sort_order. Partial-Implements: blueprint extend-limit-implementations DocImpact APIImpact Use marker, limit, etc. in list url like volume does. (Pulled from gate, cinder can no longer pass unit tests) Change-Id: I33dbdc34c61f78eab2a78a9cda08780068867a03 --- cinder/api/contrib/backups.py | 21 ++- cinder/api/views/backups.py | 14 +- cinder/backup/api.py | 14 +- cinder/db/api.py | 16 +- cinder/db/sqlalchemy/api.py | 53 ++++-- cinder/objects/backup.py | 14 +- cinder/tests/unit/api/contrib/test_backups.py | 156 ++++++++++++++++-- cinder/tests/unit/test_backup.py | 11 +- cinder/tests/unit/test_cmd.py | 3 +- 9 files changed, 243 insertions(+), 59 deletions(-) diff --git a/cinder/api/contrib/backups.py b/cinder/api/contrib/backups.py index c4f089808ab..8be8b1fb570 100644 --- a/cinder/api/contrib/backups.py +++ b/cinder/api/contrib/backups.py @@ -210,6 +210,8 @@ class BackupsController(wsgi.Controller): """Returns a list of backups, transformed through view builder.""" context = req.environ['cinder.context'] filters = req.params.copy() + marker, limit, offset = common.get_pagination_params(filters) + sort_keys, sort_dirs = common.get_sort_params(filters) utils.remove_invalid_filter_options(context, filters, @@ -219,17 +221,20 @@ class BackupsController(wsgi.Controller): filters['display_name'] = filters['name'] del filters['name'] - backups = self.backup_api.get_all(context, search_opts=filters) - backup_count = len(backups) - limited_list = common.limited(backups.objects, req) - req.cache_db_backups(limited_list) + backups = self.backup_api.get_all(context, search_opts=filters, + marker=marker, + limit=limit, + offset=offset, + sort_keys=sort_keys, + sort_dirs=sort_dirs, + ) + + req.cache_db_backups(backups.objects) if is_detail: - backups = self._view_builder.detail_list(req, limited_list, - backup_count) + backups = self._view_builder.detail_list(req, backups.objects) else: - backups = self._view_builder.summary_list(req, limited_list, - backup_count) + backups = self._view_builder.summary_list(req, backups.objects) return backups # TODO(frankm): Add some checks here including diff --git a/cinder/api/views/backups.py b/cinder/api/views/backups.py index 24375a7051f..91dc4fa5cdb 100644 --- a/cinder/api/views/backups.py +++ b/cinder/api/views/backups.py @@ -30,15 +30,13 @@ class ViewBuilder(common.ViewBuilder): """Initialize view builder.""" super(ViewBuilder, self).__init__() - def summary_list(self, request, backups, origin_backup_count): + def summary_list(self, request, backups, backup_count=None): """Show a list of backups without many details.""" - return self._list_view(self.summary, request, backups, - origin_backup_count) + return self._list_view(self.summary, request, backups, backup_count) - def detail_list(self, request, backups, origin_backup_count): + def detail_list(self, request, backups, backup_count=None): """Detailed view of a list of backups .""" - return self._list_view(self.detail, request, backups, - origin_backup_count) + return self._list_view(self.detail, request, backups, backup_count) def summary(self, request, backup): """Generic, non-detailed view of a backup.""" @@ -82,13 +80,13 @@ class ViewBuilder(common.ViewBuilder): } } - def _list_view(self, func, request, backups, origin_backup_count): + def _list_view(self, func, request, backups, backup_count): """Provide a view for a list of backups.""" backups_list = [func(request, backup)['backup'] for backup in backups] backups_links = self._get_collection_links(request, backups, self._collection_name, - origin_backup_count) + backup_count) backups_dict = dict(backups=backups_list) if backups_links: diff --git a/cinder/backup/api.py b/cinder/backup/api.py index ddfa871a4b4..ca2349da8b3 100644 --- a/cinder/backup/api.py +++ b/cinder/backup/api.py @@ -90,7 +90,7 @@ class API(base.Base): # Don't allow backup to be deleted if there are incremental # backups dependent on it. - deltas = self.get_all(context, {'parent_id': backup.id}) + deltas = self.get_all(context, search_opts={'parent_id': backup.id}) if deltas and len(deltas): msg = _('Incremental backups exist for this backup.') raise exception.InvalidBackup(reason=msg) @@ -99,7 +99,8 @@ class API(base.Base): backup.save() self.backup_rpcapi.delete_backup(context, backup) - def get_all(self, context, search_opts=None): + def get_all(self, context, search_opts=None, marker=None, limit=None, + offset=None, sort_keys=None, sort_dirs=None): check_policy(context, 'get_all') search_opts = search_opts or {} @@ -110,12 +111,13 @@ class API(base.Base): raise exception.InvalidParameterValue(err=msg) if context.is_admin and strutils.bool_from_string(all_tenants): - backups = objects.BackupList.get_all(context, filters=search_opts) + backups = objects.BackupList.get_all(context, search_opts, + marker, limit, offset, + sort_keys, sort_dirs) else: backups = objects.BackupList.get_all_by_project( - context, - context.project_id, - filters=search_opts + context, context.project_id, search_opts, + marker, limit, offset, sort_keys, sort_dirs ) return backups diff --git a/cinder/db/api.py b/cinder/db/api.py index c0c51b23d08..abf370f410d 100644 --- a/cinder/db/api.py +++ b/cinder/db/api.py @@ -853,9 +853,12 @@ def backup_get(context, backup_id): return IMPL.backup_get(context, backup_id) -def backup_get_all(context, filters=None): +def backup_get_all(context, filters=None, marker=None, limit=None, + offset=None, sort_keys=None, sort_dirs=None): """Get all backups.""" - return IMPL.backup_get_all(context, filters=filters) + return IMPL.backup_get_all(context, filters=filters, marker=marker, + limit=limit, offset=offset, sort_keys=sort_keys, + sort_dirs=sort_dirs) def backup_get_all_by_host(context, host): @@ -868,10 +871,15 @@ def backup_create(context, values): return IMPL.backup_create(context, values) -def backup_get_all_by_project(context, project_id, filters=None): +def backup_get_all_by_project(context, project_id, filters=None, marker=None, + limit=None, offset=None, sort_keys=None, + sort_dirs=None): """Get all backups belonging to a project.""" return IMPL.backup_get_all_by_project(context, project_id, - filters=filters) + filters=filters, marker=marker, + limit=limit, offset=offset, + sort_keys=sort_keys, + sort_dirs=sort_dirs) def backup_get_all_by_volume(context, volume_id, filters=None): diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index f485c395c8a..da01427ff14 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -3417,9 +3417,12 @@ def volume_glance_metadata_delete_by_snapshot(context, snapshot_id): @require_context def backup_get(context, backup_id): - result = model_query(context, models.Backup, project_only=True).\ - filter_by(id=backup_id).\ - first() + return _backup_get(context, backup_id) + + +def _backup_get(context, backup_id, session=None): + result = model_query(context, models.Backup, session=session, + project_only=True).filter_by(id=backup_id).first() if not result: raise exception.BackupNotFound(backup_id=backup_id) @@ -3427,23 +3430,41 @@ def backup_get(context, backup_id): return result -def _backup_get_all(context, filters=None): +def _backup_get_all(context, filters=None, marker=None, limit=None, + offset=None, sort_keys=None, sort_dirs=None): if filters and not is_valid_model_filters(models.Backup, filters): return [] session = get_session() with session.begin(): - # Generate the query - query = model_query(context, models.Backup) - if filters: - query = query.filter_by(**filters) - + # Generate the paginate query + query = _generate_paginate_query(context, session, marker, + limit, sort_keys, sort_dirs, filters, + offset, models.Backup) + if query is None: + return [] return query.all() +def _backups_get_query(context, session=None, project_only=False): + return model_query(context, models.Backup, session=session, + project_only=project_only) + + +def _process_backups_filters(query, filters): + if filters: + # Ensure that filters' keys exist on the model + if not is_valid_model_filters(models.Backup, filters): + return + query = query.filter_by(**filters) + return query + + @require_admin_context -def backup_get_all(context, filters=None): - return _backup_get_all(context, filters) +def backup_get_all(context, filters=None, marker=None, limit=None, + offset=None, sort_keys=None, sort_dirs=None): + return _backup_get_all(context, filters, marker, limit, offset, sort_keys, + sort_dirs) @require_admin_context @@ -3452,7 +3473,9 @@ def backup_get_all_by_host(context, host): @require_context -def backup_get_all_by_project(context, project_id, filters=None): +def backup_get_all_by_project(context, project_id, filters=None, marker=None, + limit=None, offset=None, sort_keys=None, + sort_dirs=None): authorize_project_context(context, project_id) if not filters: @@ -3462,7 +3485,8 @@ def backup_get_all_by_project(context, project_id, filters=None): filters['project_id'] = project_id - return _backup_get_all(context, filters) + return _backup_get_all(context, filters, marker, limit, offset, sort_keys, + sort_dirs) @require_context @@ -3963,5 +3987,6 @@ def driver_initiator_data_get(context, initiator, namespace): PAGINATION_HELPERS = { models.Volume: (_volume_get_query, _process_volume_filters, _volume_get), - models.Snapshot: (_snaps_get_query, _process_snaps_filters, _snapshot_get) + models.Snapshot: (_snaps_get_query, _process_snaps_filters, _snapshot_get), + models.Backup: (_backups_get_query, _process_backups_filters, _backup_get) } diff --git a/cinder/objects/backup.py b/cinder/objects/backup.py index 5a20c7840b8..7f400ca6d31 100644 --- a/cinder/objects/backup.py +++ b/cinder/objects/backup.py @@ -165,8 +165,10 @@ class BackupList(base.ObjectListBase, base.CinderObject): } @base.remotable_classmethod - def get_all(cls, context, filters=None): - backups = db.backup_get_all(context, filters) + def get_all(cls, context, filters=None, marker=None, limit=None, + offset=None, sort_keys=None, sort_dirs=None): + backups = db.backup_get_all(context, filters, marker, limit, offset, + sort_keys, sort_dirs) return base.obj_make_list(context, cls(context), objects.Backup, backups) @@ -177,8 +179,12 @@ class BackupList(base.ObjectListBase, base.CinderObject): backups) @base.remotable_classmethod - def get_all_by_project(cls, context, project_id, filters=None): - backups = db.backup_get_all_by_project(context, project_id, filters) + def get_all_by_project(cls, context, project_id, filters=None, + marker=None, limit=None, offset=None, + sort_keys=None, sort_dirs=None): + backups = db.backup_get_all_by_project(context, project_id, filters, + marker, limit, offset, + sort_keys, sort_dirs) return base.obj_make_list(context, cls(context), objects.Backup, backups) diff --git a/cinder/tests/unit/api/contrib/test_backups.py b/cinder/tests/unit/api/contrib/test_backups.py index 9ed7e9a98b0..c0827c3ffe2 100644 --- a/cinder/tests/unit/api/contrib/test_backups.py +++ b/cinder/tests/unit/api/contrib/test_backups.py @@ -156,13 +156,13 @@ class BackupsAPITestCase(test.TestCase): self.assertEqual(200, res.status_int) self.assertEqual(3, len(res_dict['backups'][0])) - self.assertEqual(backup_id1, res_dict['backups'][0]['id']) + self.assertEqual(backup_id3, res_dict['backups'][0]['id']) self.assertEqual('test_backup', res_dict['backups'][0]['name']) self.assertEqual(3, len(res_dict['backups'][1])) self.assertEqual(backup_id2, res_dict['backups'][1]['id']) self.assertEqual('test_backup', res_dict['backups'][1]['name']) self.assertEqual(3, len(res_dict['backups'][2])) - self.assertEqual(backup_id3, res_dict['backups'][2]['id']) + self.assertEqual(backup_id1, res_dict['backups'][2]['id']) self.assertEqual('test_backup', res_dict['backups'][2]['name']) db.backup_destroy(context.get_admin_context(), backup_id3) @@ -185,19 +185,89 @@ class BackupsAPITestCase(test.TestCase): backup_list = dom.getElementsByTagName('backup') self.assertEqual(2, backup_list.item(0).attributes.length) - self.assertEqual(backup_id1, + self.assertEqual(backup_id3, backup_list.item(0).getAttribute('id')) self.assertEqual(2, backup_list.item(1).attributes.length) self.assertEqual(backup_id2, backup_list.item(1).getAttribute('id')) self.assertEqual(2, backup_list.item(2).attributes.length) - self.assertEqual(backup_id3, + self.assertEqual(backup_id1, backup_list.item(2).getAttribute('id')) db.backup_destroy(context.get_admin_context(), backup_id3) db.backup_destroy(context.get_admin_context(), backup_id2) db.backup_destroy(context.get_admin_context(), backup_id1) + def test_list_backups_with_limit(self): + backup_id1 = self._create_backup() + backup_id2 = self._create_backup() + backup_id3 = self._create_backup() + + req = webob.Request.blank('/v2/fake/backups?limit=2') + req.method = 'GET' + req.headers['Content-Type'] = 'application/json' + res = req.get_response(fakes.wsgi_app()) + res_dict = json.loads(res.body) + + self.assertEqual(200, res.status_int) + self.assertEqual(2, len(res_dict['backups'])) + self.assertEqual(3, len(res_dict['backups'][0])) + self.assertEqual(backup_id3, res_dict['backups'][0]['id']) + self.assertEqual('test_backup', res_dict['backups'][0]['name']) + self.assertEqual(3, len(res_dict['backups'][1])) + self.assertEqual(backup_id2, res_dict['backups'][1]['id']) + self.assertEqual('test_backup', res_dict['backups'][1]['name']) + + db.backup_destroy(context.get_admin_context(), backup_id3) + db.backup_destroy(context.get_admin_context(), backup_id2) + db.backup_destroy(context.get_admin_context(), backup_id1) + + def test_list_backups_with_marker(self): + backup_id1 = self._create_backup() + backup_id2 = self._create_backup() + backup_id3 = self._create_backup() + url = ('/v2/fake/backups?marker=%s' % backup_id3) + req = webob.Request.blank(url) + req.method = 'GET' + req.headers['Content-Type'] = 'application/json' + res = req.get_response(fakes.wsgi_app()) + res_dict = json.loads(res.body) + + self.assertEqual(200, res.status_int) + self.assertEqual(2, len(res_dict['backups'])) + self.assertEqual(3, len(res_dict['backups'][0])) + self.assertEqual(backup_id2, res_dict['backups'][0]['id']) + self.assertEqual('test_backup', res_dict['backups'][0]['name']) + self.assertEqual(3, len(res_dict['backups'][1])) + self.assertEqual(backup_id1, res_dict['backups'][1]['id']) + self.assertEqual('test_backup', res_dict['backups'][1]['name']) + + db.backup_destroy(context.get_admin_context(), backup_id3) + db.backup_destroy(context.get_admin_context(), backup_id2) + db.backup_destroy(context.get_admin_context(), backup_id1) + + def test_list_backups_with_limit_and_marker(self): + backup_id1 = self._create_backup() + backup_id2 = self._create_backup() + backup_id3 = self._create_backup() + + url = ('/v2/fake/backups?limit=1&marker=%s' % backup_id3) + req = webob.Request.blank(url) + req.method = 'GET' + req.headers['Content-Type'] = 'application/json' + res = req.get_response(fakes.wsgi_app()) + res_dict = json.loads(res.body) + + self.assertEqual(200, res.status_int) + self.assertEqual(1, len(res_dict['backups'])) + self.assertEqual(3, len(res_dict['backups'][0])) + self.assertEqual(backup_id2, res_dict['backups'][0]['id']) + self.assertEqual('test_backup', res_dict['backups'][0]['name']) + + db.backup_destroy(context.get_admin_context(), backup_id3) + db.backup_destroy(context.get_admin_context(), backup_id2) + db.backup_destroy(context.get_admin_context(), backup_id1) + def test_list_backups_detail_json(self): backup_id1 = self._create_backup() backup_id2 = self._create_backup() @@ -219,7 +289,7 @@ class BackupsAPITestCase(test.TestCase): res_dict['backups'][0]['description']) self.assertEqual('test_backup', res_dict['backups'][0]['name']) - self.assertEqual(backup_id1, res_dict['backups'][0]['id']) + self.assertEqual(backup_id3, res_dict['backups'][0]['id']) self.assertEqual(0, res_dict['backups'][0]['object_count']) self.assertEqual(0, res_dict['backups'][0]['size']) self.assertEqual('creating', res_dict['backups'][0]['status']) @@ -241,13 +311,12 @@ class BackupsAPITestCase(test.TestCase): self.assertEqual(14, len(res_dict['backups'][2])) self.assertEqual('az1', res_dict['backups'][2]['availability_zone']) - self.assertEqual('volumebackups', - res_dict['backups'][2]['container']) + self.assertEqual('volumebackups', res_dict['backups'][2]['container']) self.assertEqual('this is a test backup', res_dict['backups'][2]['description']) self.assertEqual('test_backup', res_dict['backups'][2]['name']) - self.assertEqual(backup_id3, res_dict['backups'][2]['id']) + self.assertEqual(backup_id1, res_dict['backups'][2]['id']) self.assertEqual(0, res_dict['backups'][2]['object_count']) self.assertEqual(0, res_dict['backups'][2]['size']) self.assertEqual('creating', res_dict['backups'][2]['status']) @@ -325,7 +394,7 @@ class BackupsAPITestCase(test.TestCase): self.assertEqual( 'test_backup', backup_detail.item(0).getAttribute('name')) self.assertEqual( - backup_id1, backup_detail.item(0).getAttribute('id')) + backup_id3, backup_detail.item(0).getAttribute('id')) self.assertEqual( 0, int(backup_detail.item(0).getAttribute('object_count'))) self.assertEqual( @@ -367,7 +436,7 @@ class BackupsAPITestCase(test.TestCase): self.assertEqual( 'test_backup', backup_detail.item(2).getAttribute('name')) self.assertEqual( - backup_id3, backup_detail.item(2).getAttribute('id')) + backup_id1, backup_detail.item(2).getAttribute('id')) self.assertEqual( 0, int(backup_detail.item(2).getAttribute('object_count'))) self.assertEqual( @@ -381,6 +450,73 @@ class BackupsAPITestCase(test.TestCase): db.backup_destroy(context.get_admin_context(), backup_id2) db.backup_destroy(context.get_admin_context(), backup_id1) + def test_list_backups_detail_with_limit_and_sort_args(self): + backup_id1 = self._create_backup() + backup_id2 = self._create_backup() + backup_id3 = self._create_backup() + url = ('/v2/fake/backups/detail?limit=2&sort_key=created_at' + '&sort_dir=desc') + req = webob.Request.blank(url) + req.method = 'GET' + req.headers['Content-Type'] = 'application/json' + res = req.get_response(fakes.wsgi_app()) + res_dict = json.loads(res.body) + + self.assertEqual(200, res.status_int) + self.assertEqual(2, len(res_dict['backups'])) + self.assertEqual(14, len(res_dict['backups'][0])) + self.assertEqual(backup_id3, res_dict['backups'][0]['id']) + self.assertEqual(14, len(res_dict['backups'][1])) + self.assertEqual(backup_id2, res_dict['backups'][1]['id']) + + db.backup_destroy(context.get_admin_context(), backup_id3) + db.backup_destroy(context.get_admin_context(), backup_id2) + db.backup_destroy(context.get_admin_context(), backup_id1) + + def test_list_backups_detail_with_marker(self): + backup_id1 = self._create_backup() + backup_id2 = self._create_backup() + backup_id3 = self._create_backup() + + url = ('/v2/fake/backups/detail?marker=%s' % backup_id3) + req = webob.Request.blank(url) + req.method = 'GET' + req.headers['Content-Type'] = 'application/json' + res = req.get_response(fakes.wsgi_app()) + res_dict = json.loads(res.body) + + self.assertEqual(200, res.status_int) + self.assertEqual(2, len(res_dict['backups'])) + self.assertEqual(14, len(res_dict['backups'][0])) + self.assertEqual(backup_id2, res_dict['backups'][0]['id']) + self.assertEqual(14, len(res_dict['backups'][1])) + self.assertEqual(backup_id1, res_dict['backups'][1]['id']) + + db.backup_destroy(context.get_admin_context(), backup_id3) + db.backup_destroy(context.get_admin_context(), backup_id2) + db.backup_destroy(context.get_admin_context(), backup_id1) + + def test_list_backups_detail_with_limit_and_marker(self): + backup_id1 = self._create_backup() + backup_id2 = self._create_backup() + backup_id3 = self._create_backup() + + url = ('/v2/fake/backups/detail?limit=1&marker=%s' % backup_id3) + req = webob.Request.blank(url) + req.method = 'GET' + req.headers['Content-Type'] = 'application/json' + res = req.get_response(fakes.wsgi_app()) + res_dict = json.loads(res.body) + + self.assertEqual(200, res.status_int) + self.assertEqual(1, len(res_dict['backups'])) + self.assertEqual(14, len(res_dict['backups'][0])) + self.assertEqual(backup_id2, res_dict['backups'][0]['id']) + + db.backup_destroy(context.get_admin_context(), backup_id3) + db.backup_destroy(context.get_admin_context(), backup_id2) + db.backup_destroy(context.get_admin_context(), backup_id1) + @mock.patch('cinder.db.service_get_all_by_topic') @mock.patch( 'cinder.api.openstack.wsgi.Controller.validate_name_and_description') diff --git a/cinder/tests/unit/test_backup.py b/cinder/tests/unit/test_backup.py index 1c614e96a1e..4ae6cbb5f4c 100644 --- a/cinder/tests/unit/test_backup.py +++ b/cinder/tests/unit/test_backup.py @@ -920,7 +920,8 @@ class BackupAPITestCase(BaseBackupTest): self.assertEqual(mock_backuplist.get_all_by_project.return_value, result) mock_backuplist.get_all_by_project.assert_called_once_with( - self.ctxt, self.ctxt.project_id, filters={'key': 'value'}) + self.ctxt, self.ctxt.project_id, {'key': 'value'}, None, None, + None, None, None) @mock.patch.object(objects, 'BackupList') @ddt.data(False, 'false', '0', 0, 'no') @@ -932,7 +933,8 @@ class BackupAPITestCase(BaseBackupTest): self.assertEqual(mock_backuplist.get_all_by_project.return_value, result) mock_backuplist.get_all_by_project.assert_called_once_with( - self.ctxt, self.ctxt.project_id, filters={'key': 'value'}) + self.ctxt, self.ctxt.project_id, {'key': 'value'}, None, None, + None, None, None) @mock.patch.object(objects, 'BackupList') @ddt.data(True, 'true', '1', 1, 'yes') @@ -944,7 +946,7 @@ class BackupAPITestCase(BaseBackupTest): self.assertEqual(mock_backuplist.get_all.return_value, result) mock_backuplist.get_all.assert_called_once_with( - self.ctxt, filters={'key': 'value'}) + self.ctxt, {'key': 'value'}, None, None, None, None, None) @mock.patch.object(objects, 'BackupList') def test_get_all_true_value_all_tenants_non_admin(self, mock_backuplist): @@ -955,4 +957,5 @@ class BackupAPITestCase(BaseBackupTest): self.assertEqual(mock_backuplist.get_all_by_project.return_value, result) mock_backuplist.get_all_by_project.assert_called_once_with( - ctxt, ctxt.project_id, filters={'key': 'value'}) + ctxt, ctxt.project_id, {'key': 'value'}, None, None, None, None, + None) diff --git a/cinder/tests/unit/test_cmd.py b/cinder/tests/unit/test_cmd.py index f3d9ff2199b..ce48cc0985d 100644 --- a/cinder/tests/unit/test_cmd.py +++ b/cinder/tests/unit/test_cmd.py @@ -614,7 +614,8 @@ class TestCinderManageCmd(test.TestCase): backup_cmds.list() get_admin_context.assert_called_once_with() - backup_get_all.assert_called_once_with(ctxt, None) + backup_get_all.assert_called_once_with(ctxt, None, None, None, + None, None, None) self.assertEqual(expected_out, fake_out.getvalue()) @mock.patch('cinder.utils.service_is_up')