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 38aa4e5ed5af207ebc112119904a48178a132c38..a7bdceedc45a607544e2b38d19f88a596f021223 100644 (file)
--- 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 649a4b93dc76ad7340814d387d90aa12c5eac136..6611750227a0ec7cf8512940a5f8db6770eaccf6 100644 (file)
--- 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)