openstack/cinder
Code Issues Proposed changes

Remove the jointly loaded model in finish_volume_migration

Browse Source 
After a successful volume migration, the source volume and the
destination volume need to swap the data in the models in
finish_volume_migration. It is sufficient to load the volume
model only and there is no need to load other models, like
volume type, metadata, consistency group, etc. If we load
these additional models, it will lead to NULL key error, when
either source or destination model has a NULL key pointer.

Change-Id: I04ad0739387d602719591680854e6655cc87f9ab
Closes-Bug: #1505572
This commit is contained in:
Vincent Hou
2015-10-13 19:35:13 -07:00
parent 944763920d
commit 79414d17d8
2 changed files with 64 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -1203,9 +1203,11 @@ def finish_volume_migration(context, src_vol_id, dest_vol_id):
"""
session = get_session()
with session.begin():
src_volume_ref = _volume_get(context, src_vol_id, session=session)
src_volume_ref = _volume_get(context, src_vol_id, session=session,
joined_load=False)
src_original_data = dict(src_volume_ref.iteritems())
dest_volume_ref = _volume_get(context, dest_vol_id, session=session)
dest_volume_ref = _volume_get(context, dest_vol_id, session=session,
joined_load=False)
# NOTE(rpodolyaka): we should copy only column values, while model
# instances also have relationships attributes, which
@@ -1335,7 +1337,24 @@ def volume_detached(context, volume_id, attachment_id):
@require_context
def _volume_get_query(context, session=None, project_only=False):
def _volume_get_query(context, session=None, project_only=False,
joined_load=True):
"""Get the query to retrieve the volume.
:param context: the context used to run the method _volume_get_query
:param session: the session to use
:param project_only: the boolean used to decide whether to query the
volume in the current project or all projects
:param joined_load: the boolean used to decide whether the query loads
the other models, which join the volume model in
the database. Currently, the False value for this
parameter is specially for the case of updating
database during volume migration
:returns: updated query or None
"""
if not joined_load:
return model_query(context, models.Volume, session=session,
project_only=project_only)
if is_admin_context(context):
return model_query(context, models.Volume, session=session,
project_only=project_only).\
@@ -1354,11 +1373,12 @@ def _volume_get_query(context, session=None, project_only=False):
@require_context
def _volume_get(context, volume_id, session=None):
result = _volume_get_query(context, session=session, project_only=True).\
options(joinedload('volume_type.extra_specs')).\
filter_by(id=volume_id).\
first()
def _volume_get(context, volume_id, session=None, joined_load=True):
result = _volume_get_query(context, session=session, project_only=True,
joined_load=joined_load)
if joined_load:
result = result.options(joinedload('volume_type.extra_specs'))
result = result.filter_by(id=volume_id).first()
if not result:
raise exception.VolumeNotFound(volume_id=volume_id)

42
cinder/tests/unit/db/test_finish_migration.py
View File

@@ -25,28 +25,58 @@ from cinder.tests.unit import utils as testutils
class FinishVolumeMigrationTestCase(test.TestCase):
"""Test cases for finish_volume_migration."""
def test_finish_volume_migration(self):
def test_finish_volume_migration_no_volume_type(self):
self._test_finish_volume_migration()
def test_finish_volume_migration_with_volume_type(self):
source_type = {'name': 'old', 'extra_specs': {}}
dest_type = {'name': 'new', 'extra_specs': {}}
self._test_finish_volume_migration(source_type=source_type,
dest_type=dest_type)
def test_finish_volume_migration_none_to_volume_type(self):
dest_type = {'name': 'new', 'extra_specs': {}}
self._test_finish_volume_migration(dest_type=dest_type)
def _test_finish_volume_migration(self, source_type=None, dest_type=None):
ctxt = context.RequestContext(user_id='user_id',
project_id='project_id',
is_admin=True)
source_type_id = None
dest_type_id = None
if source_type:
source_type_id = db.volume_type_create(ctxt, source_type).id
if dest_type:
dest_type_id = db.volume_type_create(ctxt, dest_type).id
src_volume = testutils.create_volume(ctxt, host='src',
migration_status='migrating',
status='available')
status='available',
volume_type_id=source_type_id)
dest_volume = testutils.create_volume(ctxt, host='dest',
migration_status='target:fake',
status='available')
db.finish_volume_migration(ctxt, src_volume['id'], dest_volume['id'])
status='available',
volume_type_id=dest_type_id)
db.finish_volume_migration(ctxt, src_volume.id, dest_volume.id)
# Check that we have copied destination volume DB data into source DB
# entry so we can keep the id
src_volume = objects.Volume.get_by_id(ctxt, src_volume['id'])
src_volume = objects.Volume.get_by_id(ctxt, src_volume.id)
self.assertEqual('dest', src_volume.host)
self.assertEqual('available', src_volume.status)
self.assertIsNone(src_volume.migration_status)
if dest_type:
self.assertEqual(dest_type_id, src_volume.volume_type_id)
else:
self.assertIsNone(src_volume.volume_type_id)
# Check that we have copied source volume DB data into destination DB
# entry and we are setting it to deleting
dest_volume = objects.Volume.get_by_id(ctxt, dest_volume['id'])
dest_volume = objects.Volume.get_by_id(ctxt, dest_volume.id)
self.assertEqual('src', dest_volume.host)
self.assertEqual('deleting', dest_volume.status)
self.assertEqual('deleting', dest_volume.migration_status)
if source_type:
self.assertEqual(source_type_id, dest_volume.volume_type_id)
else:
self.assertIsNone(dest_volume.volume_type_id)