From 11c7b4bb512023b76cc9a70c987abceea8e0e785 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Wed, 14 Mar 2018 22:54:00 +0100 Subject: [PATCH] storage/lvm: improve handling interrupted commit First rename volume to backup revision, regardless of revisions_to_keep, then rename -snap to current volume. And only then remove backup revision (if exceed revisions_to_keep). This way even if commit operation is interrupted, there is still a volume with the data. This requires also adjusting few functions to actually fallback to most recent backup revision if the current volume isn't found - create _vid_current property for this purpose. Also, use -snap volume for clone operation and commit it normally later. This makes it safer to interrupt or even revert. QubesOS/qubes-issues#2256 --- qubes/storage/lvm.py | 195 +++++++++++++++++++++++++------------ qubes/tests/storage_lvm.py | 4 +- 2 files changed, 135 insertions(+), 64 deletions(-) diff --git a/qubes/storage/lvm.py b/qubes/storage/lvm.py index aa7c3c12..83d2c3bb 100644 --- a/qubes/storage/lvm.py +++ b/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 @@ -137,14 +165,10 @@ class ThinPool(qubes.storage.Pool): 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 +224,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 @@ -217,7 +254,19 @@ class ThinVolume(qubes.storage.Volume): @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 +278,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 +289,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 +323,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 +355,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 self.revisions_to_keep > 0: - cmd = ['clone', self.vid, - '{}-{}-back'.format(self.vid, int(time.time()))] - qubes_lvm(cmd, self.log) - reset_cache() - self._remove_revisions() + if vid_to_commit is None: + assert hasattr(self, '_vid_snap') + vid_to_commit = self._vid_snap # 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'] - 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) + 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() + + cmd = ['clone' if keep else 'rename', + vid_to_commit, + self.vid] + 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', @@ -349,7 +412,7 @@ class ThinVolume(qubes.storage.Volume): 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 +421,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,28 +430,32 @@ 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)) # 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, keep=True) else: - if src_volume.size != self.size: - self.resize(src_volume.size) + cmd = ['create', self.pool._pool_id, self._vid_snap.split('/')[1], + str(src_volume.size)] + qubes_lvm(cmd) src_path = src_volume.export() - cmd = ['dd', 'if=' + src_path, 'of=/dev/' + self.vid, + cmd = ['dd', 'if=' + src_path, 'of=/dev/' + self._vid_snap, 'conv=sparse'] p = yield from asyncio.create_subprocess_exec(*cmd) yield from p.wait() if p.returncode != 0: + cmd = ['remove', self._vid_snap] + qubes_lvm(cmd) raise qubes.storage.StoragePoolException( 'Failed to import volume {!r}, dd exit code: {}'.format( src_volume, p.returncode)) - reset_cache() + self._commit() return self @@ -408,20 +475,24 @@ 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)) 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() @@ -450,7 +521,7 @@ class ThinVolume(qubes.storage.Volume): cmd = ['extend', self._vid_snap, 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,9 +533,9 @@ 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) @@ -483,10 +554,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 +570,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 +599,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 diff --git a/qubes/tests/storage_lvm.py b/qubes/tests/storage_lvm.py index f7082cad..462d6cee 100644 --- a/qubes/tests/storage_lvm.py +++ b/qubes/tests/storage_lvm.py @@ -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):