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 commit 26d65fc882)
This commit is contained in:
Lee Yarwood
parent
f2ba67184d
commit
9cac2a8822
|
|
@ -9046,6 +9046,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()
|
||||
|
|
@ -10109,6 +10122,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({}))
|
||||
|
|
@ -15710,7 +15753,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')
|
||||
|
|
@ -15730,7 +15774,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):
|
||||
|
|
@ -15951,7 +15996,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,
|
||||
|
|
@ -19236,6 +19282,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,
|
||||
|
|
|
|||
|
|
@ -1408,7 +1408,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:
|
||||
|
|
@ -1482,11 +1482,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
|
||||
|
|
@ -1527,7 +1528,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:
|
||||
|
|
@ -1844,8 +1847,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)
|
||||
|
|
@ -1958,7 +1966,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
|
||||
|
|
@ -1970,7 +1979,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)
|
||||
|
||||
|
|
@ -3726,7 +3739,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