libvirt: Collocate encryptor and volume driver calls

This change introduces new utility methods for attaching and detaching
frontend volume encryptors. These methods centralise the optional
fetching of encryption metadata associated with a volume, fetching of the
required encryptor and calls to detach or attach the encryptor.

These new utility methods are called either after initially connecting
to or before disconnecting from a volume. This ensures encryptors are
correctly connected when swapping volumes for example, where previously
no attempt was made to attach an encryptor to the target volume.

The request context is provided to swap_volume and various other config
generation related methods to allow for the lookup of the relevant
encryption metadata if it is not provided.

Closes-bug: #1739593
Change-Id: Ica323b87fa85a454fca9d46ada3677f18fe50022

nova/compute/manager.py

@@ -5438,8 +5438,8 @@ class ComputeManager(manager.Manager):
                       "old: %(old_cinfo)s",
                       {'new_cinfo': new_cinfo, 'old_cinfo': old_cinfo},
                       instance=instance)
-            self.driver.swap_volume(old_cinfo, new_cinfo, instance, mountpoint,
+            self.driver.swap_volume(context, old_cinfo, new_cinfo, instance,
-                                    resize_to)
+                                    mountpoint, resize_to)
             if new_attachment_id:
                 self.volume_api.attachment_complete(context, new_attachment_id)
             LOG.debug("swap_volume: Driver volume swap returned, new "

nova/tests/unit/compute/test_compute_mgr.py

@@ -1966,8 +1966,9 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
         self.assertTrue(uuidutils.is_uuid_like(volume))
         return {}
 
-    def _assert_swap_volume(self, old_connection_info, new_connection_info,
+    def _assert_swap_volume(self, context, old_connection_info,
-                            instance, mountpoint, resize_to):
+                            new_connection_info, instance, mountpoint,
+                            resize_to):
         self.assertEqual(2, resize_to)
 
     @mock.patch.object(cinder.API, 'initialize_connection')
@@ -2306,7 +2307,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
                 instance, uuids.new_attachment_id)
             # Assert the expected calls.
             # The new connection_info has the new_volume_id as the serial.
-            new_cinfo = mock_driver_swap.call_args[0][1]
+            new_cinfo = mock_driver_swap.call_args[0][2]
             self.assertIn('serial', new_cinfo)
             self.assertEqual(uuids.new_volume_id, new_cinfo['serial'])
             get_bdm.assert_called_once_with(

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

@@ -6706,7 +6706,7 @@ class LibvirtConnTestCase(test.NoDBTestCase,
                     test.MatchType(objects.ImageMeta),
                     bdm)
                 mock_connect_volume.assert_called_with(
-                    connection_info, instance)
+                    self.context, connection_info, instance, encryption=None)
                 mock_get_volume_config.assert_called_with(
                     connection_info, disk_info)
                 mock_dom.attachDeviceFlags.assert_called_with(
@@ -6761,7 +6761,7 @@ class LibvirtConnTestCase(test.NoDBTestCase,
 </disk>
 """, flags=flags)
                 mock_disconnect_volume.assert_called_with(
-                    connection_info, instance)
+                    None, connection_info, instance, encryption=None)
 
     @mock.patch('nova.virt.libvirt.host.Host._get_domain')
     def test_detach_volume_disk_not_found(self, mock_get_domain):
@@ -6785,12 +6785,15 @@ class LibvirtConnTestCase(test.NoDBTestCase,
 
         mock_get_domain.assert_called_once_with(instance)
 
-    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume')
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_driver')
     @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor')
     @mock.patch('nova.virt.libvirt.host.Host.get_guest')
     def test_detach_volume_order_with_encryptors(self, mock_get_guest,
-            mock_get_encryptor, mock_disconnect_volume):
+            mock_get_encryptor, mock_get_volume_driver):
 
+        mock_volume_driver = mock.MagicMock(
+            spec=volume_drivers.LibvirtBaseVolumeDriver)
+        mock_get_volume_driver.return_value = mock_volume_driver
         mock_guest = mock.MagicMock(spec=libvirt_guest.Guest)
         mock_guest.get_power_state.return_value = power_state.RUNNING
         mock_get_guest.return_value = mock_guest
@@ -6799,7 +6802,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
         mock_get_encryptor.return_value = mock_encryptor
 
         mock_order = mock.Mock()
-        mock_order.attach_mock(mock_disconnect_volume, 'disconnect_volume')
+        mock_order.attach_mock(mock_volume_driver.disconnect_volume,
+                'disconnect_volume')
         mock_order.attach_mock(mock_guest.detach_device_with_retry(),
                 'detach_volume')
         mock_order.attach_mock(mock_encryptor.detach_volume,
@@ -6917,6 +6921,196 @@ class LibvirtConnTestCase(test.NoDBTestCase,
                           drvr.extend_volume,
                           connection_info, instance)
 
+    @mock.patch('os_brick.encryptors.get_encryption_metadata')
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor')
+    def test_use_encryptor_connection_info_incomplete(self,
+            mock_get_encryptor, mock_get_metadata):
+        """Assert no attach attempt is made given incomplete connection_info.
+        """
+        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
+        connection_info = {'data': {}}
+
+        drvr._attach_encryptor(self.context, connection_info, None)
+
+        mock_get_metadata.assert_not_called()
+        mock_get_encryptor.assert_not_called()
+
+    @mock.patch('os_brick.encryptors.get_encryption_metadata')
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor')
+    def test_attach_encryptor_unencrypted_volume_meta_missing(self,
+            mock_get_encryptor, mock_get_metadata):
+        """Assert that if not provided encryption metadata is fetched even
+        if the volume is ultimately unencrypted and no attempt to attach
+        is made.
+        """
+        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
+        encryption = {}
+        connection_info = {'data': {'volume_id': uuids.volume_id}}
+        mock_get_metadata.return_value = encryption
+
+        drvr._attach_encryptor(self.context, connection_info, None)
+
+        mock_get_metadata.assert_called_once_with(self.context,
+                drvr._volume_api, uuids.volume_id, connection_info)
+        mock_get_encryptor.assert_not_called()
+
+    @mock.patch('os_brick.encryptors.get_encryption_metadata')
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor')
+    def test_attach_encryptor_unencrypted_volume_meta_provided(self,
+            mock_get_encryptor, mock_get_metadata):
+        """Assert that if an empty encryption metadata dict is provided that
+        there is no additional attempt to lookup the metadata or attach the
+        encryptor.
+        """
+        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
+        encryption = {}
+        connection_info = {'data': {'volume_id': uuids.volume_id}}
+
+        drvr._attach_encryptor(self.context, connection_info,
+                               encryption=encryption)
+
+        mock_get_metadata.assert_not_called()
+        mock_get_encryptor.assert_not_called()
+
+    @mock.patch('os_brick.encryptors.get_encryption_metadata')
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor')
+    def test_attach_encryptor_encrypted_volume_meta_missing(self,
+            mock_get_encryptor, mock_get_metadata):
+        """Assert that if missing the encryption metadata of an encrypted
+        volume is fetched and then used to attach the encryptor for the volume.
+        """
+        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'}
+        mock_get_metadata.return_value = encryption
+        connection_info = {'data': {'volume_id': uuids.volume_id}}
+
+        drvr._attach_encryptor(self.context, connection_info, None)
+
+        mock_get_metadata.assert_called_once_with(self.context,
+                drvr._volume_api, uuids.volume_id, connection_info)
+        mock_get_encryptor.assert_called_once_with(connection_info,
+                                                   encryption)
+        mock_encryptor.attach_volume.assert_called_once_with(self.context,
+                                                             **encryption)
+
+    @mock.patch('os_brick.encryptors.get_encryption_metadata')
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor')
+    def test_attach_encryptor_encrypted_volume_meta_provided(self,
+            mock_get_encryptor, mock_get_metadata):
+        """Assert that when provided there are no further attempts to fetch the
+        encryption metadata for the volume and that the provided metadata is
+        then used to attach the volume.
+        """
+        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'}
+        connection_info = {'data': {'volume_id': uuids.volume_id}}
+
+        drvr._attach_encryptor(self.context, connection_info,
+                encryption=encryption)
+
+        mock_get_metadata.assert_not_called()
+        mock_get_encryptor.assert_called_once_with(connection_info,
+                                                   encryption)
+        mock_encryptor.attach_volume.assert_called_once_with(self.context,
+                                                             **encryption)
+
+    @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,
+            mock_get_encryptor, mock_get_metadata):
+        """Assert no detach attempt is made given incomplete connection_info.
+        """
+        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
+        connection_info = {'data': {}}
+
+        drvr._detach_encryptor(self.context, connection_info, None)
+
+        mock_get_metadata.assert_not_called()
+        mock_get_encryptor.assert_not_called()
+
+    @mock.patch('os_brick.encryptors.get_encryption_metadata')
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor')
+    def test_detach_encryptor_unencrypted_volume_meta_missing(self,
+            mock_get_encryptor, mock_get_metadata):
+        """Assert that if not provided encryption metadata is fetched even
+        if the volume is ultimately unencrypted and no attempt to detach
+        is made.
+        """
+        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
+        encryption = {}
+        connection_info = {'data': {'volume_id': uuids.volume_id}}
+        mock_get_metadata.return_value = encryption
+
+        drvr._detach_encryptor(self.context, connection_info, None)
+
+        mock_get_metadata.assert_called_once_with(self.context,
+                drvr._volume_api, uuids.volume_id, connection_info)
+        mock_get_encryptor.assert_not_called()
+
+    @mock.patch('os_brick.encryptors.get_encryption_metadata')
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor')
+    def test_detach_encryptor_unencrypted_volume_meta_provided(self,
+            mock_get_encryptor, mock_get_metadata):
+        """Assert that if an empty encryption metadata dict is provided that
+        there is no additional attempt to lookup the metadata or detach the
+        encryptor.
+        """
+        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
+        encryption = {}
+        connection_info = {'data': {'volume_id': uuids.volume_id}}
+
+        drvr._detach_encryptor(self.context, connection_info, encryption)
+
+        mock_get_metadata.assert_not_called()
+        mock_get_encryptor.assert_not_called()
+
+    @mock.patch('os_brick.encryptors.get_encryption_metadata')
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor')
+    def test_detach_encryptor_encrypted_volume_meta_missing(self,
+            mock_get_encryptor, mock_get_metadata):
+        """Assert that if missing the encryption metadata of an encrypted
+        volume is fetched and then used to detach the encryptor for the volume.
+        """
+        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'}
+        mock_get_metadata.return_value = encryption
+        connection_info = {'data': {'volume_id': uuids.volume_id}}
+
+        drvr._detach_encryptor(self.context, connection_info, None)
+
+        mock_get_metadata.assert_called_once_with(self.context,
+                drvr._volume_api, uuids.volume_id, connection_info)
+        mock_get_encryptor.assert_called_once_with(connection_info,
+                                                   encryption)
+        mock_encryptor.detach_volume.assert_called_once_with(**encryption)
+
+    @mock.patch('os_brick.encryptors.get_encryption_metadata')
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor')
+    def test_detach_encryptor_encrypted_volume_meta_provided(self,
+            mock_get_encryptor, mock_get_metadata):
+        """Assert that when provided there are no further attempts to fetch the
+        encryption metadata for the volume and that the provided metadata is
+        then used to detach the volume.
+        """
+        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'}
+        connection_info = {'data': {'volume_id': uuids.volume_id}}
+
+        drvr._detach_encryptor(self.context, connection_info, encryption)
+
+        mock_get_metadata.assert_not_called()
+        mock_get_encryptor.assert_called_once_with(connection_info,
+                                                   encryption)
+        mock_encryptor.detach_volume.assert_called_once_with(**encryption)
+
     def test_multi_nic(self):
         network_info = _fake_network_info(self, 2)
         drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)
@@ -10146,7 +10340,7 @@ class LibvirtConnTestCase(test.NoDBTestCase,
             ).AndReturn(vol['block_device_mapping'])
         self.mox.StubOutWithMock(drvr, "_connect_volume")
         for v in vol['block_device_mapping']:
-            drvr._connect_volume(v['connection_info'], instance)
+            drvr._connect_volume(c, v['connection_info'], instance)
         self.mox.StubOutWithMock(drvr, 'plug_vifs')
         drvr.plug_vifs(mox.IsA(instance), nw_info)
 
@@ -10278,7 +10472,7 @@ class LibvirtConnTestCase(test.NoDBTestCase,
             # Creating mocks
             self.mox.StubOutWithMock(drvr, "_connect_volume")
             for v in vol['block_device_mapping']:
-                drvr._connect_volume(v['connection_info'], inst_ref)
+                drvr._connect_volume(c, v['connection_info'], inst_ref)
             self.mox.StubOutWithMock(drvr, 'plug_vifs')
             drvr.plug_vifs(mox.IsA(inst_ref), nw_info)
             self.mox.ReplayAll()
@@ -10627,8 +10821,9 @@ class LibvirtConnTestCase(test.NoDBTestCase,
             get_volume_connector.assert_has_calls([
                 mock.call(inst_ref)])
             _disconnect_volume.assert_has_calls([
-                mock.call({'data': {'multipath_id': 'dummy1'}}, inst_ref),
+                mock.call(cntx, {'data': {'multipath_id': 'dummy1'}},
-                mock.call({'data': {}}, inst_ref)])
+                          inst_ref),
+                mock.call(cntx, {'data': {}}, inst_ref)])
 
     def test_post_live_migration_cinder_v3(self):
         cntx = context.get_admin_context()
@@ -10666,7 +10861,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
 
             mock_attachment_get.assert_called_once_with(cntx,
                                                         old_attachment_id)
-            mock_disconnect.assert_called_once_with(connection_info, instance)
+            mock_disconnect.assert_called_once_with(cntx, connection_info,
+                                                    instance)
         _test()
 
     def test_get_instance_disk_info_excludes_volumes(self):
@@ -11024,7 +11220,7 @@ class LibvirtConnTestCase(test.NoDBTestCase,
                   'delete_on_termination': False
               }
 
-        def _connect_volume_side_effect(connection_info, instance):
+        def _connect_volume_side_effect(ctxt, connection_info, instance):
             bdm['connection_info']['data']['device_path'] = '/dev/path/to/dev'
 
         def _get(key, opt=None):
@@ -14563,8 +14759,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
             connection_info = {'driver_volume_type': 'fake'}
             drvr.detach_volume(connection_info, instance, '/dev/sda')
             _get_domain.assert_called_once_with(instance)
-            _disconnect_volume.assert_called_once_with(connection_info,
+            _disconnect_volume.assert_called_once_with(None, connection_info,
-                                                       instance)
+                                instance, encryption=None)
 
     def _test_attach_detach_interface_get_config(self, method_name):
         """Tests that the get_config() method is properly called in
@@ -15047,25 +15243,20 @@ class LibvirtConnTestCase(test.NoDBTestCase,
                         network_model.VIF(id='2', active=True)]
 
         with test.nested(
-            mock.patch.object(drvr, '_get_volume_encryptor'),
             mock.patch.object(drvr, 'plug_vifs'),
             mock.patch.object(drvr.firewall_driver, 'setup_basic_filtering'),
             mock.patch.object(drvr.firewall_driver,
                               'prepare_instance_filter'),
             mock.patch.object(drvr, '_create_domain'),
             mock.patch.object(drvr.firewall_driver, 'apply_instance_filter'),
-        ) as (get_volume_encryptor, plug_vifs, setup_basic_filtering,
+        ) as (plug_vifs, setup_basic_filtering, prepare_instance_filter,
-              prepare_instance_filter, create_domain, apply_instance_filter):
+              create_domain, apply_instance_filter):
             create_domain.return_value = libvirt_guest.Guest(mock_dom)
 
             guest = drvr._create_domain_and_network(
                     self.context, fake_xml, instance, network_info,
                     block_device_info=block_device_info)
 
-            get_encryption_metadata.assert_called_once_with(self.context,
-                drvr._volume_api, fake_volume_id, connection_info)
-            get_volume_encryptor.assert_called_once_with(connection_info,
-                                                         mock_encryption_meta)
             plug_vifs.assert_called_once_with(instance, network_info)
             setup_basic_filtering.assert_called_once_with(instance,
                                                           network_info)
@@ -15109,13 +15300,14 @@ class LibvirtConnTestCase(test.NoDBTestCase,
             mock.patch.object(drvr, '_get_volume_config',
                               return_value=mock_conf)
         ) as (volume_save, connect_volume, get_volume_config):
-            devices = drvr._get_guest_storage_config(instance, image_meta,
+            devices = drvr._get_guest_storage_config(self.context, instance,
-                disk_info, False, bdi, flavor, "hvm")
+                image_meta, disk_info, False, bdi, flavor, "hvm")
 
             self.assertEqual(3, len(devices))
             self.assertEqual('/dev/vdb', instance.default_ephemeral_device)
             self.assertIsNone(instance.default_swap_device)
-            connect_volume.assert_called_with(bdm['connection_info'], instance)
+            connect_volume.assert_called_with(self.context,
+                bdm['connection_info'], instance)
             get_volume_config.assert_called_with(bdm['connection_info'],
                 {'bus': 'virtio', 'type': 'disk', 'dev': 'vdc'})
             volume_save.assert_called_once_with()
@@ -15325,14 +15517,16 @@ class LibvirtConnTestCase(test.NoDBTestCase,
         conf = mock.MagicMock(source_path='/fake-new-volume')
         get_volume_config.return_value = conf
 
-        conn.swap_volume(old_connection_info, new_connection_info, instance,
+        conn.swap_volume(self.context, old_connection_info,
-                         '/dev/vdb', 1)
+                         new_connection_info, instance, '/dev/vdb', 1)
 
         get_guest.assert_called_once_with(instance)
-        connect_volume.assert_called_once_with(new_connection_info, instance)
+        connect_volume.assert_called_once_with(self.context,
+                                               new_connection_info, instance)
 
         swap_volume.assert_called_once_with(guest, 'vdb', conf, 1)
-        disconnect_volume.assert_called_once_with(old_connection_info,
+        disconnect_volume.assert_called_once_with(self.context,
+                                                  old_connection_info,
                                                   instance)
 
     def test_swap_volume_driver_source_is_volume(self):
@@ -15366,12 +15560,12 @@ class LibvirtConnTestCase(test.NoDBTestCase,
         rebase.side_effect = exc
 
         self.assertRaises(exception.VolumeRebaseFailed, conn.swap_volume,
-                          mock.sentinel.old_connection_info,
+                          self.context, mock.sentinel.old_connection_info,
                           mock.sentinel.new_connection_info,
                           instance, '/dev/vdb', 0)
-        connect_volume.assert_called_once_with(
+        connect_volume.assert_called_once_with(self.context,
                 mock.sentinel.new_connection_info, instance)
-        disconnect_volume.assert_called_once_with(
+        disconnect_volume.assert_called_once_with(self.context,
                 mock.sentinel.new_connection_info, instance)
 
     @mock.patch('nova.virt.libvirt.guest.BlockDevice.is_job_complete')
@@ -15398,12 +15592,12 @@ class LibvirtConnTestCase(test.NoDBTestCase,
         abort_job.side_effect = [None, exc]
 
         self.assertRaises(exception.VolumeRebaseFailed, conn.swap_volume,
-                          mock.sentinel.old_connection_info,
+                          self.context, mock.sentinel.old_connection_info,
                           mock.sentinel.new_connection_info,
                           instance, '/dev/vdb', 0)
-        connect_volume.assert_called_once_with(
+        connect_volume.assert_called_once_with(self.context,
                 mock.sentinel.new_connection_info, instance)
-        disconnect_volume.assert_called_once_with(
+        disconnect_volume.assert_called_once_with(self.context,
                 mock.sentinel.new_connection_info, instance)
 
     @mock.patch('nova.virt.libvirt.guest.BlockDevice.is_job_complete')
@@ -16171,7 +16365,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase):
                           context.get_admin_context(), ins_ref, '10.0.0.2',
                           flavor_obj, None)
 
-    def _test_migrate_disk_and_power_off(self, flavor_obj,
+    def _test_migrate_disk_and_power_off(self, ctxt, flavor_obj,
                                          block_device_info=None,
                                          params_for_instance=None):
         """Test for nova.virt.libvirt.libvirt_driver.LivirtConnection
@@ -16210,21 +16404,20 @@ class LibvirtDriverTestCase(test.NoDBTestCase):
 
         # dest is different host case
         out = self.drvr.migrate_disk_and_power_off(
-               context.get_admin_context(), instance, '10.0.0.2',
+               ctxt, instance, '10.0.0.2', flavor_obj, None,
-               flavor_obj, None, block_device_info=block_device_info)
+               block_device_info=block_device_info)
         self.assertEqual(out, disk_info_text)
 
         # dest is same host case
         out = self.drvr.migrate_disk_and_power_off(
-               context.get_admin_context(), instance, '10.0.0.1',
+               ctxt, instance, '10.0.0.1', flavor_obj, None,
-               flavor_obj, None, block_device_info=block_device_info)
+               block_device_info=block_device_info)
         self.assertEqual(out, disk_info_text)
 
     def test_migrate_disk_and_power_off(self):
         flavor = {'root_gb': 10, 'ephemeral_gb': 20}
         flavor_obj = objects.Flavor(**flavor)
-
+        self._test_migrate_disk_and_power_off(self.context, flavor_obj)
-        self._test_migrate_disk_and_power_off(flavor_obj)
 
     @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume')
     def test_migrate_disk_and_power_off_boot_from_volume(self,
@@ -16240,14 +16433,14 @@ class LibvirtDriverTestCase(test.NoDBTestCase):
         flavor = {'root_gb': 1, 'ephemeral_gb': 0}
         flavor_obj = objects.Flavor(**flavor)
         # Note(Mike_D): The size of instance's ephemeral_gb is 0 gb.
-        self._test_migrate_disk_and_power_off(
+        self._test_migrate_disk_and_power_off(self.context,
             flavor_obj, block_device_info=info,
             params_for_instance={'image_ref': None,
                                  'root_gb': 10,
                                  'ephemeral_gb': 0,
                                  'flavor': {'root_gb': 10,
                                             'ephemeral_gb': 0}})
-        disconnect_volume.assert_called_with(
+        disconnect_volume.assert_called_with(self.context,
             mock.sentinel.conn_info_vda, mock.ANY)
 
     @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume')
@@ -16265,7 +16458,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase):
                  'connection_info': mock.sentinel.conn_info_vda}]}
         flavor = {'root_gb': 1, 'ephemeral_gb': 0}
         flavor_obj = objects.Flavor(**flavor)
-        self._test_migrate_disk_and_power_off(
+        self._test_migrate_disk_and_power_off(self.context,
             flavor_obj, block_device_info=info,
             params_for_instance={
                 'image_ref': uuids.fake_volume_backed_image_ref,
@@ -16273,7 +16466,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase):
                 'ephemeral_gb': 0,
                 'flavor': {'root_gb': 10,
                            'ephemeral_gb': 0}})
-        disconnect_volume.assert_called_with(
+        disconnect_volume.assert_called_with(self.context,
             mock.sentinel.conn_info_vda, mock.ANY)
 
     @mock.patch('nova.utils.execute')
@@ -16540,7 +16733,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase):
         # Old flavor, eph is 20, real disk is 3, target is 4
         flavor = {'root_gb': 10, 'ephemeral_gb': 4}
         flavor_obj = objects.Flavor(**flavor)
-        self._test_migrate_disk_and_power_off(flavor_obj)
+        self._test_migrate_disk_and_power_off(self.context, flavor_obj)
 
     @mock.patch('nova.utils.execute')
     @mock.patch('nova.virt.libvirt.utils.copy_image')

nova/tests/unit/virt/test_virt_drivers.py

@@ -494,7 +494,7 @@ class _VirtDriverTestCase(_FakeDriverBackendTestCase):
                                           instance_ref,
                                           '/dev/sda'))
         self.assertIsNone(
-            self.connection.swap_volume({'driver_volume_type': 'fake',
+            self.connection.swap_volume(None, {'driver_volume_type': 'fake',
                                          'data': {}},
                                         {'driver_volume_type': 'fake',
                                          'data': {}},

nova/virt/driver.py

@@ -471,10 +471,11 @@ class ComputeDriver(object):
         """Detach the disk attached to the instance."""
         raise NotImplementedError()
 
-    def swap_volume(self, old_connection_info, new_connection_info,
+    def swap_volume(self, context, old_connection_info, new_connection_info,
                     instance, mountpoint, resize_to):
         """Replace the volume attached to the given `instance`.
 
+        :param context: The request context.
         :param dict old_connection_info:
             The volume for this connection gets detached from the given
             `instance`.

nova/virt/fake.py

@@ -317,7 +317,7 @@ class FakeDriver(driver.ComputeDriver):
         except KeyError:
             pass
 
-    def swap_volume(self, old_connection_info, new_connection_info,
+    def swap_volume(self, context, old_connection_info, new_connection_info,
                     instance, mountpoint, resize_to):
         """Replace the disk attached to the instance."""
         instance_name = instance.name

nova/virt/libvirt/driver.py

@@ -1008,23 +1008,8 @@ class LibvirtDriver(driver.ComputeDriver):
             disk_dev = vol['mount_device']
             if disk_dev is not None:
                 disk_dev = disk_dev.rpartition("/")[2]
-
-            if ('data' in connection_info and
-                    'volume_id' in connection_info['data']):
-                volume_id = connection_info['data']['volume_id']
-                encryption = encryptors.get_encryption_metadata(
-                    context, self._volume_api, volume_id, connection_info)
-
-                if encryption:
-                    # The volume must be detached from the VM before
-                    # disconnecting it from its encryptor. Otherwise, the
-                    # encryptor may report that the volume is still in use.
-                    encryptor = self._get_volume_encryptor(connection_info,
-                                                           encryption)
-                    encryptor.detach_volume(**encryption)
-
             try:
-                self._disconnect_volume(connection_info, instance)
+                self._disconnect_volume(context, connection_info, instance)
             except Exception as exc:
                 with excutils.save_and_reraise_exception() as ctxt:
                     if destroy_disks:
@@ -1207,11 +1192,15 @@ class LibvirtDriver(driver.ComputeDriver):
             raise exception.VolumeDriverNotFound(driver_type=driver_type)
         return self.volume_drivers[driver_type]
 
-    def _connect_volume(self, connection_info, instance):
+    def _connect_volume(self, context, connection_info, instance,
+                        encryption=None):
         vol_driver = self._get_volume_driver(connection_info)
         vol_driver.connect_volume(connection_info, instance)
+        self._attach_encryptor(context, connection_info, encryption=encryption)
 
-    def _disconnect_volume(self, connection_info, instance):
+    def _disconnect_volume(self, context, connection_info, instance,
+                           encryption=None):
+        self._detach_encryptor(context, connection_info, encryption=encryption)
         vol_driver = self._get_volume_driver(connection_info)
         vol_driver.disconnect_volume(connection_info, instance)
 
@@ -1232,6 +1221,44 @@ class LibvirtDriver(driver.ComputeDriver):
                                                connection_info=connection_info,
                                                **encryption)
 
+    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']
+            encryption = encryptors.get_encryption_metadata(context,
+                            self._volume_api, volume_id, connection_info)
+        return encryption
+
+    def _attach_encryptor(self, context, connection_info, encryption):
+        """Attach the frontend encryptor if one is required by the volume.
+
+        The request context is only used when an encryption metadata dict is
+        not provided. The encryption metadata dict being populated is then used
+        to determine if an attempt to attach the encryptor should be made.
+        """
+        if encryption is None:
+            encryption = self._get_volume_encryption(context, connection_info)
+        if encryption:
+            encryptor = self._get_volume_encryptor(connection_info,
+                                                   encryption)
+            encryptor.attach_volume(context, **encryption)
+
+    def _detach_encryptor(self, context, connection_info, encryption):
+        """Detach the frontend encryptor if one is required by the volume.
+
+        The request context is only used when an encryption metadata dict is
+        not provided. The encryption metadata dict being populated is then used
+        to determine if an attempt to detach the encryptor should be made.
+        """
+        if encryption is None:
+            encryption = self._get_volume_encryption(context, connection_info)
+        if encryption:
+            encryptor = self._get_volume_encryptor(connection_info,
+                                                   encryption)
+            encryptor.detach_volume(**encryption)
+
     def _check_discard_for_attach_volume(self, conf, instance):
         """Perform some checks for volumes configured for discard support.
 
@@ -1274,7 +1301,8 @@ class LibvirtDriver(driver.ComputeDriver):
                         "block size") % CONF.libvirt.virt_type
                 raise exception.InvalidHypervisorType(msg)
 
-        self._connect_volume(connection_info, instance)
+        self._connect_volume(context, connection_info, instance,
+                             encryption=encryption)
         disk_info = blockinfo.get_info_from_bdm(
             instance, CONF.libvirt.virt_type, instance.image_meta, bdm)
         if disk_info['bus'] == 'scsi':
@@ -1288,11 +1316,6 @@ class LibvirtDriver(driver.ComputeDriver):
             state = guest.get_power_state(self._host)
             live = state in (power_state.RUNNING, power_state.PAUSED)
 
-            if encryption:
-                encryptor = self._get_volume_encryptor(connection_info,
-                                                       encryption)
-                encryptor.attach_volume(context, **encryption)
-
             guest.attach_device(conf, persistent=True, live=live)
             # NOTE(artom) If we're attaching with a device role tag, we need to
             # rebuild device_metadata. If we're attaching without a role
@@ -1309,7 +1332,8 @@ class LibvirtDriver(driver.ComputeDriver):
             LOG.exception(_('Failed to attach volume at mountpoint: %s'),
                           mountpoint, instance=instance)
             with excutils.save_and_reraise_exception():
-                self._disconnect_volume(connection_info, instance)
+                self._disconnect_volume(context, connection_info, instance,
+                                        encryption=encryption)
 
     def _swap_volume(self, guest, disk_path, conf, resize_to):
         """Swap existing disk with a new block device."""
@@ -1367,7 +1391,7 @@ class LibvirtDriver(driver.ComputeDriver):
         finally:
             self._host.write_instance_config(xml)
 
-    def swap_volume(self, old_connection_info,
+    def swap_volume(self, context, old_connection_info,
                     new_connection_info, instance, mountpoint, resize_to):
 
         guest = self._host.get_guest(instance)
@@ -1388,19 +1412,19 @@ class LibvirtDriver(driver.ComputeDriver):
         # LibvirtConfigGuestDisk object it returns. We do not explicitly save
         # this to the BDM here as the upper compute swap_volume method will
         # eventually do this for us.
-        self._connect_volume(new_connection_info, instance)
+        self._connect_volume(context, new_connection_info, instance)
         conf = self._get_volume_config(new_connection_info, disk_info)
         if not conf.source_path:
-            self._disconnect_volume(new_connection_info, instance)
+            self._disconnect_volume(context, new_connection_info, instance)
             raise NotImplementedError(_("Swap only supports host devices"))
 
         try:
             self._swap_volume(guest, disk_dev, conf, resize_to)
         except exception.VolumeRebaseFailed:
             with excutils.save_and_reraise_exception():
-                self._disconnect_volume(new_connection_info, instance)
+                self._disconnect_volume(context, new_connection_info, instance)
 
-        self._disconnect_volume(old_connection_info, instance)
+        self._disconnect_volume(context, old_connection_info, instance)
 
     def _get_existing_domain_xml(self, instance, network_info,
                                  block_device_info=None):
@@ -1426,20 +1450,15 @@ class LibvirtDriver(driver.ComputeDriver):
 
             state = guest.get_power_state(self._host)
             live = state in (power_state.RUNNING, power_state.PAUSED)
-
+            # NOTE(lyarwood): The volume must be detached from the VM before
-            # The volume must be detached from the VM before disconnecting it
+            # detaching any attached encryptors or disconnecting the underlying
-            # from its encryptor. Otherwise, the encryptor may report that the
+            # volume in _disconnect_volume. Otherwise, the encryptor or volume
-            # volume is still in use.
+            # driver may report that the volume is still in use.
             wait_for_detach = guest.detach_device_with_retry(guest.get_disk,
                                                              disk_dev,
                                                              live=live)
             wait_for_detach()
 
-            if encryption:
-                encryptor = self._get_volume_encryptor(connection_info,
-                                                       encryption)
-                encryptor.detach_volume(**encryption)
-
         except exception.InstanceNotFound:
             # NOTE(zhaoqin): If the instance does not exist, _lookup_by_name()
             #                will throw InstanceNotFound exception. Need to
@@ -1460,7 +1479,12 @@ class LibvirtDriver(driver.ComputeDriver):
             else:
                 raise
 
-        self._disconnect_volume(connection_info, instance)
+        # NOTE(lyarwood): We can provide None as the request context here as we
+        # already have the encryption metadata dict from the compute layer.
+        # This avoids the need to add the request context to the signature of
+        # detach_volume requiring changes across all drivers.
+        self._disconnect_volume(None, connection_info, instance,
+                                encryption=encryption)
 
     def extend_volume(self, connection_info, instance):
         try:
@@ -3682,7 +3706,7 @@ class LibvirtDriver(driver.ComputeDriver):
         disk = self.image_backend.by_name(instance, name, image_type)
         return disk.libvirt_fs_info("/", "ploop")
 
-    def _get_guest_storage_config(self, instance, image_meta,
+    def _get_guest_storage_config(self, context, instance, image_meta,
                                   disk_info,
                                   rescue, block_device_info,
                                   inst_type, os_type):
@@ -3793,7 +3817,7 @@ class LibvirtDriver(driver.ComputeDriver):
             connection_info = vol['connection_info']
             vol_dev = block_device.prepend_dev(vol['mount_device'])
             info = disk_mapping[vol_dev]
-            self._connect_volume(connection_info, instance)
+            self._connect_volume(context, connection_info, instance)
             if scsi_controller and scsi_controller.model == 'virtio-scsi':
                 info['unit'] = disk_mapping['unit']
                 disk_mapping['unit'] += 1
@@ -4881,7 +4905,7 @@ class LibvirtDriver(driver.ComputeDriver):
                            image_meta)
         self._set_clock(guest, instance.os_type, image_meta, virt_type)
 
-        storage_configs = self._get_guest_storage_config(
+        storage_configs = self._get_guest_storage_config(context,
                 instance, image_meta, disk_info, rescue, block_device_info,
                 flavor, guest.os_type)
         for config in storage_configs:
@@ -5097,14 +5121,15 @@ class LibvirtDriver(driver.ComputeDriver):
         # workaround, see libvirt/compat.py
         return guest.get_info(self._host)
 
-    def _create_domain_setup_lxc(self, instance, image_meta,
+    def _create_domain_setup_lxc(self, context, instance, image_meta,
                                  block_device_info):
         inst_path = libvirt_utils.get_instance_path(instance)
         block_device_mapping = driver.block_device_info_get_mapping(
             block_device_info)
         root_disk = block_device.get_root_bdm(block_device_mapping)
         if root_disk:
-            self._connect_volume(root_disk['connection_info'], instance)
+            self._connect_volume(context, root_disk['connection_info'],
+                                 instance)
             disk_path = root_disk['connection_info']['data']['device_path']
 
             # NOTE(apmelton) - Even though the instance is being booted from a
@@ -5153,7 +5178,8 @@ class LibvirtDriver(driver.ComputeDriver):
             disk_api.teardown_container(container_dir=container_dir)
 
     @contextlib.contextmanager
-    def _lxc_disk_handler(self, instance, image_meta, block_device_info):
+    def _lxc_disk_handler(self, context, instance, image_meta,
+                          block_device_info):
         """Context manager to handle the pre and post instance boot,
            LXC specific disk operations.
 
@@ -5167,7 +5193,8 @@ class LibvirtDriver(driver.ComputeDriver):
             yield
             return
 
-        self._create_domain_setup_lxc(instance, image_meta, block_device_info)
+        self._create_domain_setup_lxc(context, instance, image_meta,
+                                      block_device_info)
 
         try:
             yield
@@ -5233,23 +5260,6 @@ class LibvirtDriver(driver.ComputeDriver):
                                    destroy_disks_on_failure=False):
 
         """Do required network setup and create domain."""
-        block_device_mapping = driver.block_device_info_get_mapping(
-            block_device_info)
-
-        for vol in block_device_mapping:
-            connection_info = vol['connection_info']
-
-            if ('data' in connection_info and
-                    'volume_id' in connection_info['data']):
-                volume_id = connection_info['data']['volume_id']
-                encryption = encryptors.get_encryption_metadata(
-                    context, self._volume_api, volume_id, connection_info)
-
-                if encryption:
-                    encryptor = self._get_volume_encryptor(connection_info,
-                                                           encryption)
-                    encryptor.attach_volume(context, **encryption)
-
         timeout = CONF.vif_plugging_timeout
         if (self._conn_supports_start_paused and
             utils.is_neutron() and not
@@ -5269,7 +5279,8 @@ class LibvirtDriver(driver.ComputeDriver):
                                                            network_info)
                 self.firewall_driver.prepare_instance_filter(instance,
                                                              network_info)
-                with self._lxc_disk_handler(instance, instance.image_meta,
+                with self._lxc_disk_handler(context, instance,
+                                            instance.image_meta,
                                             block_device_info):
                     guest = self._create_domain(
                         xml, pause=pause, power_on=power_on,
@@ -7061,7 +7072,7 @@ class LibvirtDriver(driver.ComputeDriver):
 
         for bdm in block_device_mapping:
             connection_info = bdm['connection_info']
-            self._connect_volume(connection_info, instance)
+            self._connect_volume(context, connection_info, instance)
 
         # We call plug_vifs before the compute manager calls
         # ensure_filtering_rules_for_instance, to ensure bridge is set up
@@ -7260,7 +7271,7 @@ class LibvirtDriver(driver.ComputeDriver):
                 multipath_id = vol['connection_info']['data']['multipath_id']
                 connection_info['data']['multipath_id'] = multipath_id
 
-            self._disconnect_volume(connection_info, instance)
+            self._disconnect_volume(context, connection_info, instance)
 
     def post_live_migration_at_source(self, context, instance, network_info):
         """Unplug VIFs from networks at source.
@@ -7616,7 +7627,7 @@ class LibvirtDriver(driver.ComputeDriver):
             block_device_info)
         for vol in block_device_mapping:
             connection_info = vol['connection_info']
-            self._disconnect_volume(connection_info, instance)
+            self._disconnect_volume(context, connection_info, instance)
 
         disk_info = self._get_instance_disk_info(instance, block_device_info)