Fix live-migrations usage of the wrong connector information

During the post_live_migration step for the Nova libvirt driver
an incorrect assumption is being made about the connector
information being sent to _disconnect_volume. It is assumed that
the connection information on the source and destination is the
same but that is not always the case. The BDM, where the
connector information is being retrieved from only contains the
connection information for the destination. This will not work
when trying to disconnect volumes from the source during live
migration as the properties such as the target_lun and
initiator_target_map could be different. This ends up leaving
behind dangling LUNs and possibly removing the incorrect
volume's LUNs.

The solution proposed here utilizes the connection_info that
can be retrieved for a host from Cinder's initialize_connection
API. This connection information contains the correct data for
the source host and allows volume LUNs to be removed properly.

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

NOTE(mriedem): The conflicts are due to the tests being moved
in Kilo and 41f80226e0a1f73af76c7968617ebfda0aeb40b1 not being
in stable/juno (renamed conn var to drvr in libvirt tests).

Change-Id: I3dfb75eb58dfbc66b218bcee473af4c2ac282eb6
Closes-Bug: #1475411
Closes-Bug: #1288039
Closes-Bug: #1423772
(cherry picked from commit 587092c909e15e983f7aef31d7bc0862271a32c7)

2 files changed, 41 insertions, 7 deletions

diff --git a/nova/tests/virt/libvirt/test_driver.py b/nova/tests/virt/libvirt/test_driver.py
index fff02af10d..b4339c5f1e 100644
--- a/nova/tests/virt/libvirt/test_driver.py
+++ b/nova/tests/virt/libvirt/test_driver.py
@@ -6284,10 +6284,22 @@ class LibvirtConnTestCase(test.TestCase):
 
     def test_post_live_migration(self):
         vol = {'block_device_mapping': [
-                  {'connection_info': 'dummy1', 'mount_device': '/dev/sda'},
-                  {'connection_info': 'dummy2', 'mount_device': '/dev/sdb'}]}
+                  {'connection_info': {
+                       'data': {'multipath_id': 'dummy1'},
+                       'serial': 'fake_serial1'},
+                    'mount_device': '/dev/sda',
+                   },
+                  {'connection_info': {
+                       'data': {},
+                       'serial': 'fake_serial2'},
+                    'mount_device': '/dev/sdb', }]}
+
+        def fake_initialize_connection(context, volume_id, connector):
+            return {'data': {}}
+
         conn = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
 
+        fake_connector = {'host': 'fake'}
         inst_ref = {'id': 'foo'}
         cntx = context.get_admin_context()
 
@@ -6295,16 +6307,22 @@ class LibvirtConnTestCase(test.TestCase):
         with contextlib.nested(
             mock.patch.object(driver, 'block_device_info_get_mapping',
                               return_value=vol['block_device_mapping']),
+            mock.patch.object(conn, "get_volume_connector",
+                              return_value=fake_connector),
+            mock.patch.object(conn._volume_api, "initialize_connection",
+                              side_effect=fake_initialize_connection),
             mock.patch.object(conn, '_disconnect_volume')
-        ) as (block_device_info_get_mapping, _disconnect_volume):
+        ) as (block_device_info_get_mapping, get_volume_connector,
+              initialize_connection, _disconnect_volume):
             conn.post_live_migration(cntx, inst_ref, vol)
 
             block_device_info_get_mapping.assert_has_calls([
                 mock.call(vol)])
+            get_volume_connector.assert_has_calls([
+                mock.call(inst_ref)])
             _disconnect_volume.assert_has_calls([
-                mock.call(v['connection_info'],
-                          v['mount_device'].rpartition("/")[2])
-                for v in vol['block_device_mapping']])
+                mock.call({'data': {'multipath_id': 'dummy1'}}, 'sda'),
+                mock.call({'data': {}}, 'sdb')])
 
     def test_get_instance_disk_info_excludes_volumes(self):
         # Test data
diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py
index 54b721e786..beb53325ed 100644
--- a/nova/virt/libvirt/driver.py
+++ b/nova/virt/libvirt/driver.py
@@ -5662,8 +5662,24 @@ class LibvirtDriver(driver.ComputeDriver):
         # Disconnect from volume server
         block_device_mapping = driver.block_device_info_get_mapping(
                 block_device_info)
+        connector = self.get_volume_connector(instance)
+        volume_api = self._volume_api
         for vol in block_device_mapping:
-            connection_info = vol['connection_info']
+            # Retrieve connection info from Cinder's initialize_connection API.
+            # The info returned will be accurate for the source server.
+            volume_id = vol['connection_info']['serial']
+            connection_info = volume_api.initialize_connection(context,
+                                                               volume_id,
+                                                               connector)
+
+            # Pull out multipath_id from the bdm information. The
+            # multipath_id can be placed into the connection info
+            # because it is based off of the volume and will be the
+            # same on the source and destination hosts.
+            if 'multipath_id' in vol['connection_info']['data']:
+                multipath_id = vol['connection_info']['data']['multipath_id']
+                connection_info['data']['multipath_id'] = multipath_id
+
             disk_dev = vol['mount_device'].rpartition("/")[2]
             self._disconnect_volume(connection_info, disk_dev)