From 200c743400ed3fd5240fd25d0396eb6180b3f26a Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Wed, 17 Mar 2021 17:28:05 +0000 Subject: [PATCH] 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 (cherry picked from commit 99409375a03e6f923a8b6b00a5dd653bab74caaf) --- nova/compute/api.py | 18 ++++++++- nova/tests/unit/compute/test_api.py | 57 +++++++++++++++++++++++++---- 2 files changed, 66 insertions(+), 9 deletions(-) diff --git a/nova/compute/api.py b/nova/compute/api.py index 9c47d03989e4..f4d8900b83b2 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -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) diff --git a/nova/tests/unit/compute/test_api.py b/nova/tests/unit/compute/test_api.py index 2356161f69b2..822bfc1a15be 100644 --- a/nova/tests/unit/compute/test_api.py +++ b/nova/tests/unit/compute/test_api.py @@ -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}