Merge branch 'lvm-snapshots'

* lvm-snapshots:
  tests: fix handling app.pools iteration
  storage/lvm: add repr(ThinPool) for more meaningful test reports
  tests: adjust for variable volume path
  api/admin: expose volume path in admin.vm.volume.Info
  tests: LVM: import, list_volumes, volatile volume, snapshot volume
  tests: collect all SIGCHLD before cleaning event loop
  storage/lvm: use temporary volume for data import
  tests: ThinVolume.revert()
  tests: LVM volume naming migration, and new naming in general
  storage/lvm: improve handling interrupted commit

qubes/api/admin.py

@@ -335,7 +335,7 @@ class QubesAdminAPI(qubes.api.AbstractQubesAPI):
         volume = self.dest.volumes[self.arg]
         # properties defined in API
         volume_properties = [
-            'pool', 'vid', 'size', 'usage', 'rw', 'source',
+            'pool', 'vid', 'size', 'usage', 'rw', 'source', 'path',
             'save_on_stop', 'snap_on_start', 'revisions_to_keep']
 
         def _serialize(value):

qubes/storage/lvm.py

@@ -20,7 +20,6 @@
 ''' Driver for storing vm images in a LVM thin pool '''
 
 import logging
-import operator
 import os
 import subprocess
 
@@ -44,8 +43,37 @@ def check_lvm_version():
 
 lvm_is_very_old = check_lvm_version()
 
+
 class ThinPool(qubes.storage.Pool):
     ''' LVM Thin based pool implementation
+
+    Volumes are stored as LVM thin volumes, in thin pool specified by
+    *volume_group*/*thin_pool* arguments. LVM volume naming scheme:
+
+        vm-{vm_name}-{volume_name}[-suffix]
+
+    Where suffix can be one of:
+        "-snap" - snapshot for currently running VM, at VM shutdown will be
+        either discarded (if save_on_stop=False), or committed
+        (if save_on_stop=True)
+        "-{revision_id}" - volume revision - new revision is automatically
+        created at each VM shutdown, *revisions_to_keep* control how many
+        old revisions (in addition to the current one) should be stored
+        "" (no suffix) - the most recent committed volume state; also volatile
+        volume (snap_on_start=False, save_on_stop=False)
+
+    On VM startup, new volume is created, depending on volume type,
+    according to the table below:
+
+    snap_on_start, save_on_stop
+    False,         False,        - no suffix, fresh empty volume
+    False,         True,         - "-snap", snapshot of last committed revision
+    True ,         False,        - "-snap", snapshot of last committed revision
+                                   of source volume (from VM's template)
+    True,          True,         - unsupported configuration
+
+    Volume's revision_id format is "{timestamp}-back", where timestamp is in
+    '%s' format (seconds since unix epoch)
     '''  # pylint: disable=protected-access
 
     size_cache = None
@@ -62,6 +90,12 @@ class ThinPool(qubes.storage.Pool):
 
         self._volume_objects_cache = {}
 
+    def __repr__(self):
+        return '<{} at {:#x} name={!r} volume_group={!r} thin_pool={!r}>'.\
+            format(
+                type(self).__name__, id(self),
+                self.name, self.volume_group, self.thin_pool)
+
     @property
     def config(self):
         return {
@@ -131,20 +165,16 @@ class ThinPool(qubes.storage.Pool):
                 continue
             if vol_info['pool_lv'] != self.thin_pool:
                 continue
-            if vid.endswith('-snap'):
+            if vid.endswith('-snap') or vid.endswith('-import'):
                 # implementation detail volume
                 continue
             if vid.endswith('-back'):
                 # old revisions
                 continue
-            config = {
-                'pool': self,
-                'vid': vid,
-                'name': vid,
-                'volume_group': self.volume_group,
-                'rw': vol_info['attr'][1] == 'w',
-            }
-            volumes += [ThinVolume(**config)]
+            volume = self.get_volume(vid)
+            if volume in volumes:
+                continue
+            volumes.append(volume)
         return volumes
 
     @property
@@ -200,6 +230,19 @@ def init_cache(log=logging.getLogger('qubes.storage.lvm')):
 
 size_cache = init_cache()
 
+
+def _revision_sort_key(revision):
+    '''Sort key for revisions. Sort them by time
+
+    :returns timestamp
+    '''
+    if isinstance(revision, tuple):
+        revision = revision[0]
+    if '-' in revision:
+        revision = revision.split('-')[0]
+    return int(revision)
+
+
 class ThinVolume(qubes.storage.Volume):
     ''' Default LVM thin volume implementation
     '''  # pylint: disable=too-few-public-methods
@@ -212,12 +255,26 @@ class ThinVolume(qubes.storage.Volume):
 
         if self.snap_on_start or self.save_on_stop:
             self._vid_snap = self.vid + '-snap'
+        if self.save_on_stop:
+            self._vid_import = self.vid + '-import'
 
         self._size = size
 
     @property
     def path(self):
-        return '/dev/' + self.vid
+        return '/dev/' + self._vid_current
+
+    @property
+    def _vid_current(self):
+        if self.vid in size_cache:
+            return self.vid
+        vol_revisions = self.revisions
+        if vol_revisions:
+            last_revision = \
+                max(vol_revisions.items(), key=_revision_sort_key)[0]
+            return self.vid + '-' + last_revision
+        # detached pool? return expected path
+        return self.vid
 
     @property
     def revisions(self):
@@ -229,7 +286,8 @@ class ThinVolume(qubes.storage.Volume):
             if not revision_vid.endswith('-back'):
                 continue
             revision_vid = revision_vid[len(name_prefix):]
-            seconds = int(revision_vid[:-len('-back')])
+            # get revision without suffix
+            seconds = int(revision_vid.split('-')[0])
             iso_date = qubes.storage.isodate(seconds).split('.', 1)[0]
             revisions[revision_vid] = iso_date
         return revisions
@@ -239,7 +297,7 @@ class ThinVolume(qubes.storage.Volume):
         try:
             if self.is_dirty():
                 return qubes.storage.lvm.size_cache[self._vid_snap]['size']
-            return qubes.storage.lvm.size_cache[self.vid]['size']
+            return qubes.storage.lvm.size_cache[self._vid_current]['size']
         except KeyError:
             return self._size
 
@@ -273,19 +331,31 @@ class ThinVolume(qubes.storage.Volume):
         '''
         if revisions is None:
             revisions = sorted(self.revisions.items(),
-                key=operator.itemgetter(1))
+                key=_revision_sort_key)
             # pylint: disable=invalid-unary-operand-type
             revisions = revisions[:(-self.revisions_to_keep) or None]
             revisions = [rev_id for rev_id, _ in revisions]
 
         for rev_id in revisions:
+            # safety check
+            assert rev_id != self._vid_current
             try:
                 cmd = ['remove', self.vid + '-' + rev_id]
                 qubes_lvm(cmd, self.log)
             except qubes.storage.StoragePoolException:
                 pass
 
-    def _commit(self):
+    def _commit(self, vid_to_commit=None, keep=False):
+        '''
+        Commit temporary volume into current one. By default
+        :py:attr:`_vid_snap` is used (which is created by :py:meth:`start()`),
+        but can be overriden by *vid_to_commit* argument.
+
+        :param vid_to_commit: LVM volume ID to commit into this one
+        :param keep: whether to keep or not *vid_to_commit*.
+          IOW use 'clone' or 'rename' methods.
+        :return: None
+        '''
         msg = "Trying to commit {!s}, but it has save_on_stop == False"
         msg = msg.format(self)
         assert self.save_on_stop, msg
@@ -293,39 +363,40 @@ class ThinVolume(qubes.storage.Volume):
         msg = "Trying to commit {!s}, but it has rw == False"
         msg = msg.format(self)
         assert self.rw, msg
-        assert hasattr(self, '_vid_snap')
+        if vid_to_commit is None:
+            assert hasattr(self, '_vid_snap')
+            vid_to_commit = self._vid_snap
 
-        if self.revisions_to_keep > 0:
-            cmd = ['clone', self.vid,
-                '{}-{}-back'.format(self.vid, int(time.time()))]
+        # TODO: when converting this function to coroutine, this _must_ be
+        # under a lock
+        if not os.path.exists('/dev/' + vid_to_commit):
+            # nothing to commit
+            return
+
+        if self._vid_current == self.vid:
+            cmd = ['rename', self.vid,
+                   '{}-{}-back'.format(self.vid, int(time.time()))]
             qubes_lvm(cmd, self.log)
             reset_cache()
-        self._remove_revisions()
 
-        # TODO: when converting this function to coroutine, this _must_ be
-        # under a lock
-        # remove old volume only after _successful_ clone of the new one
-        cmd = ['rename', self.vid, self.vid + '-tmp']
+        cmd = ['clone' if keep else 'rename',
+               vid_to_commit,
+               self.vid]
         qubes_lvm(cmd, self.log)
-        try:
-            cmd = ['clone', self._vid_snap, self.vid]
-            qubes_lvm(cmd, self.log)
-        except:
-            # restore original volume
-            cmd = ['rename', self.vid + '-tmp', self.vid]
-            qubes_lvm(cmd, self.log)
-            raise
-        else:
-            cmd = ['remove', self.vid + '-tmp']
-            qubes_lvm(cmd, self.log)
+        reset_cache()
+        # make sure the one we've committed right now is properly
+        # detected as the current one - before removing anything
+        assert self._vid_current == self.vid
 
+        # and remove old snapshots, if needed
+        self._remove_revisions()
 
     def create(self):
         assert self.vid
         assert self.size
         if self.save_on_stop:
             if self.source:
-                cmd = ['clone', str(self.source), self.vid]
+                cmd = ['clone', self.source.path, self.vid]
             else:
                 cmd = [
                     'create',
@@ -346,10 +417,17 @@ class ThinVolume(qubes.storage.Volume):
         except AttributeError:
             pass
 
+        try:
+            if os.path.exists('/dev/' + self._vid_import):
+                cmd = ['remove', self._vid_import]
+                qubes_lvm(cmd, self.log)
+        except AttributeError:
+            pass
+
         self._remove_revisions(self.revisions.keys())
         if not os.path.exists(self.path):
             return
-        cmd = ['remove', self.vid]
+        cmd = ['remove', self.path]
         qubes_lvm(cmd, self.log)
         reset_cache()
         # pylint: disable=protected-access
@@ -358,8 +436,8 @@ class ThinVolume(qubes.storage.Volume):
     def export(self):
         ''' Returns an object that can be `open()`. '''
         # make sure the device node is available
-        qubes_lvm(['activate', self.vid], self.log)
-        devpath = '/dev/' + self.vid
+        qubes_lvm(['activate', self.path], self.log)
+        devpath = self.path
         return devpath
 
     @asyncio.coroutine
@@ -367,36 +445,78 @@ class ThinVolume(qubes.storage.Volume):
         if not src_volume.save_on_stop:
             return self
 
+        if self.is_dirty():
+            raise qubes.storage.StoragePoolException(
+                'Cannot import to dirty volume {} -'
+                ' start and stop a qube to cleanup'.format(self.vid))
+        self.abort_if_import_in_progress()
         # 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)
+            self._commit(src_volume.path[len('/dev/'):], keep=True)
         else:
-            if src_volume.size != self.size:
-                self.resize(src_volume.size)
+            cmd = ['create',
+                   self.pool._pool_id,  # pylint: disable=protected-access
+                   self._vid_import.split('/')[1],
+                   str(src_volume.size)]
+            qubes_lvm(cmd, self.log)
             src_path = src_volume.export()
-            cmd = ['dd', 'if=' + src_path, 'of=/dev/' + self.vid,
-                'conv=sparse']
+            cmd = ['dd', 'if=' + src_path, 'of=/dev/' + self._vid_import,
+                'conv=sparse', 'status=none']
+            if not os.access('/dev/' + self._vid_import, os.W_OK) or \
+                    not os.access(src_path, os.R_OK):
+                cmd.insert(0, 'sudo')
+
             p = yield from asyncio.create_subprocess_exec(*cmd)
             yield from p.wait()
             if p.returncode != 0:
+                cmd = ['remove', self._vid_import]
+                qubes_lvm(cmd, self.log)
                 raise qubes.storage.StoragePoolException(
                     'Failed to import volume {!r}, dd exit code: {}'.format(
                         src_volume, p.returncode))
-            reset_cache()
+            self._commit(self._vid_import)
 
         return self
 
     def import_data(self):
         ''' Returns an object that can be `open()`. '''
-        devpath = '/dev/' + self.vid
+        if self.is_dirty():
+            raise qubes.storage.StoragePoolException(
+                'Cannot import data to dirty volume {}, stop the qube first'.
+                format(self.vid))
+        self.abort_if_import_in_progress()
+        # pylint: disable=protected-access
+        cmd = ['create', self.pool._pool_id, self._vid_import.split('/')[1],
+               str(self.size)]
+        qubes_lvm(cmd, self.log)
+        reset_cache()
+        devpath = '/dev/' + self._vid_import
         return devpath
 
+    def import_data_end(self, success):
+        '''Either commit imported data, or discard temporary volume'''
+        if not os.path.exists('/dev/' + self._vid_import):
+            raise qubes.storage.StoragePoolException(
+                'No import operation in progress on {}'.format(self.vid))
+        if success:
+            self._commit(self._vid_import)
+        else:
+            cmd = ['remove', self._vid_import]
+            qubes_lvm(cmd, self.log)
+
+    def abort_if_import_in_progress(self):
+        try:
+            devpath = '/dev/' + self._vid_import
+            if os.path.exists(devpath):
+                raise qubes.storage.StoragePoolException(
+                    'Import operation in progress on {}'.format(self.vid))
+        except AttributeError:  # self._vid_import
+            # no vid_import - import definitely not in progress
+            pass
+
     def is_dirty(self):
         if self.save_on_stop:
             return os.path.exists('/dev/' + self._vid_snap)
@@ -408,20 +528,25 @@ class ThinVolume(qubes.storage.Volume):
         if self._vid_snap not in size_cache:
             return False
         return (size_cache[self._vid_snap]['origin'] !=
-               self.source.vid.split('/')[1])
-
+               self.source.path.split('/')[-1])
 
     def revert(self, revision=None):
+        if self.is_dirty():
+            raise qubes.storage.StoragePoolException(
+                'Cannot revert dirty volume {}, stop the qube first'.format(
+                    self.vid))
+        self.abort_if_import_in_progress()
         if revision is None:
             revision = \
-                max(self.revisions.items(), key=operator.itemgetter(1))[0]
-        old_path = self.path + '-' + revision
+                max(self.revisions.items(), key=_revision_sort_key)[0]
+        old_path = '/dev/' + self.vid + '-' + revision
         if not os.path.exists(old_path):
             msg = "Volume {!s} has no {!s}".format(self, old_path)
             raise qubes.storage.StoragePoolException(msg)
 
-        cmd = ['remove', self.vid]
-        qubes_lvm(cmd, self.log)
+        if self.vid in size_cache:
+            cmd = ['remove', self.vid]
+            qubes_lvm(cmd, self.log)
         cmd = ['clone', self.vid + '-' + revision, self.vid]
         qubes_lvm(cmd, self.log)
         reset_cache()
@@ -449,8 +574,12 @@ class ThinVolume(qubes.storage.Volume):
         if self.is_dirty():
             cmd = ['extend', self._vid_snap, str(size)]
             qubes_lvm(cmd, self.log)
+        elif hasattr(self, '_vid_import') and \
+                os.path.exists('/dev/' + self._vid_import):
+            cmd = ['extend', self._vid_import, str(size)]
+            qubes_lvm(cmd, self.log)
         elif self.save_on_stop or not self.snap_on_start:
-            cmd = ['extend', self.vid, str(size)]
+            cmd = ['extend', self._vid_current, str(size)]
             qubes_lvm(cmd, self.log)
         reset_cache()
 
@@ -462,13 +591,13 @@ class ThinVolume(qubes.storage.Volume):
             pass
 
         if self.source is None:
-            cmd = ['clone', self.vid, self._vid_snap]
+            cmd = ['clone', self._vid_current, self._vid_snap]
         else:
-            cmd = ['clone', str(self.source), self._vid_snap]
+            cmd = ['clone', self.source.path, self._vid_snap]
         qubes_lvm(cmd, self.log)
 
-
     def start(self):
+        self.abort_if_import_in_progress()
         try:
             if self.snap_on_start or self.save_on_stop:
                 if not self.save_on_stop or not self.is_dirty():
@@ -483,10 +612,10 @@ class ThinVolume(qubes.storage.Volume):
         try:
             if self.save_on_stop:
                 self._commit()
-            if self.snap_on_start or self.save_on_stop:
+            if self.snap_on_start and not self.save_on_stop:
                 cmd = ['remove', self._vid_snap]
                 qubes_lvm(cmd, self.log)
-            else:
+            elif not self.snap_on_start and not self.save_on_stop:
                 cmd = ['remove', self.vid]
                 qubes_lvm(cmd, self.log)
         finally:
@@ -499,9 +628,9 @@ class ThinVolume(qubes.storage.Volume):
             # volatile volumes don't need any files
             return True
         if self.source is not None:
-            vid = str(self.source)
+            vid = self.source.path[len('/dev/'):]
         else:
-            vid = self.vid
+            vid = self._vid_current
         try:
             vol_info = size_cache[vid]
             if vol_info['attr'][4] != 'a':
@@ -528,7 +657,7 @@ class ThinVolume(qubes.storage.Volume):
     def usage(self):  # lvm thin usage always returns at least the same usage as
                       # the parent
         try:
-            return qubes.storage.lvm.size_cache[self.vid]['usage']
+            return qubes.storage.lvm.size_cache[self._vid_current]['usage']
         except KeyError:
             return 0
 

qubes/tests/__init__.py

@@ -421,6 +421,13 @@ class QubesTestCase(unittest.TestCase):
             except asyncio.TimeoutError:
                 raise AssertionError('libvirt event impl drain timeout')
 
+        # this is stupid, but apparently it requires two passes
+        # to cleanup SIGCHLD handlers
+        self.loop.stop()
+        self.loop.run_forever()
+        self.loop.stop()
+        self.loop.run_forever()
+
         # Check there are no Tasks left.
         assert not self.loop._ready
         assert not self.loop._scheduled

qubes/tests/api_admin.py

@@ -39,7 +39,7 @@ import qubes.storage
 
 # properties defined in API
 volume_properties = [
-    'pool', 'vid', 'size', 'usage', 'rw', 'source',
+    'pool', 'vid', 'size', 'usage', 'rw', 'source', 'path',
     'save_on_stop', 'snap_on_start', 'revisions_to_keep']
 
 

qubes/tests/integ/basic.py

@@ -572,7 +572,8 @@ class TC_03_QvmRevertTemplateChanges(qubes.tests.SystemTestCase):
 
     def get_rootimg_checksum(self):
         return subprocess.check_output(
-            ['sha1sum', self.test_template.volumes['root'].path])
+            ['sha1sum', self.test_template.volumes['root'].path]).\
+            decode().split(' ')[0]
 
     def _do_test(self):
         checksum_before = self.get_rootimg_checksum()

qubes/tests/storage_lvm.py

@@ -24,15 +24,15 @@
     'volume_group/thin_pool' combination. Pool variables without a prefix
     represent a :py:class:`qubes.storage.lvm.ThinPool`.
 '''
-
 import os
 import subprocess
 import tempfile
 import unittest
+import unittest.mock
 
 import qubes.tests
 import qubes.storage
-from qubes.storage.lvm import ThinPool, ThinVolume
+from qubes.storage.lvm import ThinPool, ThinVolume, qubes_lvm
 
 if 'DEFAULT_LVM_POOL' in os.environ.keys():
     DEFAULT_LVM_POOL = os.environ['DEFAULT_LVM_POOL']
@@ -84,7 +84,7 @@ class ThinPoolBase(qubes.tests.QubesTestCase):
         ''' Returns the pool matching the specified ``volume_group`` &
             ``thin_pool``, or None.
         '''
-        pools = [p for p in self.app.pools
+        pools = [p for p in self.app.pools.values()
             if issubclass(p.__class__, ThinPool)]
         for pool in pools:
             if pool.volume_group == volume_group \
@@ -138,7 +138,7 @@ class TC_00_ThinPool(ThinPoolBase):
         self.assertEqual(volume.size, qubes.config.defaults['root_img_size'])
         volume.create()
         path = "/dev/%s" % volume.vid
-        self.assertTrue(os.path.exists(path))
+        self.assertTrue(os.path.exists(path), path)
         volume.remove()
 
     def test_003_read_write_volume(self):
@@ -158,7 +158,7 @@ class TC_00_ThinPool(ThinPoolBase):
         self.assertEqual(volume.size, qubes.config.defaults['root_img_size'])
         volume.create()
         path = "/dev/%s" % volume.vid
-        self.assertTrue(os.path.exists(path))
+        self.assertTrue(os.path.exists(path), path)
         volume.remove()
 
     def test_004_size(self):
@@ -240,6 +240,697 @@ class TC_00_ThinPool(ThinPoolBase):
         self.assertEqual(self._get_size(path), new_size)
         self.assertEqual(volume.size, new_size)
 
+    def _get_lv_uuid(self, lv):
+        sudo = [] if os.getuid() == 0 else ['sudo']
+        lvs_output = subprocess.check_output(
+            sudo + ['lvs', '--noheadings', '-o', 'lv_uuid', lv])
+        return lvs_output.strip()
+
+    def _get_lv_origin_uuid(self, lv):
+        sudo = [] if os.getuid() == 0 else ['sudo']
+        if qubes.storage.lvm.lvm_is_very_old:
+            # no support for origin_uuid directly
+            lvs_output = subprocess.check_output(
+                sudo + ['lvs', '--noheadings', '-o', 'origin', lv])
+            lvs_output = subprocess.check_output(
+                sudo + ['lvs', '--noheadings', '-o', 'lv_uuid',
+                    lv.rsplit('/', 1)[0] + '/' + lvs_output.strip().decode()])
+        else:
+            lvs_output = subprocess.check_output(
+                sudo + ['lvs', '--noheadings', '-o', 'origin_uuid', lv])
+        return lvs_output.strip()
+
+    def test_008_commit(self):
+        ''' Test volume changes commit'''
+        config = {
+            'name': 'root',
+            'pool': self.pool.name,
+            'save_on_stop': True,
+            'rw': True,
+            'size': qubes.config.defaults['root_img_size'],
+        }
+        vm = qubes.tests.storage.TestVM(self)
+        volume = self.app.get_pool(self.pool.name).init_volume(vm, config)
+        volume.create()
+        path_snap = '/dev/' + volume._vid_snap
+        self.assertFalse(os.path.exists(path_snap), path_snap)
+        origin_uuid = self._get_lv_uuid(volume.path)
+        volume.start()
+        snap_uuid = self._get_lv_uuid(path_snap)
+        self.assertNotEqual(origin_uuid, snap_uuid)
+        path = volume.path
+        self.assertTrue(path.startswith('/dev/' + volume.vid),
+                        '{} does not start with /dev/{}'.format(path, volume.vid))
+        self.assertTrue(os.path.exists(path), path)
+        volume.remove()
+
+    def test_009_interrupted_commit(self):
+        ''' Test volume changes commit'''
+        config = {
+            'name': 'root',
+            'pool': self.pool.name,
+            'save_on_stop': True,
+            'rw': True,
+            'revisions_to_keep': 2,
+            'size': qubes.config.defaults['root_img_size'],
+        }
+        vm = qubes.tests.storage.TestVM(self)
+        volume = self.app.get_pool(self.pool.name).init_volume(vm, config)
+        # mock logging, to not interfere with time.time() mock
+        volume.log = unittest.mock.Mock()
+        # do not call volume.create(), do it manually to simulate
+        # interrupted commit
+        revisions = ['-1521065904-back', '-1521065905-back', '-snap']
+        orig_uuids = {}
+        for rev in revisions:
+            cmd = ['create', self.pool._pool_id,
+                   volume.vid.split('/')[1] + rev, str(config['size'])]
+            qubes_lvm(cmd)
+            orig_uuids[rev] = self._get_lv_uuid(volume.vid + rev)
+        qubes.storage.lvm.reset_cache()
+        path_snap = '/dev/' + volume._vid_snap
+        self.assertTrue(volume.is_dirty())
+        self.assertEqual(volume.path,
+                         '/dev/' + volume.vid + revisions[1])
+        expected_revisions = {
+            revisions[0].lstrip('-'): '2018-03-14T22:18:24',
+            revisions[1].lstrip('-'): '2018-03-14T22:18:25',
+        }
+        self.assertEqual(volume.revisions, expected_revisions)
+        volume.start()
+        self.assertEqual(volume.revisions, expected_revisions)
+        snap_uuid = self._get_lv_uuid(path_snap)
+        self.assertEqual(orig_uuids['-snap'], snap_uuid)
+        self.assertTrue(volume.is_dirty())
+        self.assertEqual(volume.path,
+                         '/dev/' + volume.vid + revisions[1])
+        with unittest.mock.patch('time.time') as mock_time:
+            mock_time.side_effect = [521065906]
+            volume.stop()
+        expected_revisions = {
+            revisions[0].lstrip('-'): '2018-03-14T22:18:24',
+            revisions[1].lstrip('-'): '2018-03-14T22:18:25',
+        }
+        self.assertFalse(volume.is_dirty())
+        self.assertEqual(volume.revisions, expected_revisions)
+        self.assertEqual(volume.path, '/dev/' + volume.vid)
+        self.assertEqual(snap_uuid, self._get_lv_uuid(volume.path))
+        self.assertFalse(os.path.exists(path_snap), path_snap)
+
+        volume.remove()
+
+    def test_010_migration1(self):
+        '''Start with old revisions, then start interacting using new code'''
+        config = {
+            'name': 'root',
+            'pool': self.pool.name,
+            'save_on_stop': True,
+            'rw': True,
+            'revisions_to_keep': 2,
+            'size': qubes.config.defaults['root_img_size'],
+        }
+        vm = qubes.tests.storage.TestVM(self)
+        volume = self.app.get_pool(self.pool.name).init_volume(vm, config)
+        # mock logging, to not interfere with time.time() mock
+        volume.log = unittest.mock.Mock()
+        # do not call volume.create(), do it manually to have old LV naming
+        revisions = ['', '-1521065904-back', '-1521065905-back']
+        orig_uuids = {}
+        for rev in revisions:
+            cmd = ['create', self.pool._pool_id,
+                   volume.vid.split('/')[1] + rev, str(config['size'])]
+            qubes_lvm(cmd)
+            orig_uuids[rev] = self._get_lv_uuid(volume.vid + rev)
+        qubes.storage.lvm.reset_cache()
+        path_snap = '/dev/' + volume._vid_snap
+        self.assertFalse(os.path.exists(path_snap), path_snap)
+        expected_revisions = {
+            revisions[1].lstrip('-'): '2018-03-14T22:18:24',
+            revisions[2].lstrip('-'): '2018-03-14T22:18:25',
+        }
+        self.assertEqual(volume.revisions, expected_revisions)
+        self.assertEqual(volume.path, '/dev/' + volume.vid)
+
+        volume.start()
+        snap_uuid = self._get_lv_uuid(path_snap)
+        self.assertNotEqual(orig_uuids[''], snap_uuid)
+        snap_origin_uuid = self._get_lv_origin_uuid(path_snap)
+        self.assertEqual(orig_uuids[''], snap_origin_uuid)
+        path = volume.path
+        self.assertEqual(path, '/dev/' + volume.vid)
+        self.assertTrue(os.path.exists(path), path)
+
+        with unittest.mock.patch('time.time') as mock_time:
+            mock_time.side_effect = ('1521065906', '1521065907')
+            volume.stop()
+        revisions.extend(['-1521065906-back'])
+        expected_revisions = {
+            revisions[2].lstrip('-'): '2018-03-14T22:18:25',
+            revisions[3].lstrip('-'): '2018-03-14T22:18:26',
+        }
+        self.assertEqual(volume.revisions, expected_revisions)
+        self.assertEqual(volume.path, '/dev/' + volume.vid)
+        path_snap = '/dev/' + volume._vid_snap
+        self.assertFalse(os.path.exists(path_snap), path_snap)
+        self.assertTrue(os.path.exists('/dev/' + volume.vid))
+        self.assertEqual(self._get_lv_uuid(volume.path), snap_uuid)
+        prev_path = '/dev/' + volume.vid + revisions[3]
+        self.assertEqual(self._get_lv_uuid(prev_path), orig_uuids[''])
+
+        volume.remove()
+        for rev in revisions:
+            path = '/dev/' + volume.vid + rev
+            self.assertFalse(os.path.exists(path), path)
+
+    def test_011_migration2(self):
+        '''VM started with old code, stopped with new'''
+        config = {
+            'name': 'root',
+            'pool': self.pool.name,
+            'save_on_stop': True,
+            'rw': True,
+            'revisions_to_keep': 1,
+            'size': qubes.config.defaults['root_img_size'],
+        }
+        vm = qubes.tests.storage.TestVM(self)
+        volume = self.app.get_pool(self.pool.name).init_volume(vm, config)
+        # mock logging, to not interfere with time.time() mock
+        volume.log = unittest.mock.Mock()
+        # do not call volume.create(), do it manually to have old LV naming
+        revisions = ['', '-snap']
+        orig_uuids = {}
+        for rev in revisions:
+            cmd = ['create', self.pool._pool_id,
+                   volume.vid.split('/')[1] + rev, str(config['size'])]
+            qubes_lvm(cmd)
+            orig_uuids[rev] = self._get_lv_uuid(volume.vid + rev)
+        qubes.storage.lvm.reset_cache()
+        path_snap = '/dev/' + volume._vid_snap
+        self.assertTrue(os.path.exists(path_snap), path_snap)
+        expected_revisions = {}
+        self.assertEqual(volume.revisions, expected_revisions)
+        self.assertEqual(volume.path, '/dev/' + volume.vid)
+        self.assertTrue(volume.is_dirty())
+
+        path = volume.path
+        self.assertEqual(path, '/dev/' + volume.vid)
+        self.assertTrue(os.path.exists(path), path)
+
+        with unittest.mock.patch('time.time') as mock_time:
+            mock_time.side_effect = ('1521065906', '1521065907')
+            volume.stop()
+        revisions.extend(['-1521065906-back'])
+        expected_revisions = {
+            revisions[2].lstrip('-'): '2018-03-14T22:18:26',
+        }
+        self.assertEqual(volume.revisions, expected_revisions)
+        self.assertEqual(volume.path, '/dev/' + volume.vid)
+        path_snap = '/dev/' + volume._vid_snap
+        self.assertFalse(os.path.exists(path_snap), path_snap)
+        self.assertTrue(os.path.exists('/dev/' + volume.vid))
+        self.assertEqual(self._get_lv_uuid(volume.path), orig_uuids['-snap'])
+        prev_path = '/dev/' + volume.vid + revisions[2]
+        self.assertEqual(self._get_lv_uuid(prev_path), orig_uuids[''])
+
+        volume.remove()
+        for rev in revisions:
+            path = '/dev/' + volume.vid + rev
+            self.assertFalse(os.path.exists(path), path)
+
+    def test_012_migration3(self):
+        '''VM started with old code, started again with new, stopped with new'''
+        config = {
+            'name': 'root',
+            'pool': self.pool.name,
+            'save_on_stop': True,
+            'rw': True,
+            'revisions_to_keep': 1,
+            'size': qubes.config.defaults['root_img_size'],
+        }
+        vm = qubes.tests.storage.TestVM(self)
+        volume = self.app.get_pool(self.pool.name).init_volume(vm, config)
+        # mock logging, to not interfere with time.time() mock
+        volume.log = unittest.mock.Mock()
+        # do not call volume.create(), do it manually to have old LV naming
+        revisions = ['', '-snap']
+        orig_uuids = {}
+        for rev in revisions:
+            cmd = ['create', self.pool._pool_id,
+                   volume.vid.split('/')[1] + rev, str(config['size'])]
+            qubes_lvm(cmd)
+            orig_uuids[rev] = self._get_lv_uuid(volume.vid + rev)
+        qubes.storage.lvm.reset_cache()
+        path_snap = '/dev/' + volume._vid_snap
+        self.assertTrue(os.path.exists(path_snap), path_snap)
+        expected_revisions = {}
+        self.assertEqual(volume.revisions, expected_revisions)
+        self.assertTrue(volume.path, '/dev/' + volume.vid)
+        self.assertTrue(volume.is_dirty())
+
+        volume.start()
+        self.assertEqual(volume.revisions, expected_revisions)
+        self.assertEqual(volume.path, '/dev/' + volume.vid)
+        # -snap LV should be unchanged
+        self.assertEqual(self._get_lv_uuid(volume._vid_snap),
+                         orig_uuids['-snap'])
+
+        volume.remove()
+        for rev in revisions:
+            path = '/dev/' + volume.vid + rev
+            self.assertFalse(os.path.exists(path), path)
+
+    def test_013_migration4(self):
+        '''revisions_to_keep=0, VM started with old code, stopped with new'''
+        config = {
+            'name': 'root',
+            'pool': self.pool.name,
+            'save_on_stop': True,
+            'rw': True,
+            'revisions_to_keep': 0,
+            'size': qubes.config.defaults['root_img_size'],
+        }
+        vm = qubes.tests.storage.TestVM(self)
+        volume = self.app.get_pool(self.pool.name).init_volume(vm, config)
+        # mock logging, to not interfere with time.time() mock
+        volume.log = unittest.mock.Mock()
+        # do not call volume.create(), do it manually to have old LV naming
+        revisions = ['', '-snap']
+        orig_uuids = {}
+        for rev in revisions:
+            cmd = ['create', self.pool._pool_id,
+                   volume.vid.split('/')[1] + rev, str(config['size'])]
+            qubes_lvm(cmd)
+            orig_uuids[rev] = self._get_lv_uuid(volume.vid + rev)
+        qubes.storage.lvm.reset_cache()
+        path_snap = '/dev/' + volume._vid_snap
+        self.assertTrue(os.path.exists(path_snap), path_snap)
+        expected_revisions = {}
+        self.assertEqual(volume.revisions, expected_revisions)
+        self.assertEqual(volume.path, '/dev/' + volume.vid)
+        self.assertTrue(volume.is_dirty())
+
+        with unittest.mock.patch('time.time') as mock_time:
+            mock_time.side_effect = ('1521065906', '1521065907')
+            volume.stop()
+        expected_revisions = {}
+        self.assertEqual(volume.revisions, expected_revisions)
+        self.assertEqual(volume.path, '/dev/' + volume.vid)
+
+        volume.remove()
+        for rev in revisions:
+            path = '/dev/' + volume.vid + rev
+            self.assertFalse(os.path.exists(path), path)
+
+    def test_014_commit_keep_0(self):
+        ''' Test volume changes commit, with revisions_to_keep=0'''
+        config = {
+            'name': 'root',
+            'pool': self.pool.name,
+            'save_on_stop': True,
+            'rw': True,
+            'revisions_to_keep': 0,
+            'size': qubes.config.defaults['root_img_size'],
+        }
+        vm = qubes.tests.storage.TestVM(self)
+        volume = self.app.get_pool(self.pool.name).init_volume(vm, config)
+        # mock logging, to not interfere with time.time() mock
+        volume.log = unittest.mock.Mock()
+        volume.create()
+        self.assertFalse(volume.is_dirty())
+        path = volume.path
+        expected_revisions = {}
+        self.assertEqual(volume.revisions, expected_revisions)
+
+        volume.start()
+        self.assertEqual(volume.revisions, expected_revisions)
+        path_snap = '/dev/' + volume._vid_snap
+        snap_uuid = self._get_lv_uuid(path_snap)
+        self.assertTrue(volume.is_dirty())
+        self.assertEqual(volume.path, path)
+
+        with unittest.mock.patch('time.time') as mock_time:
+            mock_time.side_effect = [521065906]
+            volume.stop()
+        self.assertFalse(volume.is_dirty())
+        self.assertEqual(volume.revisions, {})
+        self.assertEqual(volume.path, '/dev/' + volume.vid)
+        self.assertEqual(snap_uuid, self._get_lv_uuid(volume.path))
+        self.assertFalse(os.path.exists(path_snap), path_snap)
+
+        volume.remove()
+
+    def test_020_revert_last(self):
+        ''' Test volume revert'''
+        config = {
+            'name': 'root',
+            'pool': self.pool.name,
+            'save_on_stop': True,
+            'rw': True,
+            'revisions_to_keep': 2,
+            'size': qubes.config.defaults['root_img_size'],
+        }
+        vm = qubes.tests.storage.TestVM(self)
+        volume = self.app.get_pool(self.pool.name).init_volume(vm, config)
+        volume.create()
+        volume.start()
+        volume.stop()
+        volume.start()
+        volume.stop()
+        self.assertEqual(len(volume.revisions), 2)
+        revisions = volume.revisions
+        revision_id = max(revisions.keys())
+        current_path = volume.path
+        current_uuid = self._get_lv_uuid(volume.path)
+        rev_uuid = self._get_lv_uuid(volume.vid + '-' + revision_id)
+        self.assertFalse(volume.is_dirty())
+        self.assertNotEqual(current_uuid, rev_uuid)
+        volume.revert()
+        path_snap = '/dev/' + volume._vid_snap
+        self.assertFalse(os.path.exists(path_snap), path_snap)
+        self.assertEqual(current_path, volume.path)
+        new_uuid = self._get_lv_origin_uuid(volume.path)
+        self.assertEqual(new_uuid, rev_uuid)
+        self.assertEqual(volume.revisions, revisions)
+
+        volume.remove()
+
+    def test_021_revert_earlier(self):
+        ''' Test volume revert'''
+        config = {
+            'name': 'root',
+            'pool': self.pool.name,
+            'save_on_stop': True,
+            'rw': True,
+            'revisions_to_keep': 2,
+            'size': qubes.config.defaults['root_img_size'],
+        }
+        vm = qubes.tests.storage.TestVM(self)
+        volume = self.app.get_pool(self.pool.name).init_volume(vm, config)
+        volume.create()
+        volume.start()
+        volume.stop()
+        volume.start()
+        volume.stop()
+        self.assertEqual(len(volume.revisions), 2)
+        revisions = volume.revisions
+        revision_id = min(revisions.keys())
+        current_path = volume.path
+        current_uuid = self._get_lv_uuid(volume.path)
+        rev_uuid = self._get_lv_uuid(volume.vid + '-' + revision_id)
+        self.assertFalse(volume.is_dirty())
+        self.assertNotEqual(current_uuid, rev_uuid)
+        volume.revert(revision_id)
+        path_snap = '/dev/' + volume._vid_snap
+        self.assertFalse(os.path.exists(path_snap), path_snap)
+        self.assertEqual(current_path, volume.path)
+        new_uuid = self._get_lv_origin_uuid(volume.path)
+        self.assertEqual(new_uuid, rev_uuid)
+        self.assertEqual(volume.revisions, revisions)
+
+        volume.remove()
+
+    def test_030_import_data(self):
+        ''' Test volume import'''
+        config = {
+            'name': 'root',
+            'pool': self.pool.name,
+            'save_on_stop': True,
+            'rw': True,
+            'revisions_to_keep': 2,
+            'size': qubes.config.defaults['root_img_size'],
+        }
+        vm = qubes.tests.storage.TestVM(self)
+        volume = self.app.get_pool(self.pool.name).init_volume(vm, config)
+        volume.create()
+        current_uuid = self._get_lv_uuid(volume.path)
+        self.assertFalse(volume.is_dirty())
+        import_path = volume.import_data()
+        import_uuid = self._get_lv_uuid(import_path)
+        self.assertNotEqual(current_uuid, import_uuid)
+        # success - commit data
+        volume.import_data_end(True)
+        new_current_uuid = self._get_lv_uuid(volume.path)
+        self.assertEqual(new_current_uuid, import_uuid)
+        revisions = volume.revisions
+        self.assertEqual(len(revisions), 1)
+        revision = revisions.popitem()[0]
+        self.assertEqual(current_uuid,
+                         self._get_lv_uuid(volume.vid + '-' + revision))
+        self.assertFalse(os.path.exists(import_path), import_path)
+
+        volume.remove()
+
+    def test_031_import_data_fail(self):
+        ''' Test volume import'''
+        config = {
+            'name': 'root',
+            'pool': self.pool.name,
+            'save_on_stop': True,
+            'rw': True,
+            'revisions_to_keep': 2,
+            'size': qubes.config.defaults['root_img_size'],
+        }
+        vm = qubes.tests.storage.TestVM(self)
+        volume = self.app.get_pool(self.pool.name).init_volume(vm, config)
+        volume.create()
+        current_uuid = self._get_lv_uuid(volume.path)
+        self.assertFalse(volume.is_dirty())
+        import_path = volume.import_data()
+        import_uuid = self._get_lv_uuid(import_path)
+        self.assertNotEqual(current_uuid, import_uuid)
+        # fail - discard data
+        volume.import_data_end(False)
+        new_current_uuid = self._get_lv_uuid(volume.path)
+        self.assertEqual(new_current_uuid, current_uuid)
+        revisions = volume.revisions
+        self.assertEqual(len(revisions), 0)
+        self.assertFalse(os.path.exists(import_path), import_path)
+
+        volume.remove()
+
+    def test_032_import_volume_same_pool(self):
+        '''Import volume from the same pool'''
+        # source volume
+        config = {
+            'name': 'root',
+            'pool': self.pool.name,
+            'save_on_stop': True,
+            'rw': True,
+            'revisions_to_keep': 2,
+            'size': qubes.config.defaults['root_img_size'],
+        }
+        vm = qubes.tests.storage.TestVM(self)
+        source_volume = self.app.get_pool(self.pool.name).init_volume(vm, config)
+        source_volume.create()
+
+        source_uuid = self._get_lv_uuid(source_volume.path)
+
+        # destination volume
+        config = {
+            'name': 'root2',
+            'pool': self.pool.name,
+            'save_on_stop': True,
+            'rw': True,
+            'revisions_to_keep': 2,
+            'size': qubes.config.defaults['root_img_size'],
+        }
+        volume = self.app.get_pool(self.pool.name).init_volume(vm, config)
+        volume.log = unittest.mock.Mock()
+        with unittest.mock.patch('time.time') as mock_time:
+            mock_time.side_effect = [1521065905]
+            volume.create()
+
+        self.assertEqual(volume.revisions, {})
+        uuid_before = self._get_lv_uuid(volume.path)
+
+        with unittest.mock.patch('time.time') as mock_time:
+            mock_time.side_effect = [1521065906]
+            self.loop.run_until_complete(
+                volume.import_volume(source_volume))
+
+        uuid_after = self._get_lv_uuid(volume.path)
+        self.assertNotEqual(uuid_after, uuid_before)
+
+        # also should be different than source volume (clone, not the same LV)
+        self.assertNotEqual(uuid_after, source_uuid)
+        self.assertEqual(self._get_lv_origin_uuid(volume.path), source_uuid)
+
+        expected_revisions = {
+            '1521065906-back': '2018-03-14T22:18:26',
+        }
+        self.assertEqual(volume.revisions, expected_revisions)
+
+        volume.remove()
+        source_volume.remove()
+
+    def test_033_import_volume_different_pool(self):
+        '''Import volume from a different pool'''
+        source_volume = unittest.mock.Mock()
+        # destination volume
+        config = {
+            'name': 'root2',
+            'pool': self.pool.name,
+            'save_on_stop': True,
+            'rw': True,
+            'revisions_to_keep': 2,
+            'size': qubes.config.defaults['root_img_size'],
+        }
+        vm = qubes.tests.storage.TestVM(self)
+        volume = self.app.get_pool(self.pool.name).init_volume(vm, config)
+        volume.log = unittest.mock.Mock()
+        with unittest.mock.patch('time.time') as mock_time:
+            mock_time.side_effect = [1521065905]
+            volume.create()
+
+        self.assertEqual(volume.revisions, {})
+        uuid_before = self._get_lv_uuid(volume.path)
+
+        with tempfile.NamedTemporaryFile() as source_volume_file:
+            source_volume_file.write(b'test-content')
+            source_volume_file.flush()
+            source_volume.size = 16 * 1024 * 1024  # 16MiB
+            source_volume.export.return_value = source_volume_file.name
+            with unittest.mock.patch('time.time') as mock_time:
+                mock_time.side_effect = [1521065906]
+                self.loop.run_until_complete(
+                    volume.import_volume(source_volume))
+
+        uuid_after = self._get_lv_uuid(volume.path)
+        self.assertNotEqual(uuid_after, uuid_before)
+        self.assertEqual(volume.size, 16 * 1024 * 1024)
+
+        volume_content = subprocess.check_output(['sudo', 'cat', volume.path])
+        self.assertEqual(volume_content.rstrip(b'\0'), b'test-content')
+
+        expected_revisions = {
+            '1521065906-back': '2018-03-14T22:18:26',
+        }
+        self.assertEqual(volume.revisions, expected_revisions)
+
+        volume.remove()
+
+    def test_040_volatile(self):
+        '''Volatile volume test'''
+        config = {
+            'name': 'volatile',
+            'pool': self.pool.name,
+            'rw': True,
+            'size': qubes.config.defaults['root_img_size'],
+        }
+        vm = qubes.tests.storage.TestVM(self)
+        volume = self.app.get_pool(self.pool.name).init_volume(vm, config)
+        # volatile volume don't need any file, verify should succeed
+        self.assertTrue(volume.verify())
+        volume.create()
+        self.assertTrue(volume.verify())
+        self.assertFalse(volume.save_on_stop)
+        self.assertFalse(volume.snap_on_start)
+        path = volume.path
+        self.assertEqual(path, '/dev/' + volume.vid)
+        self.assertFalse(os.path.exists(path))
+        volume.start()
+        self.assertTrue(os.path.exists(path))
+        vol_uuid = self._get_lv_uuid(path)
+        volume.start()
+        self.assertTrue(os.path.exists(path))
+        vol_uuid2 = self._get_lv_uuid(path)
+        self.assertNotEqual(vol_uuid, vol_uuid2)
+        volume.stop()
+        self.assertFalse(os.path.exists(path))
+
+    def test_050_snapshot_volume(self):
+        ''' Test snapshot volume creation '''
+        config_origin = {
+            'name': 'root',
+            'pool': self.pool.name,
+            'save_on_stop': True,
+            'rw': True,
+            'size': qubes.config.defaults['root_img_size'],
+        }
+        vm = qubes.tests.storage.TestVM(self)
+        volume_origin = self.app.get_pool(self.pool.name).init_volume(
+            vm, config_origin)
+        volume_origin.create()
+        config_snapshot = {
+            'name': 'root2',
+            'pool': self.pool.name,
+            'snap_on_start': True,
+            'source': volume_origin,
+            'rw': True,
+            'size': qubes.config.defaults['root_img_size'],
+        }
+        volume = self.app.get_pool(self.pool.name).init_volume(
+            vm, config_snapshot)
+        self.assertIsInstance(volume, ThinVolume)
+        self.assertEqual(volume.name, 'root2')
+        self.assertEqual(volume.pool, self.pool.name)
+        self.assertEqual(volume.size, qubes.config.defaults['root_img_size'])
+        # only origin volume really needs to exist, verify should succeed
+        # even before create
+        self.assertTrue(volume.verify())
+        volume.create()
+        path = volume.path
+        self.assertEqual(path, '/dev/' + volume.vid)
+        self.assertFalse(os.path.exists(path), path)
+        volume.start()
+        # snapshot volume isn't considered dirty at any time
+        self.assertFalse(volume.is_dirty())
+        # not outdated yet
+        self.assertFalse(volume.is_outdated())
+        origin_uuid = self._get_lv_uuid(volume_origin.path)
+        snap_origin_uuid = self._get_lv_origin_uuid(volume._vid_snap)
+        self.assertEqual(origin_uuid, snap_origin_uuid)
+
+        # now make it outdated
+        volume_origin.start()
+        volume_origin.stop()
+        self.assertTrue(volume.is_outdated())
+        origin_uuid = self._get_lv_uuid(volume_origin.path)
+        self.assertNotEqual(origin_uuid, snap_origin_uuid)
+
+        volume.stop()
+        # stopped volume is never outdated
+        self.assertFalse(volume.is_outdated())
+        path = volume.path
+        self.assertFalse(os.path.exists(path), path)
+        path = '/dev/' + volume._vid_snap
+        self.assertFalse(os.path.exists(path), path)
+
+        volume.remove()
+        volume_origin.remove()
+
+    def test_100_pool_list_volumes(self):
+        config = {
+            'name': 'root',
+            'pool': self.pool.name,
+            'save_on_stop': True,
+            'rw': True,
+            'revisions_to_keep': 2,
+            'size': qubes.config.defaults['root_img_size'],
+        }
+        config2 = config.copy()
+        vm = qubes.tests.storage.TestVM(self)
+        volume1 = self.app.get_pool(self.pool.name).init_volume(vm, config)
+        volume1.create()
+        config2['name'] = 'private'
+        volume2 = self.app.get_pool(self.pool.name).init_volume(vm, config2)
+        volume2.create()
+
+        # create some revisions
+        volume1.start()
+        volume1.stop()
+
+        # and have one in dirty state
+        volume2.start()
+
+        self.assertIn(volume1, list(self.pool.volumes))
+        self.assertIn(volume2, list(self.pool.volumes))
+        volume1.remove()
+        self.assertNotIn(volume1, list(self.pool.volumes))
+        self.assertIn(volume2, list(self.pool.volumes))
+        volume2.remove()
+        self.assertNotIn(volume1, list(self.pool.volumes))
+        self.assertNotIn(volume1, list(self.pool.volumes))
 
 @skipUnlessLvmPoolExists
 class TC_01_ThinPool(ThinPoolBase, qubes.tests.SystemTestCase):