From: Vincent Hou Date: Wed, 14 Oct 2015 02:35:13 +0000 (-0700) Subject: Remove the jointly loaded model in finish_volume_migration X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=79414d17d84efe7434d70a4ae1436b45cfc09421;p=openstack-build%2Fcinder-build.git Remove the jointly loaded model in finish_volume_migration 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 --- diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index 38aa4e5ed..a7bdceedc 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -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) diff --git a/cinder/tests/unit/db/test_finish_migration.py b/cinder/tests/unit/db/test_finish_migration.py index 649a4b93d..661175022 100644 --- a/cinder/tests/unit/db/test_finish_migration.py +++ b/cinder/tests/unit/db/test_finish_migration.py @@ -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)