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 9940937)

Author: lyarwood

nova/compute/api.py

@@ -5483,11 +5483,25 @@ def volume_snapshot_delete(self, context, volume_id, snapshot_id,
         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)
 

nova/tests/unit/compute/test_api.py

@@ -3601,23 +3601,29 @@ def test_volume_snapshot_delete(self, mock_get_bdm):
                     '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 @@ def test_volume_snapshot_delete_shelved_offloaded(self, bdm_get_by_volume):
                           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}