libvirt: Do not destroy volume secrets during _hard_reboot
Ia2007bc63ef09931ea0197cef29d6a5614ed821a unfortunately missed that resume_state_on_host_boot calls down into _hard_reboot always removing volume secrets rendering that change useless. This change seeks to address this by using the destroy_secrets kwarg introduced by I856268b371f7ba712b02189db3c927cd762a4dc3 within the _hard_reboot method of the libvirt driver to ensure secrets are not removed during a hard reboot. This resolves the original issue in bug #1905701 *and* allows admins to hard reboot a users instance when that instance has encrypted volumes attached with secrets stored in Barbican. This latter use case being something we can easily test within tempest unlike the compute reboot in bug #1905701. This change is kept small as it should ideally be backported alongside Ia2007bc63ef09931ea0197cef29d6a5614ed821a to stable/queens. Follow up changes on master will improve formatting, doc text and introduce functional tests to further validate this new behaviour of hard reboot within the libvirt driver. Closes-Bug: #1905701 Change-Id: I3d1b21ba6eb3f5eb728693197c24b4b315eef821 (cherry picked from commit26d65fc882
) (cherry picked from commit9cac2a8822
)
This commit is contained in:
parent
3dd05496ab
commit
1aca09b966
|
@ -9292,6 +9292,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()
|
||||
|
@ -10358,6 +10371,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({}))
|
||||
|
@ -16062,7 +16105,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')
|
||||
|
@ -16082,7 +16126,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):
|
||||
|
@ -16303,7 +16348,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,
|
||||
|
@ -19918,6 +19964,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,
|
||||
|
|
|
@ -1340,7 +1340,7 @@ class LibvirtDriver(driver.ComputeDriver):
|
|||
destroy_disks=True, destroy_secrets=True):
|
||||
self._destroy(instance)
|
||||
self.cleanup(context, instance, network_info, block_device_info,
|
||||
destroy_disks)
|
||||
destroy_disks, destroy_secrets=destroy_secrets)
|
||||
|
||||
def _undefine_domain(self, instance):
|
||||
try:
|
||||
|
@ -1414,11 +1414,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
|
||||
|
@ -1459,7 +1460,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:
|
||||
|
@ -1729,8 +1732,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)
|
||||
|
@ -1843,7 +1851,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
|
||||
|
@ -1855,7 +1864,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)
|
||||
# NOTE(lyarwood): Handle bug #1821696 where volume secrets have been
|
||||
|
@ -3346,7 +3359,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
|
||||
|
|
Loading…
Reference in New Issue