Merge branch 'two-stage-clone'

Makefile

@@ -35,7 +35,6 @@ ADMIN_API_METHODS_SIMPLE = \
 	admin.property.List \
 	admin.property.Reset \
 	admin.property.Set \
-	admin.vm.Clone \
 	admin.vm.Create.AppVM \
 	admin.vm.Create.DispVM \
 	admin.vm.Create.StandaloneVM \

qubes/api/admin.py

@@ -293,22 +293,57 @@ class QubesAdminAPI(qubes.api.AbstractQubesAPI):
         self.dest.storage.get_pool(volume).revert(revision)
         self.app.save()
 
-    @qubes.api.method('admin.vm.volume.Clone')
+    @qubes.api.method('admin.vm.volume.CloneFrom', no_payload=True)
     @asyncio.coroutine
-    def vm_volume_clone(self, untrusted_payload):
+    def vm_volume_clone_from(self):
         assert self.arg in self.dest.volumes.keys()
-        untrusted_target = untrusted_payload.decode('ascii').strip()
-        del untrusted_payload
-        qubes.vm.validate_name(None, None, untrusted_target)
-        target_vm = self.app.domains[untrusted_target]
-        del untrusted_target
-        assert self.arg in target_vm.volumes.keys()
 
         volume = self.dest.volumes[self.arg]
 
-        self.fire_event_for_permission(target_vm=target_vm, volume=volume)
+        self.fire_event_for_permission(volume=volume)
 
-        yield from target_vm.storage.clone_volume(self.dest, self.arg)
+        token = qubes.utils.random_string(32)
+        # save token on self.app, as self is not persistent
+        if not hasattr(self.app, 'api_admin_pending_clone'):
+            self.app.api_admin_pending_clone = {}
+        # don't handle collisions any better - if someone is so much out of
+        # luck, can try again anyway
+        assert token not in self.app.api_admin_pending_clone
+
+        self.app.api_admin_pending_clone[token] = volume
+        return token
+
+    @qubes.api.method('admin.vm.volume.CloneTo')
+    @asyncio.coroutine
+    def vm_volume_clone_to(self, untrusted_payload):
+        assert self.arg in self.dest.volumes.keys()
+        untrusted_token = untrusted_payload.decode('ascii').strip()
+        del untrusted_payload
+        assert untrusted_token in getattr(self.app,
+            'api_admin_pending_clone', {})
+        token = untrusted_token
+        del untrusted_token
+
+        src_volume = self.app.api_admin_pending_clone[token]
+        del self.app.api_admin_pending_clone[token]
+
+        # make sure the volume still exists, but invalidate token anyway
+        assert str(src_volume.pool) in self.app.pools
+        assert src_volume in self.app.pools[str(src_volume.pool)].volumes
+
+        dst_volume = self.dest.volumes[self.arg]
+
+        self.fire_event_for_permission(src_volume=src_volume,
+            dst_volume=dst_volume)
+
+        op_retval = dst_volume.import_volume(src_volume)
+
+        # clone/import functions may be either synchronous or asynchronous
+        # in the later case, we need to wait for them to finish
+        if asyncio.iscoroutine(op_retval):
+            op_retval = yield from op_retval
+
+        self.dest.volumes[self.arg] = op_retval
         self.app.save()
 
     @qubes.api.method('admin.vm.volume.Resize')
@@ -829,40 +864,6 @@ class QubesAdminAPI(qubes.api.AbstractQubesAPI):
 
         self.app.save()
 
-    @qubes.api.method('admin.vm.Clone')
-    @asyncio.coroutine
-    def vm_clone(self, untrusted_payload):
-        assert not self.arg
-
-        assert untrusted_payload.startswith(b'name=')
-        untrusted_name = untrusted_payload[5:].decode('ascii')
-        qubes.vm.validate_name(None, None, untrusted_name)
-        new_name = untrusted_name
-
-        del untrusted_payload
-
-        if new_name in self.app.domains:
-            raise qubes.exc.QubesValueError('Already exists')
-
-        self.fire_event_for_permission(new_name=new_name)
-
-        src_vm = self.dest
-
-        dst_vm = self.app.add_new_vm(src_vm.__class__, name=new_name)
-        try:
-            dst_vm.clone_properties(src_vm)
-            dst_vm.tags.update(src_vm.tags)
-            dst_vm.features.update(src_vm.features)
-            dst_vm.firewall.clone(src_vm.firewall)
-            for devclass in src_vm.devices:
-                for device_assignment in src_vm.devices[devclass].assignments():
-                    dst_vm.devices[devclass].attach(device_assignment.clone())
-            yield from dst_vm.clone_disk_files(src_vm)
-        except:
-            del self.app.domains[dst_vm]
-            raise
-        self.app.save()
-
     @qubes.api.method('admin.vm.device.{endpoint}.Available', endpoints=(ep.name
             for ep in pkg_resources.iter_entry_points('qubes.devices')),
             no_payload=True)

qubes/storage/__init__.py

@@ -146,6 +146,12 @@ class Volume(object):
         '''
         raise self._not_implemented("create")
 
+    def remove(self):
+        ''' Remove volume.
+
+        This can be implemented as a coroutine.'''
+        raise self._not_implemented("remove")
+
     def commit(self):
         ''' Write the snapshot to disk
 
@@ -171,7 +177,11 @@ class Volume(object):
 
     def import_volume(self, src_volume):
         ''' Imports data from a different volume (possibly in a different
-        pool '''
+        pool.
+
+        The needs to be create()d first.
+
+        This can be implemented as a coroutine. '''
         # pylint: disable=unused-argument
         raise self._not_implemented("import_volume")
 
@@ -442,16 +452,9 @@ class Storage(object):
         dst_pool = self.vm.app.get_pool(config['pool'])
         dst = dst_pool.init_volume(self.vm, config)
         src_volume = src_vm.volumes[name]
-        src_pool = src_volume.pool
-        if dst_pool == src_pool:
-            msg = "Cloning volume {!s} from vm {!s}"
-            self.vm.log.info(msg.format(src_volume.name, src_vm.name))
-            clone_op_ret = dst_pool.clone(src_volume, dst)
-        else:
-            msg = "Importing volume {!s} from vm {!s}"
-            self.vm.log.info(msg.format(src_volume.name, src_vm.name))
-            clone_op_ret = dst_pool.import_volume(
-                dst_pool, dst, src_pool, src_volume)
+        msg = "Importing volume {!s} from vm {!s}"
+        self.vm.log.info(msg.format(src_volume.name, src_vm.name))
+        clone_op_ret = dst.import_volume(src_volume)
 
         # clone/import functions may be either synchronous or asynchronous
         # in the later case, we need to wait for them to finish
@@ -520,7 +523,7 @@ class Storage(object):
         for name, volume in self.vm.volumes.items():
             self.log.info('Removing volume %s: %s' % (name, volume.vid))
             try:
-                ret = volume.pool.remove(volume)
+                ret = volume.remove()
                 if asyncio.iscoroutine(ret):
                     futures.append(ret)
             except (IOError, OSError) as e:
@@ -614,6 +617,58 @@ class Storage(object):
         return self.vm.volumes[volume].import_data_end(success=success)
 
 
+class VolumesCollection(object):
+    '''Convenient collection wrapper for pool.get_volume and
+    pool.list_volumes
+    '''
+    def __init__(self, pool):
+        self._pool = pool
+
+    def __getitem__(self, item):
+        ''' Get a single volume with given Volume ID.
+
+        You can also a Volume instance to get the same Volume or KeyError if
+        Volume no longer exists.
+
+        :param item: a Volume ID (str) or a Volume instance
+        '''
+        if isinstance(item, Volume):
+            if item.pool == self._pool:
+                return self[item.vid]
+            else:
+                raise KeyError(item)
+        try:
+            return self._pool.get_volume(item)
+        except NotImplementedError:
+            for vol in self:
+                if vol.vid == item:
+                    return vol
+            # if list_volumes is not implemented too, it will raise
+            # NotImplementedError again earlier
+            raise KeyError(item)
+
+    def __iter__(self):
+        ''' Get iterator over pool's volumes '''
+        return iter(self._pool.list_volumes())
+
+    def __contains__(self, item):
+        ''' Check if given volume (either Volume ID or Volume instance) is
+        present in the pool
+        '''
+        try:
+            return self[item] is not None
+        except KeyError:
+            return False
+
+    def keys(self):
+        ''' Return list of volume IDs '''
+        return [vol.vid for vol in self]
+
+    def values(self):
+        ''' Return list of Volumes'''
+        return [vol for vol in self]
+
+
 class Pool(object):
     ''' A Pool is used to manage different kind of volumes (File
         based/LVM/Btrfs/...).
@@ -626,6 +681,7 @@ class Pool(object):
 
     def __init__(self, name, revisions_to_keep=1, **kwargs):
         super(Pool, self).__init__(**kwargs)
+        self._volumes_collection = VolumesCollection(self)
         self.name = name
         self.revisions_to_keep = revisions_to_keep
         kwargs['name'] = self.name
@@ -666,12 +722,6 @@ class Pool(object):
         '''
         raise self._not_implemented("init_volume")
 
-    def remove(self, volume):
-        ''' Remove volume.
-
-        This can be implemented as a coroutine.'''
-        raise self._not_implemented("remove")
-
     def rename(self, volume, old_name, new_name):
         ''' Called when the domain changes its name '''
         raise self._not_implemented("rename")
@@ -684,8 +734,19 @@ class Pool(object):
 
     @property
     def volumes(self):
+        ''' Return a collection of volumes managed by this pool '''
+        return self._volumes_collection
+
+    def list_volumes(self):
         ''' Return a list of volumes managed by this pool '''
-        raise self._not_implemented("volumes")
+        raise self._not_implemented("list_volumes")
+
+    def get_volume(self, vid):
+        ''' Return a volume with *vid* from this pool
+
+        :raise KeyError: if no volume is found
+        '''
+        raise self._not_implemented("get_volume")
 
     def _not_implemented(self, method_name):
         ''' Helper for emitting helpful `NotImplementedError` exceptions '''
@@ -740,8 +801,7 @@ class VmCreationManager(object):
         if type is not None and value is not None and tb is not None:
             for volume in self.vm.volumes.values():
                 try:
-                    pool = volume.pool
-                    pool.remove(volume)
+                    volume.remove()
                 except Exception:  # pylint: disable=broad-except
                     pass
             os.rmdir(self.vm.dir_path)

qubes/storage/file.py

@@ -79,16 +79,6 @@ class FilePool(qubes.storage.Pool):
         self._volumes += [volume]
         return volume
 
-    def remove(self, volume):
-        if not volume.internal:
-            return  # do not remove random attached file volumes
-        elif volume._is_snapshot:
-            return  # no need to remove, because it's just a snapshot
-        else:
-            _remove_if_exists(volume.path)
-            if volume._is_origin:
-                _remove_if_exists(volume.path_cow)
-
     def rename(self, volume, old_name, new_name):
         assert issubclass(volume.__class__, FileVolume)
         subdir, _, volume_path = volume.vid.split('/', 2)
@@ -152,8 +142,7 @@ class FilePool(qubes.storage.Pool):
 
         return os.path.join(self.dir_path, self._vid_prefix(vm))
 
-    @property
-    def volumes(self):
+    def list_volumes(self):
         return self._volumes
 
 
@@ -202,6 +191,16 @@ class FileVolume(qubes.storage.Volume):
             else:
                 create_sparse_file(self.path, self.size)
 
+    def remove(self):
+        if not self.internal:
+            return  # do not remove random attached file volumes
+        elif self._is_snapshot:
+            return  # no need to remove, because it's just a snapshot
+        else:
+            _remove_if_exists(self.path)
+            if self._is_origin:
+                _remove_if_exists(self.path_cow)
+
     def is_dirty(self):
         return False  # TODO: How to implement this?
 
@@ -262,6 +261,7 @@ class FileVolume(qubes.storage.Volume):
         msg = msg.format(src_volume, self)
         assert not src_volume.snap_on_start, msg
         if self.save_on_stop:
+            _remove_if_exists(self.path)
             copy_file(src_volume.export(), self.path)
         return self
 

qubes/storage/kernels.py

@@ -93,6 +93,9 @@ class LinuxModules(Volume):
     def create(self):
         return self
 
+    def remove(self):
+        pass
+
     def commit(self):
         return self
 
@@ -154,9 +157,6 @@ class LinuxKernel(Pool):
     def import_volume(self, dst_pool, dst_volume, src_pool, src_volume):
         pass
 
-    def remove(self, volume):
-        pass
-
     def rename(self, volume, old_name, new_name):
         return volume
 

qubes/storage/lvm.py

@@ -88,18 +88,6 @@ class ThinPool(qubes.storage.Pool):
         volume_config['pool'] = self
         return ThinVolume(**volume_config)
 
-    def remove(self, volume):
-        assert volume.vid
-        if volume.is_dirty():
-            cmd = ['remove', volume._vid_snap]
-            qubes_lvm(cmd, self.log)
-
-        if not os.path.exists(volume.path):
-            return
-        cmd = ['remove', volume.vid]
-        qubes_lvm(cmd, self.log)
-        reset_cache()
-
     def rename(self, volume, old_name, new_name):
         ''' Called when the domain changes its name '''
         new_vid = "{!s}/vm-{!s}-{!s}".format(self.volume_group, new_name,
@@ -120,8 +108,7 @@ class ThinPool(qubes.storage.Pool):
     def setup(self):
         pass  # TODO Should we create a non existing pool?
 
-    @property
-    def volumes(self):
+    def list_volumes(self):
         ''' Return a list of volumes managed by this pool '''
         volumes = []
         for vid, vol_info in size_cache.items():
@@ -133,7 +120,7 @@ class ThinPool(qubes.storage.Pool):
                 # implementation detail volume
                 continue
             config = {
-                'pool': self.name,
+                'pool': self,
                 'vid': vid,
                 'name': vid,
                 'volume_group': self.volume_group,
@@ -297,6 +284,18 @@ class ThinVolume(qubes.storage.Volume):
             reset_cache()
         return self
 
+    def remove(self):
+        assert self.vid
+        if self.is_dirty():
+            cmd = ['remove', self._vid_snap]
+            qubes_lvm(cmd, self.log)
+
+        if not os.path.exists(self.path):
+            return
+        cmd = ['remove', self.vid]
+        qubes_lvm(cmd, self.log)
+        reset_cache()
+
     def export(self):
         ''' Returns an object that can be `open()`. '''
         devpath = '/dev/' + self.vid
@@ -306,18 +305,17 @@ class ThinVolume(qubes.storage.Volume):
         if not src_volume.save_on_stop:
             return self
 
-        src_path = src_volume.export()
-
         # HACK: neat trick to speed up testing if you have same physical thin
         # pool assigned to two qubes-pools i.e: qubes_dom0 and test-lvm
         # pylint: disable=line-too-long
         if isinstance(src_volume.pool, ThinPool) and \
                 src_volume.pool.thin_pool == self.pool.thin_pool:  # NOQA
+            cmd = ['remove', self.vid]
+            qubes_lvm(cmd, self.log)
             cmd = ['clone', str(src_volume), str(self)]
             qubes_lvm(cmd, self.log)
         else:
-            self.create()
-
+            src_path = src_volume.export()
             cmd = ['sudo', 'dd', 'if=' + src_path, 'of=/dev/' + self.vid,
                 'conv=sparse']
             subprocess.check_call(cmd)

qubes/tests/api_admin.py

@@ -21,6 +21,7 @@
 ''' Tests for management calls endpoints '''
 
 import asyncio
+import operator
 import os
 import shutil
 import unittest.mock
@@ -31,6 +32,7 @@ import qubes
 import qubes.devices
 import qubes.api.admin
 import qubes.tests
+import qubes.storage
 
 # properties defined in API
 volume_properties = [
@@ -1236,72 +1238,6 @@ class TC_00_VMs(AdminAPITestCase):
         self.assertNotIn('test-vm2', self.app.domains)
         self.assertFalse(self.app.save.called)
 
-    @unittest.mock.patch('qubes.storage.Storage.clone')
-    @unittest.mock.patch('qubes.storage.Storage.verify')
-    def test_350_vm_clone(self, mock_verify, mock_clone):
-        mock_clone.side_effect = self.dummy_coro
-        mock_verify.side_effect = self.dummy_coro
-        self.call_mgmt_func(b'admin.vm.Clone',
-            b'test-vm1', b'', b'name=test-vm2')
-
-        self.assertIn('test-vm2', self.app.domains)
-        vm = self.app.domains['test-vm2']
-        self.assertEqual(vm.label, self.app.get_label('red'))
-        self.assertEqual(vm.template, self.app.domains['test-template'])
-        self.assertEqual(vm.tags, self.vm.tags)
-        self.assertEqual(vm.features, self.vm.features)
-        self.assertEqual(vm.firewall, self.vm.firewall)
-        self.assertEqual(mock_clone.mock_calls,
-            [unittest.mock.call(self.app.domains['test-vm2']).clone(
-                self.app.domains['test-vm1'])])
-        self.assertTrue(os.path.exists(os.path.join(
-            self.test_base_dir, 'appvms', 'test-vm2')))
-
-        self.assertTrue(self.app.save.called)
-
-    @unittest.mock.patch('qubes.storage.Storage.clone')
-    @unittest.mock.patch('qubes.storage.Storage.verify')
-    def test_351_vm_clone_extra_params(self, mock_verify, mock_clone):
-        mock_clone.side_effect = self.dummy_coro
-        mock_verify.side_effect = self.dummy_coro
-        with self.assertRaises(qubes.exc.QubesException):
-            self.call_mgmt_func(b'admin.vm.Clone',
-                b'test-vm1', b'', b'name=test-vm2 label=red')
-
-        self.assertNotIn('test-vm2', self.app.domains)
-        self.assertEqual(mock_clone.mock_calls, [])
-        self.assertFalse(os.path.exists(os.path.join(
-            self.test_base_dir, 'appvms', 'test-vm2')))
-
-        self.assertFalse(self.app.save.called)
-
-    @unittest.mock.patch('qubes.storage.Storage.clone')
-    @unittest.mock.patch('qubes.storage.Storage.verify')
-    def test_352_vm_clone_duplicate_name(self, mock_verify, mock_clone):
-        mock_clone.side_effect = self.dummy_coro
-        mock_verify.side_effect = self.dummy_coro
-        with self.assertRaises(qubes.exc.QubesException):
-            self.call_mgmt_func(b'admin.vm.Clone',
-                b'test-vm1', b'', b'name=test-vm1')
-
-        self.assertFalse(self.app.save.called)
-
-    @unittest.mock.patch('qubes.storage.Storage.clone')
-    @unittest.mock.patch('qubes.storage.Storage.verify')
-    def test_353_vm_clone_invalid_name(self, mock_verify, mock_clone):
-        mock_clone.side_effect = self.dummy_coro
-        mock_verify.side_effect = self.dummy_coro
-        with self.assertRaises(qubes.exc.QubesException):
-            self.call_mgmt_func(b'admin.vm.Clone',
-                b'test-vm1', b'', b'name=test-vm2/..')
-
-        self.assertNotIn('test-vm2/..', self.app.domains)
-        self.assertEqual(mock_clone.mock_calls, [])
-        self.assertFalse(os.path.exists(os.path.join(
-            self.test_base_dir, 'appvms', 'test-vm2/..')))
-
-        self.assertFalse(self.app.save.called)
-
 
     def test_400_property_list(self):
         # actual function tested for admin.vm.property.* already
@@ -1598,93 +1534,100 @@ class TC_00_VMs(AdminAPITestCase):
                 self.call_mgmt_func(b'admin.vm.volume.Import', b'test-vm1',
                     b'private')
 
+    def setup_for_clone(self):
+        self.pool = unittest.mock.MagicMock()
+        self.app.pools['test'] = self.pool
+        self.vm2 = self.app.add_new_vm('AppVM', label='red',
+            name='test-vm2',
+            template='test-template', kernel='')
+        self.pool.configure_mock(**{
+            'volumes': qubes.storage.VolumesCollection(self.pool),
+            'init_volume.return_value.pool': self.pool,
+            '__str__.return_value': 'test',
+            'get_volume.side_effect': (lambda vid:
+                self.vm.volumes['private']
+                    if vid is self.vm.volumes['private'].vid
+                    else self.vm2.volumes['private']
+            ),
+        })
+        self.loop.run_until_complete(
+            self.vm.create_on_disk(pool='test'))
+        self.loop.run_until_complete(
+            self.vm2.create_on_disk(pool='test'))
+
+        # the call replaces self.vm.volumes[...] with result of import
+        # operation - make sure it stays as the same object
+        self.vm.volumes['private'].import_volume.return_value = \
+            self.vm.volumes['private']
+        self.vm2.volumes['private'].import_volume.return_value = \
+            self.vm2.volumes['private']
+
     def test_520_vm_volume_clone(self):
-        self.vm2 = self.app.add_new_vm('AppVM', label='red', name='test-vm2',
-            template='test-template')
-        self.vm.volumes = unittest.mock.MagicMock()
-        self.vm2.volumes = unittest.mock.MagicMock()
-        volumes_conf = {
-            'keys.return_value': ['root', 'private', 'volatile', 'kernel'],
-        }
-        self.vm.volumes.configure_mock(**volumes_conf)
-        self.vm2.volumes.configure_mock(**volumes_conf)
-        self.vm2.storage = unittest.mock.Mock()
-        func_mock = unittest.mock.Mock()
-
-        @asyncio.coroutine
-        def coroutine_mock(*args, **kwargs):
-            return func_mock(*args, **kwargs)
-        self.vm2.storage.clone_volume = coroutine_mock
-
-        self.call_mgmt_func(b'admin.vm.volume.Clone',
-                b'test-vm1', b'private', b'test-vm2')
-        self.assertEqual(self.vm.volumes.mock_calls,
-            [('keys', (), {}),
-             ('__getitem__', ('private', ), {}),
-             ('__getitem__().__hash__', (), {}),
-            ])
-        self.assertEqual(self.vm2.volumes.mock_calls,
-            [unittest.mock.call.keys()])
-        self.assertEqual(self.vm2.storage.mock_calls, [])
-        self.assertEqual(func_mock.mock_calls, [
-            unittest.mock.call(self.vm, 'private')
-        ])
+        self.setup_for_clone()
+        token = self.call_mgmt_func(b'admin.vm.volume.CloneFrom',
+                b'test-vm1', b'private', b'')
+        # token
+        self.assertEqual(len(token), 32)
+        self.assertFalse(self.app.save.called)
+        value = self.call_mgmt_func(b'admin.vm.volume.CloneTo',
+                b'test-vm2', b'private', token.encode())
+        self.assertIsNone(value)
+        self.vm2.volumes['private'].import_volume.assert_called_once_with(
+            self.vm.volumes['private']
+        )
+        self.vm2.volumes['private'].import_volume.assert_called_once_with(
+            self.vm2.volumes['private']
+        )
         self.app.save.assert_called_once_with()
 
     def test_521_vm_volume_clone_invalid_volume(self):
-        self.vm2 = self.app.add_new_vm('AppVM', label='red', name='test-vm2',
-            template='test-template')
-        self.vm.volumes = unittest.mock.MagicMock()
-        self.vm2.volumes = unittest.mock.MagicMock()
-        volumes_conf = {
-            'keys.return_value': ['root', 'private', 'volatile', 'kernel'],
-        }
-        self.vm.volumes.configure_mock(**volumes_conf)
-        self.vm2.volumes.configure_mock(**volumes_conf)
-        self.vm2.storage = unittest.mock.Mock()
-        func_mock = unittest.mock.Mock()
-
-        @asyncio.coroutine
-        def coroutine_mock(*args, **kwargs):
-            return func_mock(*args, **kwargs)
-        self.vm2.storage.clone_volume = coroutine_mock
+        self.setup_for_clone()
 
         with self.assertRaises(AssertionError):
-            self.call_mgmt_func(b'admin.vm.volume.Clone',
-                    b'test-vm1', b'private123', b'test-vm2')
-        self.assertEqual(self.vm.volumes.mock_calls,
-            [('keys', (), {})])
-        self.assertEqual(self.vm2.volumes.mock_calls, [])
-        self.assertEqual(self.vm2.storage.mock_calls, [])
-        self.assertEqual(func_mock.mock_calls, [])
+            self.call_mgmt_func(b'admin.vm.volume.CloneFrom',
+                    b'test-vm1', b'private123', None)
+        self.assertNotIn('init_volume().import_volume',
+            map(operator.itemgetter(0), self.pool.mock_calls))
         self.assertFalse(self.app.save.called)
 
-    def test_522_vm_volume_clone_invalid_vm(self):
-        self.vm2 = self.app.add_new_vm('AppVM', label='red', name='test-vm2',
-            template='test-template')
-        self.vm.volumes = unittest.mock.MagicMock()
-        self.vm2.volumes = unittest.mock.MagicMock()
-        volumes_conf = {
-            'keys.return_value': ['root', 'private', 'volatile', 'kernel'],
-        }
-        self.vm.volumes.configure_mock(**volumes_conf)
-        self.vm2.volumes.configure_mock(**volumes_conf)
-        self.vm2.storage = unittest.mock.Mock()
-        func_mock = unittest.mock.Mock()
+    def test_522_vm_volume_clone_invalid_volume2(self):
+        self.setup_for_clone()
 
-        @asyncio.coroutine
-        def coroutine_mock(*args, **kwargs):
-            return func_mock(*args, **kwargs)
-        self.vm2.storage.clone_volume = coroutine_mock
+        token = self.call_mgmt_func(b'admin.vm.volume.CloneFrom',
+                b'test-vm1', b'private', b'')
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'admin.vm.volume.CloneTo',
+                    b'test-vm1', b'private123', token.encode())
+        self.assertNotIn('init_volume().import_volume',
+            map(operator.itemgetter(0), self.pool.mock_calls))
+        self.assertFalse(self.app.save.called)
+
+    def test_523_vm_volume_clone_removed_volume(self):
+        self.setup_for_clone()
+
+        token = self.call_mgmt_func(b'admin.vm.volume.CloneFrom',
+                b'test-vm1', b'private', b'')
+        def get_volume(vid):
+            if vid == self.vm.volumes['private']:
+                raise KeyError(vid)
+            else:
+                return unittest.mock.DEFAULT
+        self.pool.get_volume.side_effect = get_volume
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'admin.vm.volume.CloneTo',
+                    b'test-vm1', b'private', token.encode())
+        self.assertNotIn('init_volume().import_volume',
+            map(operator.itemgetter(0), self.pool.mock_calls))
+        self.assertFalse(self.app.save.called)
+
+    def test_524_vm_volume_clone_invlid_token(self):
+        self.setup_for_clone()
 
         with self.assertRaises(AssertionError):
-            self.call_mgmt_func(b'admin.vm.volume.Clone',
-                    b'test-vm1', b'private123', b'no-such-vm')
-        self.assertEqual(self.vm.volumes.mock_calls,
-            [('keys', (), {})])
-        self.assertEqual(self.vm2.volumes.mock_calls, [])
-        self.assertEqual(self.vm2.storage.mock_calls, [])
-        self.assertEqual(func_mock.mock_calls, [])
+            self.call_mgmt_func(b'admin.vm.volume.CloneTo',
+                    b'test-vm1', b'private', b'no-such-token')
+        self.assertNotIn('init_volume().import_volume',
+            map(operator.itemgetter(0), self.pool.mock_calls))
         self.assertFalse(self.app.save.called)
 
     def test_530_tag_list(self):

qubes/tests/storage_lvm.py

@@ -137,7 +137,7 @@ class TC_00_ThinPool(ThinPoolBase):
         volume.create()
         path = "/dev/%s" % volume.vid
         self.assertTrue(os.path.exists(path))
-        self.pool.remove(volume)
+        volume.remove()
 
     def test_003_read_write_volume(self):
         ''' Test read-write volume creation '''
@@ -157,7 +157,7 @@ class TC_00_ThinPool(ThinPoolBase):
         volume.create()
         path = "/dev/%s" % volume.vid
         self.assertTrue(os.path.exists(path))
-        self.pool.remove(volume)
+        volume.remove()
 
 @skipUnlessLvmPoolExists
 class TC_01_ThinPool(qubes.tests.SystemTestsMixin, ThinPoolBase):