From: Yuriy Nesenenko Date: Tue, 19 May 2015 15:17:01 +0000 (+0300) Subject: Filter snapshots data on the DB side X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=f5e77c982df2818aee113676e195d4aa7ba6c23d;p=openstack-build%2Fcinder-build.git Filter snapshots data on the DB side It filters snapshots data on the DB side to improve performance. Some tests are removed from test_snapshots.py because their function implemented on the DB side. Partial-Implements: blueprint data-filtering-on-the-db-side Change-Id: I1c3e47a4f3780de54d7a5920f463e26cc8b7bcc2 --- diff --git a/cinder/db/api.py b/cinder/db/api.py index c82689bc5..45a63b318 100644 --- a/cinder/db/api.py +++ b/cinder/db/api.py @@ -285,14 +285,14 @@ def snapshot_get(context, snapshot_id): return IMPL.snapshot_get(context, snapshot_id) -def snapshot_get_all(context): +def snapshot_get_all(context, filters=None): """Get all snapshots.""" - return IMPL.snapshot_get_all(context) + return IMPL.snapshot_get_all(context, filters) -def snapshot_get_all_by_project(context, project_id): +def snapshot_get_all_by_project(context, project_id, filters=None): """Get all snapshots belonging to a project.""" - return IMPL.snapshot_get_all_by_project(context, project_id) + return IMPL.snapshot_get_all_by_project(context, project_id, filters) def snapshot_get_by_host(context, host, filters=None): diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index b0536c889..7dc908013 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -1974,10 +1974,22 @@ def snapshot_get(context, snapshot_id): @require_admin_context -def snapshot_get_all(context): - return model_query(context, models.Snapshot).\ - options(joinedload('snapshot_metadata')).\ - all() +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 [] + + query = model_query(context, models.Snapshot) + + if filters: + query = query.filter_by(**filters) + + return query.options(joinedload('snapshot_metadata')).all() @require_context @@ -2012,12 +2024,15 @@ def snapshot_get_all_for_cgsnapshot(context, cgsnapshot_id): @require_context -def snapshot_get_all_by_project(context, project_id): +def snapshot_get_all_by_project(context, project_id, filters=None): authorize_project_context(context, project_id) - return model_query(context, models.Snapshot).\ - filter_by(project_id=project_id).\ - options(joinedload('snapshot_metadata')).\ - all() + query = model_query(context, models.Snapshot) + + if filters: + query = query.filter_by(**filters) + + return query.filter_by(project_id=project_id).\ + options(joinedload('snapshot_metadata')).all() @require_context diff --git a/cinder/objects/snapshot.py b/cinder/objects/snapshot.py index d266a756a..8c561358c 100644 --- a/cinder/objects/snapshot.py +++ b/cinder/objects/snapshot.py @@ -211,8 +211,8 @@ class SnapshotList(base.ObjectListBase, base.CinderObject): } @base.remotable_classmethod - def get_all(cls, context): - snapshots = db.snapshot_get_all(context) + def get_all(cls, context, search_opts): + snapshots = db.snapshot_get_all(context, search_opts) return base.obj_make_list(context, cls(), objects.Snapshot, snapshots, expected_attrs=['metadata']) @@ -224,8 +224,9 @@ class SnapshotList(base.ObjectListBase, base.CinderObject): snapshots, expected_attrs=['metadata']) @base.remotable_classmethod - def get_all_by_project(cls, context, project_id): - snapshots = db.snapshot_get_all_by_project(context, project_id) + def get_all_by_project(cls, context, project_id, search_opts): + snapshots = db.snapshot_get_all_by_project(context, project_id, + search_opts) return base.obj_make_list(context, cls(context), objects.Snapshot, snapshots, expected_attrs=['metadata']) diff --git a/cinder/tests/unit/api/v1/stubs.py b/cinder/tests/unit/api/v1/stubs.py index 764bfeec1..2f1d40bb6 100644 --- a/cinder/tests/unit/api/v1/stubs.py +++ b/cinder/tests/unit/api/v1/stubs.py @@ -118,13 +118,13 @@ def stub_snapshot(id, **kwargs): return snapshot -def stub_snapshot_get_all(self): +def stub_snapshot_get_all(self, search_opts=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): +def stub_snapshot_get_all_by_project(self, context, search_opts=None): return [stub_snapshot(1)] diff --git a/cinder/tests/unit/api/v1/test_snapshots.py b/cinder/tests/unit/api/v1/test_snapshots.py index 3d498412d..fd4b7cc71 100644 --- a/cinder/tests/unit/api/v1/test_snapshots.py +++ b/cinder/tests/unit/api/v1/test_snapshots.py @@ -311,95 +311,6 @@ class SnapshotApiTest(test.TestCase): resp_snapshot = resp_snapshots.pop() self.assertEqual(resp_snapshot['id'], UUID) - @mock.patch('cinder.db.snapshot_metadata_get', return_value=dict()) - def test_snapshot_list_by_status(self, snapshot_metadata_get): - def stub_snapshot_get_all_by_project(context, project_id): - return [ - stubs.stub_snapshot(1, display_name='backup1', - status='available'), - stubs.stub_snapshot(2, display_name='backup2', - status='available'), - stubs.stub_snapshot(3, display_name='backup3', - status='creating'), - ] - self.stubs.Set(db, 'snapshot_get_all_by_project', - stub_snapshot_get_all_by_project) - - # no status filter - req = fakes.HTTPRequest.blank('/v1/snapshots') - resp = self.controller.index(req) - self.assertEqual(len(resp['snapshots']), 3) - # single match - req = fakes.HTTPRequest.blank('/v1/snapshots?status=creating') - resp = self.controller.index(req) - self.assertEqual(len(resp['snapshots']), 1) - self.assertEqual(resp['snapshots'][0]['status'], 'creating') - # multiple match - req = fakes.HTTPRequest.blank('/v1/snapshots?status=available') - resp = self.controller.index(req) - self.assertEqual(len(resp['snapshots']), 2) - for snapshot in resp['snapshots']: - self.assertEqual(snapshot['status'], 'available') - # no match - req = fakes.HTTPRequest.blank('/v1/snapshots?status=error') - resp = self.controller.index(req) - self.assertEqual(len(resp['snapshots']), 0) - - @mock.patch('cinder.db.snapshot_metadata_get', return_value=dict()) - def test_snapshot_list_by_volume(self, snapshot_metadata_get): - def stub_snapshot_get_all_by_project(context, project_id): - return [ - stubs.stub_snapshot(1, volume_id='vol1', status='creating'), - stubs.stub_snapshot(2, volume_id='vol1', status='available'), - stubs.stub_snapshot(3, volume_id='vol2', status='available'), - ] - self.stubs.Set(db, 'snapshot_get_all_by_project', - stub_snapshot_get_all_by_project) - - # single match - req = fakes.HTTPRequest.blank('/v1/snapshots?volume_id=vol2') - resp = self.controller.index(req) - self.assertEqual(len(resp['snapshots']), 1) - self.assertEqual(resp['snapshots'][0]['volume_id'], 'vol2') - # multiple match - req = fakes.HTTPRequest.blank('/v1/snapshots?volume_id=vol1') - resp = self.controller.index(req) - self.assertEqual(len(resp['snapshots']), 2) - for snapshot in resp['snapshots']: - self.assertEqual(snapshot['volume_id'], 'vol1') - # multiple filters - req = fakes.HTTPRequest.blank('/v1/snapshots?volume_id=vol1' - '&status=available') - resp = self.controller.index(req) - self.assertEqual(len(resp['snapshots']), 1) - self.assertEqual(resp['snapshots'][0]['volume_id'], 'vol1') - self.assertEqual(resp['snapshots'][0]['status'], 'available') - - @mock.patch('cinder.db.snapshot_metadata_get', return_value=dict()) - def test_snapshot_list_by_name(self, snapshot_metadata_get): - def stub_snapshot_get_all_by_project(context, project_id): - 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) - - # no display_name filter - req = fakes.HTTPRequest.blank('/v1/snapshots') - resp = self.controller.index(req) - self.assertEqual(len(resp['snapshots']), 3) - # filter by one name - req = fakes.HTTPRequest.blank('/v1/snapshots?display_name=backup2') - resp = self.controller.index(req) - self.assertEqual(len(resp['snapshots']), 1) - self.assertEqual(resp['snapshots'][0]['display_name'], 'backup2') - # filter no match - req = fakes.HTTPRequest.blank('/v1/snapshots?display_name=backup4') - resp = self.controller.index(req) - self.assertEqual(len(resp['snapshots']), 0) - @mock.patch('cinder.db.snapshot_metadata_get', return_value=dict()) def test_admin_list_snapshots_limited_to_project(self, snapshot_metadata_get): @@ -414,7 +325,8 @@ class SnapshotApiTest(test.TestCase): 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): + 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'), diff --git a/cinder/tests/unit/api/v2/stubs.py b/cinder/tests/unit/api/v2/stubs.py index ae5a690d9..3841e94b8 100644 --- a/cinder/tests/unit/api/v2/stubs.py +++ b/cinder/tests/unit/api/v2/stubs.py @@ -153,13 +153,13 @@ def stub_snapshot(id, **kwargs): return snapshot -def stub_snapshot_get_all(self): +def stub_snapshot_get_all(self, search_opts=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): +def stub_snapshot_get_all_by_project(self, context, search_opts=None): return [stub_snapshot(1)] diff --git a/cinder/tests/unit/api/v2/test_snapshots.py b/cinder/tests/unit/api/v2/test_snapshots.py index 82ff94f05..1a7e4f8d3 100644 --- a/cinder/tests/unit/api/v2/test_snapshots.py +++ b/cinder/tests/unit/api/v2/test_snapshots.py @@ -323,95 +323,6 @@ class SnapshotApiTest(test.TestCase): resp_snapshot = resp_snapshots.pop() self.assertEqual(resp_snapshot['id'], UUID) - @mock.patch('cinder.db.snapshot_metadata_get', return_value=dict()) - def test_snapshot_list_by_status(self, snapshot_metadata_get): - def stub_snapshot_get_all_by_project(context, project_id): - return [ - stubs.stub_snapshot(1, display_name='backup1', - status='available'), - stubs.stub_snapshot(2, display_name='backup2', - status='available'), - stubs.stub_snapshot(3, display_name='backup3', - status='creating'), - ] - self.stubs.Set(db, 'snapshot_get_all_by_project', - stub_snapshot_get_all_by_project) - - # no status filter - req = fakes.HTTPRequest.blank('/v2/snapshots') - resp = self.controller.index(req) - self.assertEqual(len(resp['snapshots']), 3) - # single match - req = fakes.HTTPRequest.blank('/v2/snapshots?status=creating') - resp = self.controller.index(req) - self.assertEqual(len(resp['snapshots']), 1) - self.assertEqual(resp['snapshots'][0]['status'], 'creating') - # multiple match - req = fakes.HTTPRequest.blank('/v2/snapshots?status=available') - resp = self.controller.index(req) - self.assertEqual(len(resp['snapshots']), 2) - for snapshot in resp['snapshots']: - self.assertEqual(snapshot['status'], 'available') - # no match - req = fakes.HTTPRequest.blank('/v2/snapshots?status=error') - resp = self.controller.index(req) - self.assertEqual(len(resp['snapshots']), 0) - - @mock.patch('cinder.db.snapshot_metadata_get', return_value={}) - def test_snapshot_list_by_volume(self, snapshot_metadata_get): - def stub_snapshot_get_all_by_project(context, project_id): - return [ - stubs.stub_snapshot(1, volume_id='vol1', status='creating'), - stubs.stub_snapshot(2, volume_id='vol1', status='available'), - stubs.stub_snapshot(3, volume_id='vol2', status='available'), - ] - self.stubs.Set(db, 'snapshot_get_all_by_project', - stub_snapshot_get_all_by_project) - - # single match - req = fakes.HTTPRequest.blank('/v2/snapshots?volume_id=vol2') - resp = self.controller.index(req) - self.assertEqual(len(resp['snapshots']), 1) - self.assertEqual(resp['snapshots'][0]['volume_id'], 'vol2') - # multiple match - req = fakes.HTTPRequest.blank('/v2/snapshots?volume_id=vol1') - resp = self.controller.index(req) - self.assertEqual(len(resp['snapshots']), 2) - for snapshot in resp['snapshots']: - self.assertEqual(snapshot['volume_id'], 'vol1') - # multiple filters - req = fakes.HTTPRequest.blank('/v2/snapshots?volume_id=vol1' - '&status=available') - resp = self.controller.index(req) - self.assertEqual(len(resp['snapshots']), 1) - self.assertEqual(resp['snapshots'][0]['volume_id'], 'vol1') - self.assertEqual(resp['snapshots'][0]['status'], 'available') - - @mock.patch('cinder.db.snapshot_metadata_get', return_value={}) - def test_snapshot_list_by_name(self, snapshot_metadata_get): - def stub_snapshot_get_all_by_project(context, project_id): - 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) - - # no name filter - req = fakes.HTTPRequest.blank('/v2/snapshots') - resp = self.controller.index(req) - self.assertEqual(len(resp['snapshots']), 3) - # filter by one name - req = fakes.HTTPRequest.blank('/v2/snapshots?name=backup2') - resp = self.controller.index(req) - self.assertEqual(len(resp['snapshots']), 1) - self.assertEqual(resp['snapshots'][0]['name'], 'backup2') - # filter no match - req = fakes.HTTPRequest.blank('/v2/snapshots?name=backup4') - resp = self.controller.index(req) - self.assertEqual(len(resp['snapshots']), 0) - @mock.patch('cinder.db.snapshot_metadata_get', return_value=dict()) def test_admin_list_snapshots_limited_to_project(self, snapshot_metadata_get): @@ -426,7 +337,8 @@ class SnapshotApiTest(test.TestCase): 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): + 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'), diff --git a/cinder/tests/unit/objects/test_snapshot.py b/cinder/tests/unit/objects/test_snapshot.py index 5b928f8db..6e824d138 100644 --- a/cinder/tests/unit/objects/test_snapshot.py +++ b/cinder/tests/unit/objects/test_snapshot.py @@ -146,9 +146,12 @@ class TestSnapshotList(test_objects.BaseObjectsTestCase): fake_volume_obj = fake_volume.fake_volume_obj(self.context) volume_get_by_id.return_value = fake_volume_obj - snapshots = objects.SnapshotList.get_all(self.context) + search_opts = mock.sentinel.search_opts + snapshots = objects.SnapshotList.get_all( + self.context, search_opts) self.assertEqual(1, len(snapshots)) TestSnapshot._compare(self, fake_snapshot, snapshots[0]) + snapshot_get_all.assert_called_once_with(self.context, search_opts) @mock.patch('cinder.db.snapshot_metadata_get', return_value={}) @mock.patch('cinder.objects.Volume.get_by_id') @@ -173,10 +176,14 @@ class TestSnapshotList(test_objects.BaseObjectsTestCase): fake_volume_obj = fake_volume.fake_volume_obj(self.context) volume_get_by_id.return_value = fake_volume_obj + search_opts = mock.sentinel.search_opts snapshots = objects.SnapshotList.get_all_by_project( - self.context, self.project_id) + self.context, self.project_id, search_opts) self.assertEqual(1, len(snapshots)) TestSnapshot._compare(self, fake_snapshot, snapshots[0]) + get_all_by_project.assert_called_once_with(self.context, + self.project_id, + search_opts) @mock.patch('cinder.db.snapshot_metadata_get', return_value={}) @mock.patch('cinder.objects.volume.Volume.get_by_id') diff --git a/cinder/tests/unit/test_db_api.py b/cinder/tests/unit/test_db_api.py index 74cb105b7..1688759cd 100644 --- a/cinder/tests/unit/test_db_api.py +++ b/cinder/tests/unit/test_db_api.py @@ -954,11 +954,72 @@ class DBAPISnapshotTestCase(BaseTest): actual = db.snapshot_data_get_for_project(self.ctxt, 'project1') self.assertEqual(actual, (1, 42)) - def test_snapshot_get_all(self): + def test_snapshot_get_all_by_filter(self): db.volume_create(self.ctxt, {'id': 1}) - snapshot = db.snapshot_create(self.ctxt, {'id': 1, 'volume_id': 1}) - self._assertEqualListsOfObjects([snapshot], - db.snapshot_get_all(self.ctxt), + db.volume_create(self.ctxt, {'id': 2}) + snapshot1 = db.snapshot_create(self.ctxt, {'id': 1, 'volume_id': 1, + 'display_name': 'one', + 'status': 'available'}) + snapshot2 = db.snapshot_create(self.ctxt, {'id': 2, 'volume_id': 1, + 'display_name': 'two', + 'status': 'creating'}) + snapshot3 = db.snapshot_create(self.ctxt, {'id': 3, 'volume_id': 2, + 'display_name': 'three', + 'status': 'available'}) + # no filter + filters = {} + snapshots = db.snapshot_get_all(self.ctxt, filters=filters) + self.assertEqual(3, len(snapshots)) + # single match + filters = {'display_name': 'two'} + self._assertEqualListsOfObjects([snapshot2], + db.snapshot_get_all( + self.ctxt, + filters), + ignored_keys=['metadata', 'volume']) + filters = {'volume_id': 2} + self._assertEqualListsOfObjects([snapshot3], + db.snapshot_get_all( + self.ctxt, + filters), + ignored_keys=['metadata', 'volume']) + # filter no match + filters = {'volume_id': 5} + self._assertEqualListsOfObjects([], + db.snapshot_get_all( + self.ctxt, + filters), + ignored_keys=['metadata', 'volume']) + filters = {'status': 'error'} + self._assertEqualListsOfObjects([], + db.snapshot_get_all( + self.ctxt, + filters), + ignored_keys=['metadata', 'volume']) + # multiple match + filters = {'volume_id': 1} + self._assertEqualListsOfObjects([snapshot1, snapshot2], + db.snapshot_get_all( + self.ctxt, + filters), + ignored_keys=['metadata', 'volume']) + filters = {'status': 'available'} + self._assertEqualListsOfObjects([snapshot1, snapshot3], + db.snapshot_get_all( + self.ctxt, + filters), + ignored_keys=['metadata', 'volume']) + filters = {'volume_id': 1, 'status': 'available'} + self._assertEqualListsOfObjects([snapshot1], + db.snapshot_get_all( + self.ctxt, + filters), + ignored_keys=['metadata', 'volume']) + filters = {'fake_key': 'fake'} + self._assertEqualListsOfObjects([], + db.snapshot_get_all( + self.ctxt, + filters), ignored_keys=['metadata', 'volume']) def test_snapshot_get_by_host(self): diff --git a/cinder/volume/api.py b/cinder/volume/api.py index 21431c613..11abff87e 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -507,23 +507,12 @@ class API(base.Base): 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) + snapshots = objects.SnapshotList.get_all(context, + search_opts) else: snapshots = objects.SnapshotList.get_all_by_project( - context, context.project_id) + context, context.project_id, search_opts) - if search_opts: - LOG.debug("Searching by: %s", search_opts) - - results = [] - not_found = object() - for snapshot in snapshots: - for opt, value in search_opts.items(): - if snapshot.get(opt, not_found) != value: - break - else: - results.append(snapshot) - snapshots.objects = results LOG.info(_LI("Get all snaphsots completed successfully.")) return snapshots