Merge "compute: Update volume_id within connection_info during swap_volume"

3 files changed, 36 insertions, 31 deletions

diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index ec37eea65a..35f887c619 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -7467,7 +7467,16 @@ class ComputeManager(manager.Manager):
         # NOTE(lyarwood): Update the BDM with the modified new_cinfo and
         # correct volume_id returned by Cinder.
         save_volume_id = comp_ret['save_volume_id']
+
+        # NOTE(lyarwood): Overwrite the possibly stale serial and volume_id in
+        # the connection_info with the volume_id returned from Cinder. This
+        # could be the case during a volume migration where the new_cinfo here
+        # refers to the temporary volume *before* Cinder renames it to the
+        # original volume UUID at the end of the migration.
         new_cinfo['serial'] = save_volume_id
+        new_cinfo['volume_id'] = save_volume_id
+        if 'data' in new_cinfo:
+            new_cinfo['data']['volume_id'] = save_volume_id
         values = {
             'connection_info': jsonutils.dumps(new_cinfo),
             'source_type': 'volume',
diff --git a/nova/tests/functional/regressions/test_bug_1943431.py b/nova/tests/functional/regressions/test_bug_1943431.py
index 6ca0b4e464..69c900c789 100644
--- a/nova/tests/functional/regressions/test_bug_1943431.py
+++ b/nova/tests/functional/regressions/test_bug_1943431.py
@@ -16,7 +16,6 @@ from oslo_serialization import jsonutils
 
 from nova import context
 from nova import objects
-from nova.tests.functional.api import client
 from nova.tests.functional import integrated_helpers
 from nova.tests.functional.libvirt import base
 from nova.virt import block_device as driver_block_device
@@ -28,9 +27,9 @@ class TestLibvirtROMultiattachMigrate(
 ):
     """Regression test for bug 1939545
 
-    This regression test asserts the current broken behaviour of Nova during
+    This regression test asserts the now fixed behaviour of Nova during
     a Cinder orchestrated volume migration that leaves the stashed
-    connection_info of the attachment pointing at a now deleted temporary
+    connection_info of the attachment pointing at the original
     volume UUID used during the migration.
 
     This is slightly different to the Nova orchestrated pure swap_volume
@@ -83,6 +82,10 @@ class TestLibvirtROMultiattachMigrate(
         self.assertEqual(
             self.cinder.MULTIATTACH_RO_SWAP_NEW_VOL,
             connection_info['data']['volume_id'])
+        self.assertIn('volume_id', connection_info)
+        self.assertEqual(
+            self.cinder.MULTIATTACH_RO_SWAP_NEW_VOL,
+            connection_info['volume_id'])
         self.assertIn('serial', connection_info)
         self.assertEqual(
             self.cinder.MULTIATTACH_RO_SWAP_NEW_VOL,
@@ -123,39 +126,26 @@ class TestLibvirtROMultiattachMigrate(
             server_id)
         connection_info = jsonutils.loads(bdm.connection_info)
 
-        # FIXME(lyarwood): This is bug #1943431 where only the serial within
-        # the connection_info of the temporary volume has been updated to point
-        # to the old volume UUID with the stashed volume_id provided by the
-        # backend still pointing to the temporary volume UUID that has now been
-        # deleted by cinder.
+        # Assert that only the old volume UUID is referenced within the stashed
+        # connection_info and returned by driver_block_device.get_volume_id
         self.assertIn('serial', connection_info)
         self.assertEqual(
             self.cinder.MULTIATTACH_RO_MIGRATE_OLD_VOL,
             connection_info.get('serial'))
+        self.assertIn('volume_id', connection_info)
+        self.assertEqual(
+            self.cinder.MULTIATTACH_RO_MIGRATE_OLD_VOL,
+            connection_info['volume_id'])
         self.assertIn('volume_id', connection_info.get('data'))
         self.assertEqual(
-            self.cinder.MULTIATTACH_RO_MIGRATE_NEW_VOL,
+            self.cinder.MULTIATTACH_RO_MIGRATE_OLD_VOL,
             connection_info['data']['volume_id'])
         self.assertEqual(
-            self.cinder.MULTIATTACH_RO_MIGRATE_NEW_VOL,
+            self.cinder.MULTIATTACH_RO_MIGRATE_OLD_VOL,
             driver_block_device.get_volume_id(connection_info))
 
-        # FIXME(lyarwood): As a result of the above any request to detach the
-        # migrated multiattach volume from the instance or any action that
-        # would cause the _disconnect_volume and _should_disconnect_target
-        # logic to trigger in the libvirt driver will fail as
-        # driver_block_device.get_volume_id points to the now deleted temporary
-        # volume used during the migration.
-        #
-        # Replace this with the following once fixed:
-        #
-        # self.api.delete_server_volume(
-        #    server_id, self.cinder.MULTIATTACH_RO_MIGRATE_OLD_VOL)
-        # self._wait_for_volume_detach(
-        #    server_id, self.cinder.MULTIATTACH_RO_MIGRATE_OLD_VOL)
-        ex = self.assertRaises(
-            client.OpenStackApiException,
-            self.api.delete_server_volume,
-            server_id,
-            self.cinder.MULTIATTACH_RO_MIGRATE_OLD_VOL)
-        self.assertEqual(500, ex.response.status_code)
+        # Assert that the old volume can be detached from the instance
+        self.api.delete_server_volume(
+            server_id, self.cinder.MULTIATTACH_RO_MIGRATE_OLD_VOL)
+        self._wait_for_volume_detach(
+            server_id, self.cinder.MULTIATTACH_RO_MIGRATE_OLD_VOL)
diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py
index 091c762f0a..945d2206ae 100644
--- a/nova/tests/unit/compute/test_compute_mgr.py
+++ b/nova/tests/unit/compute/test_compute_mgr.py
@@ -2977,8 +2977,11 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
             self.assertEqual(uuids.old_volume_id, bdm.volume_id)
             self.assertEqual(uuids.new_attachment_id, bdm.attachment_id)
             self.assertEqual(1, bdm.volume_size)
-            self.assertEqual(uuids.old_volume_id,
-                             jsonutils.loads(bdm.connection_info)['serial'])
+            new_conn_info = jsonutils.loads(bdm.connection_info)
+            self.assertEqual(uuids.old_volume_id, new_conn_info['serial'])
+            self.assertEqual(uuids.old_volume_id, new_conn_info['volume_id'])
+            self.assertEqual(
+                uuids.old_volume_id, new_conn_info['data']['volume_id'])
 
     @mock.patch.object(compute_utils, 'notify_about_volume_swap')
     @mock.patch.object(objects.BlockDeviceMapping,
@@ -3049,6 +3052,9 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
             self.assertEqual(2, bdm.volume_size)
             new_conn_info = jsonutils.loads(bdm.connection_info)
             self.assertEqual(uuids.new_volume_id, new_conn_info['serial'])
+            self.assertEqual(uuids.new_volume_id, new_conn_info['volume_id'])
+            self.assertEqual(
+                uuids.new_volume_id, new_conn_info['data']['volume_id'])
             self.assertNotIn('multiattach', new_conn_info)
 
     @mock.patch.object(compute_utils, 'add_instance_fault_from_exc')