openstack
/
nova
Code Issues Proposed changes

compute: Reject requests to commit intermediary snapshot of an inactive instance 

Introduced by I76eb2e4da027a13525314bd58264f482374d270d the
os-assisted-volume-snapshots API is only implemented by the libvirt virt
driver and should only be called by c-vol as part of an orchestrated
remotefs based volume snapshot creation or deletion workflow.

While not documented clearly in the current api-ref there are code
comments within the compute API suggesting that this API can be called
against a volume attached to an instance that is in *any* vm_state.

This however is not correct when deleting and in turn committing an
intermediary volume snapshot of an instance that is not running given
the current implementation within the libvirt driver. With a request to
virDomainBlockCommit being made that fails if the instance and
underlying domain is not running.

Adding support for an offline commit isn't trivial and would be
considered a new feature and not something we could backport on the
stable branches. As such this change seeks to ensure requests to commit
an intermediary volume snapshot from an inactive instance are rejected
quickly and clearly by the compute API to the caller before we cast to
the compute.

Closes-Bug: #1919487
Change-Id: I212a2db8d71702d330b146dc6f871b402a309e74
This commit is contained in:
Lee Yarwood 2021-03-17 17:28:05 +00:00
parent a086a88cd1
commit 99409375a0
2 changed files with 66 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
nova/compute/api.py
View File

@ -5483,11 +5483,25 @@ class API(base.Base):
bdm = self._get_bdm_by_volume_id(
context, volume_id, expected_attrs=['instance'])
# We allow deleting the snapshot in any vm_state as long as there is
# no task being performed on the instance and it has a host.
@check_instance_host()
@check_instance_state(vm_state=None)
def do_volume_snapshot_delete(self, context, instance):
# FIXME(lyarwood): Avoid bug #1919487 by rejecting the request
# to delete an intermediary volume snapshot offline as this isn't
# currently implemented within the libvirt driver and will fail.
# This should be fixed in a future release but as it is essentially
# a new feature wouldn't be something we could backport. As such
# reject the request here so n-api can respond correctly to c-vol.
if (delete_info.get('merge_target_file') is not None and
instance.vm_state != vm_states.ACTIVE
):
raise exception.InstanceInvalidState(
attr='vm_state',
instance_uuid=instance.uuid,
state=instance.vm_state,
method='volume_snapshot_delete'
)
self.compute_rpcapi.volume_snapshot_delete(context, instance,
volume_id, snapshot_id, delete_info)

57
nova/tests/unit/compute/test_api.py
View File

@ -3601,23 +3601,29 @@ class _ComputeAPIUnitTestMixIn(object):
'boot_index': -1})
fake_bdm['instance'] = fake_instance.fake_db_instance(
launched_at=timeutils.utcnow(),
vm_state=vm_states.STOPPED)
vm_state=vm_states.ACTIVE)
fake_bdm['instance_uuid'] = fake_bdm['instance']['uuid']
fake_bdm = objects.BlockDeviceMapping._from_db_object(
self.context, objects.BlockDeviceMapping(),
fake_bdm, expected_attrs=['instance'])
mock_get_bdm.return_value = fake_bdm
delete_info = {
'merge_target_file': 'foo.qcow2',
}
with mock.patch.object(self.compute_api.compute_rpcapi,
'volume_snapshot_delete') as mock_snapshot:
self.compute_api.volume_snapshot_delete(self.context, volume_id,
snapshot_id, {})
with mock.patch.object(
self.compute_api.compute_rpcapi, 'volume_snapshot_delete'
) as mock_snapshot:
self.compute_api.volume_snapshot_delete(
self.context, volume_id, snapshot_id, delete_info
)
mock_get_bdm.assert_called_once_with(self.context, volume_id,
expected_attrs=['instance'])
mock_snapshot.assert_called_once_with(
self.context, fake_bdm['instance'], volume_id, snapshot_id, {})
self.context, fake_bdm['instance'], volume_id, snapshot_id,
delete_info
)
@mock.patch.object(
objects.BlockDeviceMapping, 'get_by_volume',
@ -3651,6 +3657,43 @@ class _ComputeAPIUnitTestMixIn(object):
self.context, mock.sentinel.volume_id,
mock.sentinel.snapshot_id, mock.sentinel.delete_info)
@mock.patch.object(compute_api.API, '_get_bdm_by_volume_id')
def test_volume_snapshot_delete_intermediary_commit(self, mock_get_bdm):
fake_bdm = fake_block_device.FakeDbBlockDeviceDict({
'id': 123,
'device_name': '/dev/sda2',
'source_type': 'volume',
'destination_type': 'volume',
'connection_info': "{'fake': 'connection_info'}",
'volume_id': uuids.volume_id,
'boot_index': -1
})
fake_bdm['instance'] = fake_instance.fake_db_instance(
launched_at=timeutils.utcnow(),
vm_state=vm_states.STOPPED)
fake_bdm['instance_uuid'] = fake_bdm['instance']['uuid']
fake_bdm = objects.BlockDeviceMapping._from_db_object(
self.context, objects.BlockDeviceMapping(),
fake_bdm, expected_attrs=['instance'])
mock_get_bdm.return_value = fake_bdm
# c-vol can provide delete_info with merge_target_file pointing to
# an intermediary snapshot to commit into it's base. This is only
# supported while the instance is running at present.
delete_info = {
'merge_target_file': 'snap.img'
}
# Assert that the request is rejected as offline commit isn't supported
self.assertRaises(
exception.InstanceInvalidState,
self.compute_api.volume_snapshot_delete,
self.context,
uuids.volume_id,
uuids.snapshot_id,
delete_info
)
def _create_instance_with_disabled_disk_config(self, object=False):
sys_meta = {"image_auto_disk_config": "Disabled"}
params = {"system_metadata": sys_meta}