openstack
/
cinder
Code Issues Proposed changes

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
This commit is contained in:
Gorka Eguileor 2016-06-22 18:16:47 +02:00
parent 268da28330
commit daf0b43dbf
6 changed files with 30 additions and 53 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
cinder/db/sqlalchemy/api.py
View File

@ -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

9
cinder/tests/unit/api/contrib/test_admin_actions.py
View File

@ -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')

10
cinder/tests/unit/db/test_name_id.py
View File

@ -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'])

20
cinder/tests/unit/test_db_api.py
View File

@ -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,

32
cinder/tests/unit/test_volume.py
View File

@ -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,

4
cinder/volume/driver.py
View File

@ -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 "