Set 'serial' to new volume ID in swap volumes

In swap_volume method of nova/virt/libvirt/driver.py,
before BDM was got by using the instance's UUID and
'serial' of new connection_info as the volume ID,
and driver BDM was updated by using the BDM.
('serial' has the volume ID information.)
But in _init_volume_connection method in ComputeManager class,
'serial' is passed from old connection_info to new connection_info.

It works fine in the case that cinder initiates swapping volumes
because the ID of the attached volume isn't changed after
swapping volumes.
But in the case that nova initiates swapping volumes,
the ID of the attached volume is changed.

So in the case that nova initiated swapping volumes,
after swap volume function was performed once,
BDM was got by wrong old volume id (serial)
when swap volume function was performed for the second time.

So if 'serial' of new connection_info is None,
it is set to new volume ID.
And if cinder 'migrate_volume_completion' API returns
old volume ID (the case that cinder initiates swapping volumes),
the 'serial' of new connection_info is set to old volume ID.
If cinder 'migrate_volume_completion' API returns new volume ID
(the case that nova initiated swapping volumes),
the 'serial' is left as it is (new volume ID).

Change-Id: I86b8fbb09b0f1ed4c667683de3827cd9b63bca7f
Closes-Bug: #1490236

nova/compute/manager.py

@@ -4846,9 +4846,8 @@ class ComputeManager(manager.Manager):
                                                           new_volume_id,
                                                           connector)
         old_cinfo = jsonutils.loads(bdm['connection_info'])
-        if old_cinfo and 'serial' not in old_cinfo:
-            old_cinfo['serial'] = old_volume_id
-        new_cinfo['serial'] = old_cinfo['serial']
+        if 'serial' not in new_cinfo:
+            new_cinfo['serial'] = new_volume_id
         return (old_cinfo, new_cinfo)
 
     def _swap_volume(self, context, instance, bdm, connector,
@@ -4905,6 +4904,13 @@ class ComputeManager(manager.Manager):
                                                       old_volume_id,
                                                       new_volume_id,
                                                       error=failed)
+            # If Cinder initiated the swap, the serial of new connection info
+            # is set to old volume ID.
+            if (new_cinfo is not None and
+                new_cinfo['serial'] == new_volume_id and
+                comp_ret['save_volume_id'] == old_volume_id):
+                new_cinfo['serial'] = old_volume_id
+
             LOG.debug("swap_volume: Cinder migrate_volume_completion "
                       "returned: %(comp_ret)s", {'comp_ret': comp_ret},
                       context=context, instance=instance)

nova/tests/unit/compute/test_compute_mgr.py

@@ -1852,6 +1852,72 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
         self.assertEqual(volumes[old_volume_id]['status'], 'in-use')
         self.assertEqual(volumes[new_volume_id]['status'], 'available')
 
+    @mock.patch('nova.db.block_device_mapping_get_by_instance_and_volume_id')
+    @mock.patch.object(fake_driver.FakeDriver, 'get_volume_connector',
+                       return_value={})
+    @mock.patch.object(fake_driver.FakeDriver, 'swap_volume')
+    @mock.patch('nova.volume.cinder.API.get')
+    @mock.patch('nova.volume.cinder.API.initialize_connection',
+                return_value={})
+    @mock.patch('nova.volume.cinder.API.terminate_connection')
+    @mock.patch('nova.volume.cinder.API.migrate_volume_completion')
+    @mock.patch('nova.objects.BlockDeviceMapping.update')
+    @mock.patch('nova.objects.BlockDeviceMapping.save')
+    def test_swap_volume_cinder_initiated(self, mock_bdm_save, mock_bdm_update,
+                                          mock_migrate_volume_completion,
+                                          mock_terminate_connection,
+                                          mock_initialize_connection,
+                                          mock_get, mock_swap_volume,
+                                          mock_get_volume_connector,
+                                          mock_bdm_get):
+        # Check whether the 'serial' in new connection info is equal to
+        # the old volume ID in the case that cinder initiated
+        # swapping volumes
+        mock_get.return_value = {'id': uuids.old_volume,
+                                 'display_name': 'old_volume',
+                                 'status': 'detaching',
+                                 'size': 2}
+        fake_bdm = fake_block_device.FakeDbBlockDeviceDict(
+            {'device_name': '/dev/vdb', 'source_type': 'volume',
+             'destination_type': 'volume',
+             'instance_uuid': uuids.instance,
+             'delete_on_termination': True,
+             'connection_info': '{"foo": "bar"}'})
+        mock_bdm_get.return_value = fake_bdm
+        mock_migrate_volume_completion.return_value = {'save_volume_id':
+                                                       uuids.old_volume}
+        instance = fake_instance.fake_instance_obj(self.context,
+                                                   **{'uuid': uuids.instance})
+
+        self.compute.swap_volume(
+            self.context, uuids.old_volume, uuids.new_volume, instance)
+
+        mock_get_volume_connector.assert_called_once_with(instance)
+        mock_get.assert_has_calls(
+            [mock.call(test.MatchType(context.RequestContext),
+                       uuids.old_volume),
+             mock.call(test.MatchType(context.RequestContext),
+                       uuids.new_volume)])
+        mock_initialize_connection.assert_called_once_with(
+            test.MatchType(context.RequestContext), uuids.new_volume, {})
+        mock_swap_volume.assert_called_once_with(
+            {"foo": "bar"}, {'serial': uuids.old_volume}, instance,
+            '/dev/vdb', 0)
+        mock_terminate_connection.assert_called_once_with(
+            test.MatchType(context.RequestContext), uuids.old_volume, {})
+        mock_migrate_volume_completion.assert_called_once_with(
+            test.MatchType(context.RequestContext), uuids.old_volume,
+            uuids.new_volume, error=False)
+        # Check 'serial' in new connection info
+        mock_bdm_update.assert_called_once_with(
+            {'connection_info': jsonutils.dumps({'serial': uuids.old_volume}),
+             'source_type': 'volume',
+             'destination_type': 'volume',
+             'snapshot_id': None,
+             'volume_id': uuids.old_volume,
+             'no_device': None})
+        mock_bdm_save.assert_called_once_with()
+
     @mock.patch('nova.db.block_device_mapping_get_by_instance_and_volume_id')
     @mock.patch('nova.db.block_device_mapping_update')
     @mock.patch('nova.volume.cinder.API.get')

nova/tests/unit/virt/libvirt/test_driver.py

@@ -14308,6 +14308,7 @@ class LibvirtConnTestCase(test.NoDBTestCase):
                 srcfile, 1 * units.Gi / units.Ki)
             mock_define.assert_called_once_with(xmldoc)
 
+    @mock.patch('nova.context.get_admin_context', return_value='fake-context')
     @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume')
     @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._swap_volume')
     @mock.patch('nova.objects.block_device.BlockDeviceMapping.'
@@ -14316,12 +14317,13 @@ class LibvirtConnTestCase(test.NoDBTestCase):
     @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._connect_volume')
     @mock.patch('nova.virt.libvirt.host.Host.get_guest')
     def _test_swap_volume_driver_bdm_save(self, get_guest,
-                                         connect_volume, get_volume_config,
-                                         get_by_volume_and_instance,
-                                         swap_volume,
-                                         disconnect_volume,
-                                         volume_save,
-                                         source_type):
+                                          connect_volume, get_volume_config,
+                                          get_by_volume_and_instance,
+                                          swap_volume,
+                                          disconnect_volume,
+                                          get_admin_context,
+                                          volume_save,
+                                          source_type):
         conn = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI())
         instance = objects.Instance(**self.test_instance)
         old_connection_info = {'driver_volume_type': 'fake',
@@ -14365,6 +14367,9 @@ class LibvirtConnTestCase(test.NoDBTestCase):
 
         get_guest.assert_called_once_with(instance)
         connect_volume.assert_called_once_with(new_connection_info, disk_info)
+        get_by_volume_and_instance.assert_called_once_with('fake-context',
+                                                           'old-volume-id',
+                                                           instance.uuid)
 
         swap_volume.assert_called_once_with(guest, 'vdb',
                                             '/fake-new-volume', 1)

nova/virt/libvirt/driver.py

@@ -1217,7 +1217,7 @@ class LibvirtDriver(driver.ComputeDriver):
             raise NotImplementedError(_("Swap only supports host devices"))
 
         # Save updates made in connection_info when connect_volume was called
-        volume_id = new_connection_info.get('serial')
+        volume_id = old_connection_info.get('serial')
         bdm = objects.BlockDeviceMapping.get_by_volume_and_instance(
             nova_context.get_admin_context(), volume_id, instance.uuid)
         driver_bdm = driver_block_device.convert_volume(bdm)