libvirt: fix native luks encryption failure to find volume_id

Not all volume types put a 'volume_id' entry in the
connection_info['data'] dict. This change uses a new
utility method to look up the volume_id in the connection_info
data dict and if not found there, uses the 'serial' value
from the connection_info, which we know at least gets set
when the DriverVolumeBlockDevice code attaches the volume.

This also has to update pre_live_migration since the connection_info
dict doesn't have a 'volume_id' key in it. It's unclear what
this code was expecting, or if it ever really worked, but since
an attached volume represented by a BlockDeviceMapping here has
a volume_id attribute, we can just use that. As that code path
was never tested, this updates related unit tests and refactors
the tests to actually use the type of DriverVolumeBlockDevice
objects that the ComputeManager would be sending down to the
driver pre_live_migration method. The hard-to-read squashed
dicts in the tests are also re-formatted so a human can actually
read them.

Change-Id: Ie02d298cd92d5b5ebcbbcd2b0e8be01f197bfafb
Closes-Bug: #1746609

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

@@ -7127,6 +7127,40 @@ def test_attach_encryptor_encrypted_volume_meta_provided(self,
         mock_encryptor.attach_volume.assert_called_once_with(self.context,
                                                              **encryption)
 
+    @mock.patch.object(key_manager, 'API')
+    @mock.patch('os_brick.encryptors.get_encryption_metadata')
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor')
+    def test_attach_encryptor_encrypted_native_luks_serial(self,
+            mock_get_encryptor, mock_get_metadata, mock_get_key_mgr):
+        """Uses native luks encryption with a provider encryptor and the
+        connection_info has a serial but not volume_id in the 'data'
+        sub-dict.
+        """
+        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
+        mock_encryptor = mock.MagicMock()
+        mock_get_encryptor.return_value = mock_encryptor
+        encryption = {'provider': 'luks', 'control_location': 'front-end',
+                      'encryption_key_id': uuids.encryption_key_id}
+        connection_info = {'serial': uuids.serial, 'data': {}}
+        # Mock out the key manager
+        key = u'3734363537333734'
+        key_encoded = binascii.unhexlify(key)
+        mock_key = mock.Mock()
+        mock_key_mgr = mock.Mock()
+        mock_get_key_mgr.return_value = mock_key_mgr
+        mock_key_mgr.get.return_value = mock_key
+        mock_key.get_encoded.return_value = key_encoded
+
+        with mock.patch.object(drvr, '_use_native_luks', return_value=True):
+            with mock.patch.object(drvr._host, 'create_secret') as crt_scrt:
+                drvr._attach_encryptor(self.context, connection_info,
+                                       encryption, allow_native_luks=True)
+
+        mock_get_metadata.assert_not_called()
+        mock_get_encryptor.assert_not_called()
+        crt_scrt.assert_called_once_with(
+            'volume', uuids.serial, password=key)
+
     @mock.patch('os_brick.encryptors.get_encryption_metadata')
     @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor')
     def test_detach_encryptor_connection_info_incomplete(self,
@@ -10473,15 +10507,42 @@ def _test_pre_live_migration_works_correctly_mocked(self,
             target_ret=None, src_supports_native_luks=True,
             dest_supports_native_luks=True, allow_native_luks=True):
         # Creating testdata
-        vol = {'block_device_mapping': [
-           {'connection_info': {'serial': '12345', u'data':
-            {'device_path':
-             u'/dev/disk/by-path/ip-1.2.3.4:3260-iqn.abc.12345.opst-lun-X'}},
-             'mount_device': '/dev/sda'},
-           {'connection_info': {'serial': '67890', u'data':
-            {'device_path':
-             u'/dev/disk/by-path/ip-1.2.3.4:3260-iqn.cde.67890.opst-lun-Z'}},
-             'mount_device': '/dev/sdb'}]}
+        c = context.get_admin_context()
+        instance = objects.Instance(root_device_name='/dev/vda',
+                                    **self.test_instance)
+        bdms = objects.BlockDeviceMappingList(objects=[
+            fake_block_device.fake_bdm_object(c, {
+                'connection_info': jsonutils.dumps({
+                    'serial': '12345',
+                    'data': {
+                        'device_path': '/dev/disk/by-path/ip-1.2.3.4:3260'
+                                       '-iqn.abc.12345.opst-lun-X'
+                    }
+                }),
+                'device_name': '/dev/sda',
+                'volume_id': uuids.volume1,
+                'source_type': 'volume',
+                'destination_type': 'volume'
+            }),
+            fake_block_device.fake_bdm_object(c, {
+                'connection_info': jsonutils.dumps({
+                    'serial': '67890',
+                    'data': {
+                        'device_path': '/dev/disk/by-path/ip-1.2.3.4:3260'
+                                       '-iqn.cde.67890.opst-lun-Z'
+                    }
+                }),
+                'device_name': '/dev/sdb',
+                'volume_id': uuids.volume2,
+                'source_type': 'volume',
+                'destination_type': 'volume'
+            })
+        ])
+        # We go through get_block_device_info to simulate what the
+        # ComputeManager sends to the driver (make sure we're using the
+        # correct type of BDM objects since there are many of them and
+        # they are super confusing).
+        block_device_info = driver.get_block_device_info(instance, bdms)
 
         drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
 
@@ -10496,16 +10557,11 @@ def fake_none(*args, **kwargs):
         self.stubs.Set(drvr, '_is_native_luks_available',
                         lambda: dest_supports_native_luks)
 
-        instance = objects.Instance(**self.test_instance)
-        c = context.get_admin_context()
         nw_info = FakeNetworkInfo()
 
         # Creating mocks
-        self.mox.StubOutWithMock(driver, "block_device_info_get_mapping")
-        driver.block_device_info_get_mapping(vol
-            ).AndReturn(vol['block_device_mapping'])
         self.mox.StubOutWithMock(drvr, "_connect_volume")
-        for v in vol['block_device_mapping']:
+        for v in block_device_info['block_device_mapping']:
             drvr._connect_volume(c, v['connection_info'], instance,
                                  allow_native_luks=allow_native_luks)
         self.mox.StubOutWithMock(drvr, 'plug_vifs')
@@ -10526,14 +10582,14 @@ def fake_none(*args, **kwargs):
             migrate_data.src_supports_native_luks = True
 
         result = drvr.pre_live_migration(
-            c, instance, vol, nw_info, None,
+            c, instance, block_device_info, nw_info, None,
             migrate_data=migrate_data)
         if not target_ret:
             target_ret = self._generate_target_ret()
         self.assertEqual(
+            target_ret,
             result.to_legacy_dict(
-                pre_migration_result=True)['pre_live_migration_result'],
-            target_ret)
+                pre_migration_result=True)['pre_live_migration_result'])
 
     @mock.patch.object(os, 'mkdir')
     @mock.patch('nova.virt.libvirt.utils.get_instance_path_at_destination')
@@ -10617,15 +10673,42 @@ def test_pre_live_migration_vol_backed_works_correctly_mocked(self):
         # Creating testdata, using temp dir.
         with utils.tempdir() as tmpdir:
             self.flags(instances_path=tmpdir)
-            vol = {'block_device_mapping': [
-             {'connection_info': {'serial': '12345', u'data':
-             {'device_path':
-              u'/dev/disk/by-path/ip-1.2.3.4:3260-iqn.abc.12345.opst-lun-X'}},
-             'mount_device': '/dev/sda'},
-             {'connection_info': {'serial': '67890', u'data':
-             {'device_path':
-             u'/dev/disk/by-path/ip-1.2.3.4:3260-iqn.cde.67890.opst-lun-Z'}},
-             'mount_device': '/dev/sdb'}]}
+            c = context.get_admin_context()
+            inst_ref = objects.Instance(root_device_name='/dev/vda',
+                                        **self.test_instance)
+            bdms = objects.BlockDeviceMappingList(objects=[
+                fake_block_device.fake_bdm_object(c, {
+                    'connection_info': jsonutils.dumps({
+                        'serial': '12345',
+                        'data': {
+                            'device_path': '/dev/disk/by-path/ip-1.2.3.4:3260'
+                                           '-iqn.abc.12345.opst-lun-X'
+                        }
+                    }),
+                    'device_name': '/dev/sda',
+                    'volume_id': uuids.volume1,
+                    'source_type': 'volume',
+                    'destination_type': 'volume'
+                }),
+                fake_block_device.fake_bdm_object(c, {
+                    'connection_info': jsonutils.dumps({
+                        'serial': '67890',
+                        'data': {
+                            'device_path': '/dev/disk/by-path/ip-1.2.3.4:3260'
+                                           '-iqn.cde.67890.opst-lun-Z'
+                        }
+                    }),
+                    'device_name': '/dev/sdb',
+                    'volume_id': uuids.volume2,
+                    'source_type': 'volume',
+                    'destination_type': 'volume'
+                })
+            ])
+            # We go through get_block_device_info to simulate what the
+            # ComputeManager sends to the driver (make sure we're using the
+            # correct type of BDM objects since there are many of them and
+            # they are super confusing).
+            block_device_info = driver.get_block_device_info(inst_ref, bdms)
 
             drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
 
@@ -10638,12 +10721,10 @@ def fake_none(*args, **kwargs):
             class FakeNetworkInfo(object):
                 def fixed_ips(self):
                     return ["test_ip_addr"]
-            inst_ref = objects.Instance(**self.test_instance)
-            c = context.get_admin_context()
             nw_info = FakeNetworkInfo()
             # Creating mocks
             self.mox.StubOutWithMock(drvr, "_connect_volume")
-            for v in vol['block_device_mapping']:
+            for v in block_device_info['block_device_mapping']:
                 drvr._connect_volume(c, v['connection_info'], inst_ref,
                                      allow_native_luks=True)
             self.mox.StubOutWithMock(drvr, 'plug_vifs')
@@ -10661,9 +10742,9 @@ def fixed_ips(self):
                 filename='foo',
                 src_supports_native_luks=True,
             )
-            ret = drvr.pre_live_migration(c, inst_ref, vol, nw_info, None,
-                                          migrate_data)
-            target_ret = {
+            ret = drvr.pre_live_migration(
+                c, inst_ref, block_device_info, nw_info, None, migrate_data)
+            expected_result = {
             'graphics_listen_addrs': {'spice': None,
                                       'vnc': None},
             'target_connect_addr': None,
@@ -10682,8 +10763,8 @@ def fixed_ips(self):
                                     'dev': 'sdb',
                                     'type': 'disk'}}}}
             self.assertEqual(
-                ret.to_legacy_dict(True)['pre_live_migration_result'],
-                target_ret)
+                expected_result,
+                ret.to_legacy_dict(True)['pre_live_migration_result'])
             self.assertTrue(os.path.exists('%s/%s/' % (tmpdir,
                                                        inst_ref['name'])))
 

nova/tests/unit/virt/test_block_device.py

@@ -1274,3 +1274,28 @@ def test_volume_attach_multiattach_no_virt_driver_support(self):
         self.assertRaises(exception.MultiattachNotSupportedByVirtDriver,
                           test_bdm.attach, self.context, instance,
                           self.volume_api, self.virt_driver)
+
+
+class TestGetVolumeId(test.NoDBTestCase):
+
+    def test_get_volume_id_none_found(self):
+        self.assertIsNone(driver_block_device.get_volume_id(None))
+        self.assertIsNone(driver_block_device.get_volume_id({}))
+        self.assertIsNone(driver_block_device.get_volume_id({'data': {}}))
+
+    def test_get_volume_id_found_volume_id_no_serial(self):
+        self.assertEqual(uuids.volume_id,
+                         driver_block_device.get_volume_id(
+                             {'data': {'volume_id': uuids.volume_id}}))
+
+    def test_get_volume_id_found_no_volume_id_serial(self):
+        self.assertEqual(uuids.serial,
+                         driver_block_device.get_volume_id(
+                             {'serial': uuids.serial}))
+
+    def test_get_volume_id_found_both(self):
+        # volume_id is taken over serial
+        self.assertEqual(uuids.volume_id,
+                         driver_block_device.get_volume_id(
+                             {'serial': uuids.serial,
+                              'data': {'volume_id': uuids.volume_id}}))

nova/virt/block_device.py

@@ -880,3 +880,13 @@ def is_block_device_mapping(bdm):
     return (bdm.source_type in ('image', 'volume', 'snapshot', 'blank')
             and bdm.destination_type == 'volume'
             and is_implemented(bdm))
+
+
+def get_volume_id(connection_info):
+    if connection_info:
+        # Check for volume_id in 'data' and if not there, fallback to
+        # the 'serial' that the DriverVolumeBlockDevice adds during attach.
+        volume_id = connection_info.get('data', {}).get('volume_id')
+        if not volume_id:
+            volume_id = connection_info.get('serial')
+        return volume_id

nova/virt/libvirt/driver.py

@@ -1274,8 +1274,8 @@ def _get_volume_encryption(self, context, connection_info):
         """Get the encryption metadata dict if it is not provided
         """
         encryption = {}
-        if connection_info.get('data', {}).get('volume_id'):
-            volume_id = connection_info['data']['volume_id']
+        volume_id = driver_block_device.get_volume_id(connection_info)
+        if volume_id:
             encryption = encryptors.get_encryption_metadata(context,
                             self._volume_api, volume_id, connection_info)
         return encryption
@@ -1323,7 +1323,7 @@ def _attach_encryptor(self, context, connection_info, encryption,
             # NOTE(lyarwood): Store the passphrase as a libvirt secret locally
             # on the compute node. This secret is used later when generating
             # the volume config.
-            volume_id = connection_info.get('data', {}).get('volume_id')
+            volume_id = driver_block_device.get_volume_id(connection_info)
             self._host.create_secret('volume', volume_id, password=passphrase)
         elif encryption:
             encryptor = self._get_volume_encryptor(connection_info,
@@ -1340,7 +1340,7 @@ def _detach_encryptor(self, context, connection_info, encryption):
         If native LUKS decryption is enabled then delete previously created
         Libvirt volume secret from the host.
         """
-        volume_id = connection_info.get('data', {}).get('volume_id')
+        volume_id = driver_block_device.get_volume_id(connection_info)
         if volume_id and self._host.find_secret('volume', volume_id):
             return self._host.delete_secret('volume', volume_id)
         if encryption is None:
@@ -7427,10 +7427,7 @@ def pre_live_migration(self, context, instance, block_device_info,
                 bdmi.type = disk_info['type']
                 bdmi.format = disk_info.get('format')
                 bdmi.boot_index = disk_info.get('boot_index')
-                volume_id = connection_info.get('volume_id')
-                volume_secret = None
-                if volume_id:
-                    volume_secret = self._host.find_secret('volume', volume_id)
+                volume_secret = self._host.find_secret('volume', vol.volume_id)
                 if volume_secret:
                     bdmi.encryption_secret_uuid = volume_secret.UUIDString()
 

nova/virt/libvirt/volume/volume.py

@@ -22,6 +22,7 @@
 import nova.conf
 from nova import exception
 from nova import profiler
+from nova.virt import block_device as driver_block_device
 from nova.virt.libvirt import config as vconfig
 import nova.virt.libvirt.driver
 from nova.virt.libvirt import utils as libvirt_utils
@@ -109,7 +110,7 @@ def get_config(self, connection_info, disk_info):
             # a shareable disk.
             conf.shareable = True
 
-        volume_id = connection_info.get('data', {}).get('volume_id')
+        volume_id = driver_block_device.get_volume_id(connection_info)
         volume_secret = None
         if volume_id:
             volume_secret = self.host.find_secret('volume', volume_id)