Merge "compute: Reject requests to commit intermediary snapshot of an inactive instance" into stable/wallaby

Author: Zuul

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}