From daf0b43dbf195c30a931b646481ffee5e28feda9 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Wed, 22 Jun 2016 18:16:47 +0200 Subject: [PATCH] DB: Optimize volume_update method Our current volume_update method reads the volume from the DB, joining the volume table with 4 or 5 other tables, and then do the update using SQLAlchemy ORM engine before returning updated ORM object. But the truth is that we don't really need to retrieve the volume again in any of existing cases, since the caller either doesn't care about current value in the DB -for example when setting status to error- or it already has that information -composing it with the current value it has and the updated fields it's passing- and doesn't need to reload it. This additional joined read from the DB on every volume update is creating unnecessary load on the DB and reducing our network capacity and in other resources we are not retrieving it either. This patch modifies the way we do updates so we don't read current DB values. Change-Id: I92e76eb8eb64ac96995b29017c7683a564dc5e0e --- cinder/db/sqlalchemy/api.py | 8 ++--- .../unit/api/contrib/test_admin_actions.py | 9 +++--- cinder/tests/unit/db/test_name_id.py | 10 +++--- cinder/tests/unit/test_db_api.py | 20 ++++++------ cinder/tests/unit/test_volume.py | 32 ++++--------------- cinder/volume/driver.py | 4 +-- 6 files changed, 30 insertions(+), 53 deletions(-) diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index 58306358144..05f92cbe15c 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -2173,10 +2173,10 @@ def volume_update(context, volume_id, values): delete=True, session=session) - volume_ref = _volume_get(context, volume_id, session=session) - volume_ref.update(values) - - return volume_ref + query = _volume_get_query(context, session, joined_load=False) + result = query.filter_by(id=volume_id).update(values) + if not result: + raise exception.VolumeNotFound(volume_id=volume_id) @handle_db_data_error diff --git a/cinder/tests/unit/api/contrib/test_admin_actions.py b/cinder/tests/unit/api/contrib/test_admin_actions.py index 0454208c2ce..6ce9daaff84 100644 --- a/cinder/tests/unit/api/contrib/test_admin_actions.py +++ b/cinder/tests/unit/api/contrib/test_admin_actions.py @@ -569,8 +569,8 @@ class AdminActionsTest(BaseAdminTest): expected_status = 400 host = 'test2' volume = self._migrate_volume_prep() - model_update = {'migration_status': 'migrating'} - volume = db.volume_update(self.ctx, volume['id'], model_update) + volume.migration_status = 'migrating' + volume.save() self._migrate_volume_exec(self.ctx, volume, host, expected_status) def test_migrate_volume_with_snap(self): @@ -1050,8 +1050,7 @@ class AdminActionsAttachDetachTest(BaseAdminTest): volume = self._create_volume(self.ctx, {'provider_location': '', 'size': 1}) - values = {'status': 'attaching', - 'instance_uuid': fake.INSTANCE_ID} + values = {'status': 'attaching'} db.volume_update(self.ctx, volume['id'], values) db.volume_admin_metadata_update(self.ctx, volume['id'], {"attached_mode": 'rw'}, False) @@ -1060,7 +1059,7 @@ class AdminActionsAttachDetachTest(BaseAdminTest): self.volume_api.attach, self.ctx, volume, - values['instance_uuid'], + fake.INSTANCE_ID, None, mountpoint, 'ro') diff --git a/cinder/tests/unit/db/test_name_id.py b/cinder/tests/unit/db/test_name_id.py index fbb7f9b51a5..0bf5c9e3140 100644 --- a/cinder/tests/unit/db/test_name_id.py +++ b/cinder/tests/unit/db/test_name_id.py @@ -43,18 +43,16 @@ class NameIDsTestCase(test.TestCase): def test_name_id_diff(self): """Change name ID to mimic volume after migration.""" - vol_ref = testutils.create_volume(self.ctxt, size=1) - db.volume_update(self.ctxt, vol_ref['id'], - {'name_id': fake.VOLUME2_ID}) + vol_ref = testutils.create_volume(self.ctxt, size=1, + _name_id=fake.VOLUME2_ID) vol_ref = db.volume_get(self.ctxt, vol_ref['id']) expected_name = CONF.volume_name_template % fake.VOLUME2_ID self.assertEqual(expected_name, vol_ref['name']) def test_name_id_snapshot_volume_name(self): """Make sure snapshot['volume_name'] is updated.""" - vol_ref = testutils.create_volume(self.ctxt, size=1) - db.volume_update(self.ctxt, vol_ref['id'], - {'name_id': fake.VOLUME2_ID}) + vol_ref = testutils.create_volume(self.ctxt, size=1, + _name_id=fake.VOLUME2_ID) snap_ref = testutils.create_snapshot(self.ctxt, vol_ref['id']) expected_name = CONF.volume_name_template % fake.VOLUME2_ID self.assertEqual(expected_name, snap_ref['volume_name']) diff --git a/cinder/tests/unit/test_db_api.py b/cinder/tests/unit/test_db_api.py index a931215f8d1..941986351ff 100644 --- a/cinder/tests/unit/test_db_api.py +++ b/cinder/tests/unit/test_db_api.py @@ -942,17 +942,15 @@ class DBAPIVolumeTestCase(BaseTest): def test_volume_update(self): volume = db.volume_create(self.ctxt, {'host': 'h1'}) - ref_a = db.volume_update(self.ctxt, volume['id'], - {'host': 'h2', - 'metadata': {'m1': 'v1'}}) - volume = db.volume_get(self.ctxt, volume['id']) - self.assertEqual('h2', volume['host']) - expected = dict(ref_a) - expected['volume_metadata'] = list(map(dict, - expected['volume_metadata'])) - result = dict(volume) - result['volume_metadata'] = list(map(dict, result['volume_metadata'])) - self.assertEqual(expected, result) + db.volume_update(self.ctxt, volume.id, + {'host': 'h2', + 'metadata': {'m1': 'v1'}}) + volume = db.volume_get(self.ctxt, volume.id) + self.assertEqual('h2', volume.host) + self.assertEqual(1, len(volume.volume_metadata)) + db_metadata = volume.volume_metadata[0] + self.assertEqual('m1', db_metadata.key) + self.assertEqual('v1', db_metadata.value) def test_volume_update_nonexistent(self): self.assertRaises(exception.VolumeNotFound, db.volume_update, diff --git a/cinder/tests/unit/test_volume.py b/cinder/tests/unit/test_volume.py index f150e4a067b..11adb8adb10 100644 --- a/cinder/tests/unit/test_volume.py +++ b/cinder/tests/unit/test_volume.py @@ -5426,16 +5426,13 @@ class VolumeMigrationTestCase(base.BaseVolumeTestCase): def _test_delete_volume_in_migration(self, migration_status): """Test deleting a volume that is in migration.""" - volume = tests_utils.create_volume(self.context, **self.volume_params) - volume.status = 'available' - volume.migration_status = migration_status - volume.save() - self.volume.delete_volume(self.context, volume) + volume = tests_utils.create_volume(self.context, host=CONF.host, + migration_status=migration_status) + self.volume.delete_volume(self.context, volume=volume) # The volume is successfully removed during the volume delete # and won't exist in the database any more. - self.assertRaises(exception.VolumeNotFound, db.volume_get, - self.context, volume.id) + self.assertRaises(exception.VolumeNotFound, volume.refresh) class ReplicationTestCase(base.BaseVolumeTestCase): @@ -5614,24 +5611,9 @@ class CopyVolumeToImageTestCase(base.BaseVolumeTestCase): '45b1161abb02' db.volume_create(self.context, self.volume_attrs) - # Storing unmocked db api function reference here, because we have to - # update volume status (set instance_uuid to None) before calling the - # 'volume_update_status_based_on_attached_instance_id' db api. - unmocked_db_api = db.volume_update_status_based_on_attachment - - def mock_volume_update_after_upload(context, volume_id): - # First update volume and set 'instance_uuid' to None - # because after deleting instance, instance_uuid of volume is - # set to None - db.volume_update(context, volume_id, {'instance_uuid': None}) - # Calling unmocked db api - unmocked_db_api(context, volume_id) - - with mock.patch.object( - db, - 'volume_update_status_based_on_attachment', - side_effect=mock_volume_update_after_upload) as mock_update: - + method = 'volume_update_status_based_on_attachment' + with mock.patch.object(db, method, + wraps=getattr(db, method)) as mock_update: # Start test self.volume.copy_volume_to_image(self.context, self.volume_id, diff --git a/cinder/volume/driver.py b/cinder/volume/driver.py index 07b1b3f63a5..12e6ea001eb 100644 --- a/cinder/volume/driver.py +++ b/cinder/volume/driver.py @@ -938,8 +938,8 @@ class BaseVD(object): LOG.debug("Volume %s: creating export", volume['id']) model_update = self.create_export(context, volume, properties) if model_update: - volume = self.db.volume_update(context, volume['id'], - model_update) + volume.update(model_update) + volume.save() except exception.CinderException as ex: if model_update: LOG.exception(_LE("Failed updating model of volume "