diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 9f5e7865db92..644e0aa61a93 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -9049,6 +9049,19 @@ class LibvirtConnTestCase(test.NoDBTestCase, uuids.volume_id) mock_encryptor.detach_volume.assert_not_called() + # assert that no attempt to remove the secret is made when + # destroy_secrets=False + drvr._host.find_secret.reset_mock() + drvr._host.delete_secret.reset_mock() + drvr._disconnect_volume( + self.context, + connection_info, + instance, + encryption=encryption, + destroy_secrets=False + ) + drvr._host.delete_secret.assert_not_called() + # assert that the encryptor is used if no secret is found drvr._host.find_secret.reset_mock() drvr._host.delete_secret.reset_mock() @@ -10112,6 +10125,36 @@ class LibvirtConnTestCase(test.NoDBTestCase, mock_find_secret.assert_called_once_with('volume', uuids.volume_id) mock_get_encryptor.assert_not_called() + @mock.patch('nova.virt.libvirt.host.Host.delete_secret') + @mock.patch('nova.virt.libvirt.host.Host.find_secret', new=mock.Mock()) + def test_detach_encryptor_skip_secret_removal(self, mock_delete_secret): + drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) + drvr._detach_encryptor( + self.context, + { + 'data': { + 'volume_id': uuids.volume_id + } + }, + None, + destroy_secrets=False + ) + # Assert that no attempt is made to delete the volume secert + mock_delete_secret.assert_not_called() + + drvr._detach_encryptor( + self.context, + { + 'data': { + 'volume_id': uuids.volume_id + } + }, + None, + destroy_secrets=True + ) + # Assert that volume secert is deleted + mock_delete_secret.assert_called_once_with('volume', uuids.volume_id) + def test_allow_native_luksv1(self): drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) self.assertFalse(drvr._allow_native_luksv1({})) @@ -15752,7 +15795,8 @@ class LibvirtConnTestCase(test.NoDBTestCase, mock_domain_destroy.assert_called_once_with() mock_teardown_container.assert_called_once_with(instance) mock_cleanup.assert_called_once_with(self.context, instance, - network_info, None, False) + network_info, None, False, + destroy_secrets=True) @mock.patch.object(libvirt_driver.LibvirtDriver, 'cleanup') @mock.patch.object(libvirt_driver.LibvirtDriver, '_teardown_container') @@ -15772,7 +15816,8 @@ class LibvirtConnTestCase(test.NoDBTestCase, mock.call(instance)]) mock_teardown_container.assert_called_once_with(instance) mock_cleanup.assert_called_once_with(self.context, instance, - network_info, None, False) + network_info, None, False, + destroy_secrets=True) @mock.patch.object(host.Host, 'get_guest') def test_reboot_different_ids(self, mock_get): @@ -15993,7 +16038,8 @@ class LibvirtConnTestCase(test.NoDBTestCase, mock_get_mdev.assert_called_once_with(instance) mock_destroy.assert_called_once_with(self.context, instance, network_info, destroy_disks=False, - block_device_info=block_device_info) + block_device_info=block_device_info, + destroy_secrets=False) mock_get_guest_xml.assert_called_once_with(self.context, instance, network_info, mock.ANY, mock.ANY, @@ -19278,6 +19324,59 @@ class LibvirtConnTestCase(test.NoDBTestCase, self.assertTrue(instance.cleaned) save.assert_called_once_with() + @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume') + @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._undefine_domain', + new=mock.Mock()) + @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_vpmems', + new=mock.Mock(return_value=None)) + def test_cleanup_destroy_secrets(self, mock_disconnect_volume): + block_device_info = { + 'block_device_mapping': [ + { + 'connection_info': mock.sentinel.connection_info + } + ] + } + instance = objects.Instance(self.context, **self.test_instance) + drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI()) + + # Pass destroy_vifs=False and destroy_disks=False as we only care about + # asserting the behaviour of destroy_secrets in this test. + drvr.cleanup( + self.context, + instance, + network_info={}, + block_device_info=block_device_info, + destroy_vifs=False, + destroy_disks=False, + destroy_secrets=False + ) + drvr.cleanup( + self.context, + instance, + network_info={}, + block_device_info=block_device_info, + destroy_vifs=False, + destroy_disks=False, + ) + + # Assert that disconnect_volume is called with destroy_secrets=False + # and destroy_secrets=True by default + mock_disconnect_volume.assert_has_calls([ + mock.call( + self.context, + mock.sentinel.connection_info, + instance, + destroy_secrets=False + ), + mock.call( + self.context, + mock.sentinel.connection_info, + instance, + destroy_secrets=True + ) + ]) + @mock.patch.object(libvirt_driver.LibvirtDriver, '_get_volume_encryption') @mock.patch.object(libvirt_driver.LibvirtDriver, '_allow_native_luksv1') def test_swap_volume_native_luks_blocked(self, mock_allow_native_luksv1, diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index e7ff852350bb..7efc5512cd27 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -1411,7 +1411,7 @@ class LibvirtDriver(driver.ComputeDriver): # unblock the waiting threads and clean up. self._device_event_handler.cleanup_waiters(instance.uuid) self.cleanup(context, instance, network_info, block_device_info, - destroy_disks) + destroy_disks, destroy_secrets=destroy_secrets) def _undefine_domain(self, instance): try: @@ -1485,11 +1485,12 @@ class LibvirtDriver(driver.ComputeDriver): block_device_info=block_device_info, destroy_vifs=destroy_vifs, cleanup_instance_dir=cleanup_instance_dir, - cleanup_instance_disks=cleanup_instance_disks) + cleanup_instance_disks=cleanup_instance_disks, + destroy_secrets=destroy_secrets) def _cleanup(self, context, instance, network_info, block_device_info=None, destroy_vifs=True, cleanup_instance_dir=False, - cleanup_instance_disks=False): + cleanup_instance_disks=False, destroy_secrets=True): """Cleanup the domain and any attached resources from the host. This method cleans up any pmem devices, unplugs VIFs, disconnects @@ -1530,7 +1531,9 @@ class LibvirtDriver(driver.ComputeDriver): continue try: - self._disconnect_volume(context, connection_info, instance) + self._disconnect_volume( + context, connection_info, instance, + destroy_secrets=destroy_secrets) except Exception as exc: with excutils.save_and_reraise_exception() as ctxt: if cleanup_instance_disks: @@ -1847,8 +1850,13 @@ class LibvirtDriver(driver.ComputeDriver): return (False if connection_count > 1 else True) def _disconnect_volume(self, context, connection_info, instance, - encryption=None): - self._detach_encryptor(context, connection_info, encryption=encryption) + encryption=None, destroy_secrets=True): + self._detach_encryptor( + context, + connection_info, + encryption=encryption, + destroy_secrets=destroy_secrets + ) vol_driver = self._get_volume_driver(connection_info) volume_id = driver_block_device.get_volume_id(connection_info) multiattach = connection_info.get('multiattach', False) @@ -1961,7 +1969,8 @@ class LibvirtDriver(driver.ComputeDriver): encryption) encryptor.attach_volume(context, **encryption) - def _detach_encryptor(self, context, connection_info, encryption): + def _detach_encryptor(self, context, connection_info, encryption, + destroy_secrets=True): """Detach the frontend encryptor if one is required by the volume. The request context is only used when an encryption metadata dict is @@ -1973,7 +1982,11 @@ class LibvirtDriver(driver.ComputeDriver): """ volume_id = driver_block_device.get_volume_id(connection_info) if volume_id and self._host.find_secret('volume', volume_id): + if not destroy_secrets: + LOG.debug("Skipping volume secret destruction") + return return self._host.delete_secret('volume', volume_id) + if encryption is None: encryption = self._get_volume_encryption(context, connection_info) @@ -3729,7 +3742,8 @@ class LibvirtDriver(driver.ComputeDriver): # we can here without losing data. This allows us to re-initialise from # scratch, and hopefully fix, most aspects of a non-functioning guest. self.destroy(context, instance, network_info, destroy_disks=False, - block_device_info=block_device_info) + block_device_info=block_device_info, + destroy_secrets=False) # Convert the system metadata to image metadata # NOTE(mdbooth): This is a workaround for stateless Nova compute