compute: Update volume_id within connection_info during swap_volume

When Cinder orchestrates a volume migration between backends it
initially creates a temporary volume on the destination before calling
Nova to swap to that volume. When this is complete Nova calls back to
Cinder and the temporary volume on the destination is renamed to the
original volume UUID making the migration transparent to end users.

Previously Nova would not account for this within the connection_info
stahed when connecting the new volume and would continue to point to the
original UUID of the temporary volume. For most codepaths in Nova this
isn't an issue but when dealing with multiattach volumes the libvirt
driver has a specific path that uses this stored volume_id within the
connection_info when an attempt is made to detach the volume,
nova.virt.libvirt.LibvirtDriver._should_disconnect_target. In this case
this would lead to a failed lookup of the volume in Cinder and an
eventual 500 returned by Nova.

This change corrects this by ensuring any volume_id stashed in the
new connection_info we gather during a swap_volume is overwritten with
the correct id returned by the eventual call to Cinder's
os-migrate_volume_completion API [1].

[1] https://docs.openstack.org/api-ref/block-storage/v3/index.html?expanded=complete-migration-of-a-volume-detail#volumes-volumes

Co-Authored-By: Lee Yarwood <[email protected]>
Closes-Bug: #1943431
Change-Id: I43612714b343d98320b19b5b38264afc700790e3

Author: maaoyu

nova/compute/manager.py

@@ -7463,7 +7463,16 @@ def _do_swap_volume(self, context, old_volume_id, new_volume_id,
         # 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',

nova/tests/functional/regressions/test_bug_1943431.py

@@ -16,7 +16,6 @@
 
 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 @@ def test_ro_multiattach_swap_volume(self):
         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 @@ def test_ro_multiattach_migrate_volume(self):
             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)

nova/tests/unit/compute/test_compute_mgr.py

@@ -2977,8 +2977,11 @@ def test_swap_volume_with_new_attachment_id_cinder_migrate_true(
             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 @@ def test_swap_volume_with_new_attachment_id_cinder_migrate_false(
             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')