From 9197bde76eb9c74f239f7cae7ba81fb9d8f82868 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Thu, 3 Nov 2016 03:47:28 +0100 Subject: [PATCH 1/9] storage/lvm: use dd for importing volumes ...instead of manual copy in python. DD is much faster and when used with `conv=sparse` it will correctly preserve sparse image. QubesOS/qubes-issues#2256 --- qubes/storage/lvm.py | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/qubes/storage/lvm.py b/qubes/storage/lvm.py index ea7d1165..4d59ca47 100644 --- a/qubes/storage/lvm.py +++ b/qubes/storage/lvm.py @@ -141,19 +141,9 @@ class ThinPool(qubes.storage.Pool): else: dst_volume = self.create(dst_volume) - cmd = ['sudo', 'qubes-lvm', 'import', dst_volume.vid] - blk_size = 4096 - p = subprocess.Popen(cmd, stdin=subprocess.PIPE) - dst = p.stdin - with open(src_path, 'rb') as src: - while True: - tmp = src.read(blk_size) - if not tmp: - break - else: - dst.write(tmp) - p.stdin.close() - p.wait() + cmd = ['sudo', 'dd', 'if=' + src_path, 'of=/dev/' + dst_volume.vid, + 'conv=sparse'] + subprocess.check_call(cmd) reset_cache() return dst_volume From 047145377365a88de7bd46a8dafe704c1eb2d9c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Thu, 3 Nov 2016 03:53:06 +0100 Subject: [PATCH 2/9] storage/lvm: call `lvm` directly, don't use qubes-lvm wrapper The wrapper doesn't do anything else than translating command parameters, but it's load time is significant (because of python imports mostly). Since we can't use python lvm API from non-root user anyway, lets drop the wrapper and call `lvm` directly (or through sudo when necessary). This makes VM startup much faster - storage preparation is down from over 10s to about 3s. QubesOS/qubes-issues#2256 --- qubes/storage/lvm.py | 80 ++++++----- qubes/tools/qubes_lvm.py | 277 --------------------------------------- rpm_spec/core-dom0.spec | 1 - 3 files changed, 46 insertions(+), 312 deletions(-) delete mode 100644 qubes/tools/qubes_lvm.py diff --git a/qubes/storage/lvm.py b/qubes/storage/lvm.py index 4d59ca47..fd0cbdc2 100644 --- a/qubes/storage/lvm.py +++ b/qubes/storage/lvm.py @@ -267,37 +267,27 @@ class ThinPool(qubes.storage.Pool): def verify(self, volume): ''' Verifies the volume. ''' - cmd = ['sudo', 'qubes-lvm', 'volumes', - self.volume_group + '/' + self.thin_pool] - p = subprocess.Popen(cmd, stdout=subprocess.PIPE) - result = p.communicate()[0] - for line in result.splitlines(): - if not line.strip(): - continue - vid, atr = line.strip().split(' ') - if vid == volume.vid: - return atr[4] == 'a' - - return False + try: + vol_info = size_cache[volume.vid] + return vol_info['attr'][4] == 'a' + except KeyError: + return False @property def volumes(self): ''' Return a list of volumes managed by this pool ''' - cmd = ['sudo', 'qubes-lvm', 'volumes', - self.volume_group + '/' + self.thin_pool] - p = subprocess.Popen(cmd, stdout=subprocess.PIPE) - result = p.communicate()[0] volumes = [] - for line in result.splitlines(): - if not line.strip(): + for vid, vol_info in size_cache.items(): + if not vid.startswith(self.volume_group + '/'): + continue + if vol_info['pool_lv'] != self.thin_pool: continue - vid, atr = line.strip().split(' ') config = { 'pool': self.name, 'vid': vid, 'name': vid, 'volume_group': self.volume_group, - 'rw': atr[1] == 'w', + 'rw': vol_info['attr'][1] == 'w', } volumes += [ThinVolume(**config)] return volumes @@ -315,10 +305,13 @@ class ThinPool(qubes.storage.Pool): def init_cache(log=logging.getLogger('qube.storage.lvm')): - cmd = ['sudo', 'lvs', '--noheadings', '-o', - 'vg_name,name,lv_size,data_percent', '--units', 'b', '--separator', - ','] - p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + cmd = ['lvs', '--noheadings', '-o', + 'vg_name,pool_lv,name,lv_size,data_percent,lv_attr', + '--units', 'b', '--separator', ','] + if os.getuid() != 0: + cmd.insert(0, 'sudo') + p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, + close_fds=True) out, err = p.communicate() return_code = p.returncode if return_code == 0 and err: @@ -330,13 +323,14 @@ def init_cache(log=logging.getLogger('qube.storage.lvm')): for line in out.splitlines(): line = line.strip() - pool_name, name, size, usage_percent = line.split(',', 3) - if '' in [pool_name, name, size, usage_percent]: + pool_name, pool_lv, name, size, usage_percent, attr = line.split(',', 5) + if '' in [pool_name, pool_lv, name, size, usage_percent]: continue name = pool_name + "/" + name size = int(size[:-1]) usage = int(size / 100 * float(usage_percent)) - result[name] = {'size':size, 'usage': usage} + result[name] = {'size': size, 'usage': usage, 'pool_lv': pool_lv, + 'attr': attr} return result @@ -416,16 +410,34 @@ class ThinVolume(qubes.storage.Volume): def pool_exists(pool_id): ''' Return true if pool exists ''' - cmd = ['pool', pool_id] - return qubes_lvm(cmd) + try: + vol_info = size_cache[pool_id] + return vol_info['attr'][0] == 't' + except KeyError: + return False def qubes_lvm(cmd, log=logging.getLogger('qube.storage.lvm')): - ''' Call :program:`qubes-lvm` to execute an LVM operation ''' - # TODO Refactor this ones the udev groups gets fixed and we don't need root - # for operations on lvm devices - cmd = ['sudo', 'qubes-lvm'] + cmd - p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + ''' Call :program:`lvm` to execute an LVM operation ''' + action = cmd[0] + if action == 'remove': + lvm_cmd = ['lvremove', '-f', cmd[1]] + elif action == 'clone': + lvm_cmd = ['lvcreate', '-kn', '-ay', '-s', cmd[1], '-n', cmd[2]] + elif action == 'create': + lvm_cmd = ['lvcreate', '-T', cmd[1], '-kn', '-ay', '-n', cmd[2], '-V', + str(cmd[3]) + 'B'] + elif action == 'extend': + size = int(cmd[2]) / (1000 * 1000) + lvm_cmd = ["lvextend", "-L%s" % size, cmd[1]] + else: + raise NotImplementedError('unsupported action: ' + action) + if os.getuid() != 0: + cmd = ['sudo', 'lvm'] + lvm_cmd + else: + cmd = ['lvm'] + lvm_cmd + p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, + close_fds=True) out, err = p.communicate() return_code = p.returncode if out: diff --git a/qubes/tools/qubes_lvm.py b/qubes/tools/qubes_lvm.py deleted file mode 100644 index 8ad9627c..00000000 --- a/qubes/tools/qubes_lvm.py +++ /dev/null @@ -1,277 +0,0 @@ -#!/usr/bin/python2 -# -# The Qubes OS Project, http://www.qubes-os.org -# -# Copyright (C) 2016 Bahtiar `kalkin-` Gadimov -# -# This program is free software; you can redistribute it and/or modify -# it under the terms of the GNU General Public License as published by -# the Free Software Foundation; either version 2 of the License, or -# (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License along -# with this program; if not, write to the Free Software Foundation, Inc., -# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. -# -''' Manage pools and volumes managed by the 'lvm_thin' driver. ''' - -from __future__ import print_function - -import argparse -import logging -import subprocess -import sys -import time -import lvm # pylint: disable=import-error - -log = logging.getLogger('qubes.storage.lvm') - - -def pool_exists(args): - """ Check if given name is an lvm thin volume. """ - # TODO Implement a faster and proper working version pool_exists - vg_name, thin_pool_name = args.pool_id.split('/', 1) - volume_group = lvm.vgOpen(vg_name) - for p in volume_group.listLVs(): - if p.getAttr()[0] == 't' and p.getName() == thin_pool_name: - volume_group.close() - return True - - volume_group.close() - return False - - -def volume_exists(volume): - """ Check if the given volume exists and is a thin volume """ - log.debug("Checking if the %s thin volume exists", volume) - assert volume is not None - vg_name, volume_name = volume.split('/', 1) - volume_group = lvm.vgOpen(vg_name) - for p in volume_group.listLVs(): - if p.getAttr()[0] == 'V' and p.getName() == volume_name: - volume_group.close() - return True - - volume_group.close() - return False - - -def remove_volume(args): - """ Tries to remove the specified logical volume. - - If the removal fails it will try up to 3 times waiting 1, 2 and 3 - seconds between tries. Most of the time this function fails if some - process still has the volume locked. - """ - img = args.name - if not volume_exists(img): - log.info("Expected to remove %s, but volume does not exist", img) - return - - tries = 1 - successful = False - cmd = ['lvremove', '-f', img] - - while tries <= 3 and not successful: - log.info("Trying to remove LVM %s", img) - try: - output = subprocess.check_output(cmd, stderr=subprocess.STDOUT) - log.debug(output) - successful = True - except subprocess.CalledProcessError: - successful = False - - if successful: - break - else: - time.sleep(tries) - tries += 1 - - if not successful: - log.error('Could not remove volume ' + img) - - -def clone_volume(args): - """ Calls lvcreate and creates new snapshot. """ - old = args.source - new_name = args.destination - cmd = ["lvcreate", "-kn", "-ay", "-s", old, "-n", new_name] - return subprocess.call(cmd) - - -def new_volume(args): - ''' Creates a new volume in the specified thin pool, formated with ext4 ''' - - thin_pool = args.pool_id - name = args.name - size = args.size - log.info('Creating new Thin LVM %s in %s VG %s bytes', name, thin_pool, - size) - cmd = ['lvcreate', '-T', thin_pool, '-kn', '-ay', '-n', name, '-V', - str(size) + 'B'] - - return subprocess.call(cmd) - - -def rename_volume(old_name, new_name): - ''' Rename volume ''' - log.debug("Renaming LVM %s to %s ", old_name, new_name) - retcode = subprocess.call(["lvrename", old_name, new_name]) - if retcode != 0: - raise IOError("Error renaming LVM %s to %s " % (old_name, new_name)) - return new_name - - -def extend_volume(args): - ''' Extends an existing lvm volume. Note this works on any lvm volume not - only on thin volumes. - ''' - vid = args.name - size = int(args.size) / (1000 * 1000) - log.debug("Extending LVM %s to %s", vid, size) - cmd = ["lvextend", "-L%s" % size, vid] - log.debug(cmd) - retcode = subprocess.call(cmd) - if retcode != 0: - raise IOError("Error extending LVM %s to %s " % (vid, size)) - return 0 - - -def init_pool_parser(sub_parsers): - ''' Initialize pool subparser ''' - pool_parser = sub_parsers.add_parser( - 'pool', - help="Exit with exit code 0 if pool exists") - pool_parser.add_argument('pool_id', metavar='VG/POOL', - help="volume_group/pool_name") - pool_parser.set_defaults(func=pool_exists) - - -def init_new_parser(sub_parsers): - ''' Initialize the 'new' subparser ''' - new_parser = sub_parsers.add_parser( - 'create', - help='Creates a new thin ThinPoolLogicalVolume') - new_parser.add_argument('pool_id', metavar='VG/POOL', - help="volume_group/pool_name") - - new_parser.add_argument('name', - help='name of the new ThinPoolLogicalVolume') - new_parser.add_argument( - 'size', help='size in bytes of the new ThinPoolLogicalVolume') - - new_parser.set_defaults(func=new_volume) - - -def init_import_parser(sub_parsers): - ''' Initialize import subparser ''' - import_parser = sub_parsers.add_parser( - 'import', - help='sparse copy data from stdin to a thin volume') - import_parser.add_argument('name', metavar='VG/VID', - help='volume_group/volume_name') - import_parser.set_defaults(func=import_volume) - -def init_clone_parser(sub_parsers): - ''' Initialize clone subparser ''' - clone_parser = sub_parsers.add_parser( - 'clone', - help='sparse copy data from stdin to a thin volume') - clone_parser.add_argument('source', metavar='VG/VID', - help='volume_group/volume_name') - clone_parser.add_argument('destination', metavar='VG/VID', - help='volume_group/volume_name') - clone_parser.set_defaults(func=clone_volume) - -def import_volume(args): - ''' Imports from stdin to a thin volume ''' - name = args.name - src = sys.stdin - blk_size = 4096 - zeros = '\x00' * blk_size - dst_path = '/dev/%s' % name - with open(dst_path, 'wb') as dst: - while True: - tmp = src.read(blk_size) - if not tmp: - break - elif tmp == zeros: - dst.seek(blk_size, 1) - else: - dst.write(tmp) - - -def list_volumes(args): - ''' lists volumes ''' - vg_name, _ = args.name.split('/') - volume_group = lvm.vgOpen(vg_name) - for p in volume_group.listLVs(): - if p.getAttr()[0] == 'V': - print(vg_name + "/" + p.getName() + ' ' + p.getAttr()) - volume_group.close() - - -def init_volumes_parser(sub_parsers): - ''' Initialize volumes subparser ''' - parser = sub_parsers.add_parser('volumes', - help='list volumes in a pool') - parser.add_argument('name', metavar='VG/THIN_POOL', - help='volume_group/thin_pool_name') - parser.set_defaults(func=list_volumes) - - -def init_remove_parser(sub_parsers): - ''' Initialize remove subparser ''' - remove_parser = sub_parsers.add_parser('remove', - help='Removes a LogicalVolume') - remove_parser.add_argument('name', metavar='VG/VID', - help='volume_group/volume_name') - remove_parser.set_defaults(func=remove_volume) - - -def init_extend_parser(sub_parsers): - ''' Initialize extend subparser ''' - extend_parser = sub_parsers.add_parser('extend', - help='extends a LogicalVolume') - extend_parser.add_argument('name', metavar='VG/VID', - help='volume_group/volume_name') - extend_parser.set_defaults(func=extend_volume) - extend_parser.add_argument( - 'size', help='size in bytes of the new ThinPoolLogicalVolume') - - -def get_parser(): - '''Create :py:class:`argparse.ArgumentParser` suitable for - :program:`qubes-lvm`. - ''' - parser = argparse.ArgumentParser(description=__doc__) - # pylint: disable=protected-access - parser.register('action', 'parsers', argparse._SubParsersAction) - sub_parsers = parser.add_subparsers( - title='commands', - description="For more information see qubes-lvm command -h", - dest='command') - init_clone_parser(sub_parsers) - init_extend_parser(sub_parsers) - init_import_parser(sub_parsers) - init_new_parser(sub_parsers) - init_pool_parser(sub_parsers) - init_remove_parser(sub_parsers) - init_volumes_parser(sub_parsers) - - return parser - - -def main(args=None): - '''Main routine of :program:`qubes-lvm`.''' - args = get_parser().parse_args(args) - return args.func(args) - - -if __name__ == '__main__': - sys.exit(main()) diff --git a/rpm_spec/core-dom0.spec b/rpm_spec/core-dom0.spec index ecfd4391..45fd4012 100644 --- a/rpm_spec/core-dom0.spec +++ b/rpm_spec/core-dom0.spec @@ -250,7 +250,6 @@ fi %{python_sitelib}/qubes/tools/qubes_monitor_layout_notify.py* %{python_sitelib}/qubes/tools/qubes_prefs.py* %{python_sitelib}/qubes/tools/qvm_block.py* -%{python_sitelib}/qubes/tools/qubes_lvm.py* %{python_sitelib}/qubes/tools/qvm_backup.py* %{python_sitelib}/qubes/tools/qvm_backup_restore.py* %{python_sitelib}/qubes/tools/qvm_create.py* From 400e92b25a84cd0357ac1ddbfd8636ac5937fd85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Thu, 3 Nov 2016 21:41:48 +0100 Subject: [PATCH 3/9] storage/lvm: misc fixes - add missing lvm remove call when commiting changes - delay creating volatile image until domain startup (it will be created then anyway) - reset cache only when really changed anything - attach VM to the volume (snapshot) created for its runtime - to not expose changes (for example in root volume) to child VMs until shutdown QubesOS/qubes-issues#2412 QubesOS/qubes-issues#2256 --- qubes/storage/lvm.py | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/qubes/storage/lvm.py b/qubes/storage/lvm.py index fd0cbdc2..42c0d9e5 100644 --- a/qubes/storage/lvm.py +++ b/qubes/storage/lvm.py @@ -69,6 +69,7 @@ class ThinPool(qubes.storage.Pool): cmd = ['clone', volume._vid_snap, volume.vid] qubes_lvm(cmd, self.log) cmd = ['remove', volume._vid_snap] + qubes_lvm(cmd, self.log) @property def config(self): @@ -83,8 +84,9 @@ class ThinPool(qubes.storage.Pool): assert volume.vid assert volume.size if volume.source: - return self.clone(volume.source, volume) - else: + # will clone in start() + return volume + elif not volume._is_volatile: cmd = [ 'create', self._pool_id, @@ -92,7 +94,7 @@ class ThinPool(qubes.storage.Pool): str(volume.size) ] qubes_lvm(cmd, self.log) - reset_cache() + reset_cache() return volume def destroy(self): @@ -398,6 +400,16 @@ class ThinVolume(qubes.storage.Volume): raise qubes.storage.StoragePoolException( "You shouldn't use lvm size setter") + def block_device(self): + ''' Return :py:class:`qubes.devices.BlockDevice` for serialization in + the libvirt XML template as . + ''' + if not self._is_volatile: + return qubes.devices.BlockDevice( + '/dev/' + self._vid_snap, self.name, self.script, + self.rw, self.domain, self.devtype) + else: + return super(ThinVolume, self).block_device() @property def usage(self): # lvm thin usage always returns at least the same usage as From 37dbf29bc1678570fa8a578bc77148538942ddcb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Thu, 3 Nov 2016 22:46:57 +0100 Subject: [PATCH 4/9] storage/lvm: don't fail on removing already removed volumes This may happen when removing not fully created VM. QubesOS/qubes-issues#2256 --- qubes/storage/lvm.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/qubes/storage/lvm.py b/qubes/storage/lvm.py index 42c0d9e5..f1c801fc 100644 --- a/qubes/storage/lvm.py +++ b/qubes/storage/lvm.py @@ -160,6 +160,8 @@ class ThinPool(qubes.storage.Pool): 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() From 4323651afb6bbb095a07a75bda7d849f43d2bafd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Fri, 4 Nov 2016 02:03:46 +0100 Subject: [PATCH 5/9] storage/lvm: remove duplicated _reset function There were two: _reset and _reset_volume. Neither of them was working, but the later was closer. Remove the other one. QubesOS/qubes-issues#2256 --- qubes/storage/lvm.py | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/qubes/storage/lvm.py b/qubes/storage/lvm.py index f1c801fc..e075bb19 100644 --- a/qubes/storage/lvm.py +++ b/qubes/storage/lvm.py @@ -218,14 +218,6 @@ class ThinPool(qubes.storage.Pool): qubes_lvm(cmd, self.log) reset_cache() - def _reset(self, volume): - try: - self.remove(volume) - except qubes.storage.StoragePoolException: - pass - - self.create(volume) - def setup(self): pass # TODO Should we create a non existing pool? @@ -233,7 +225,7 @@ class ThinPool(qubes.storage.Pool): if volume._is_snapshot: self._snapshot(volume) elif volume._is_volatile: - self._reset(volume) + self._reset_volume(volume) else: if not self.is_dirty(volume): self._snapshot(volume) @@ -298,11 +290,14 @@ class ThinPool(qubes.storage.Pool): def _reset_volume(self, volume): ''' Resets a volatile volume ''' - assert volume.volume_type == 'volatile', \ + assert volume._is_volatile, \ 'Expected a volatile volume, but got {!r}'.format(volume) self.log.debug('Resetting volatile ' + volume.vid) - cmd = ['remove', volume.vid] - qubes_lvm(cmd, self.log) + try: + cmd = ['remove', volume.vid] + qubes_lvm(cmd, self.log) + except qubes.storage.StoragePoolException: + pass cmd = ['create', self._pool_id, volume.vid.split('/')[1], str(volume.size)] qubes_lvm(cmd, self.log) From ab9d7fbb76ef2d12637af385560138a1eebf3f7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Fri, 4 Nov 2016 12:39:29 +0100 Subject: [PATCH 6/9] storage: improve/fix handling extra volumes Just calling pool.init_volume isn't enough - a lot of code depends on additional data loaded into vm.storage object. Provide a convenient wrapper for this. At the same time, fix loading extra volumes from qubes.xml - don't fail on volume not mentioned in initial vm.volume_config. QubesOS/qubes-issues#2256 --- qubes/storage/__init__.py | 19 ++++++++++++++----- qubes/vm/qubesvm.py | 11 ++++++++--- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/qubes/storage/__init__.py b/qubes/storage/__init__.py index 5f0d64bd..db06a232 100644 --- a/qubes/storage/__init__.py +++ b/qubes/storage/__init__.py @@ -194,14 +194,23 @@ class Storage(object): if hasattr(vm, 'volume_config'): for name, conf in self.vm.volume_config.items(): - assert 'pool' in conf, "Pool missing in volume_config" % str( - conf) if 'volume_type' in conf: conf = self._migrate_config(conf) - pool = self.vm.app.get_pool(conf['pool']) - self.vm.volumes[name] = pool.init_volume(self.vm, conf) - self.pools[name] = pool + self.init_volume(name, conf) + + def init_volume(self, name, volume_config): + ''' Initialize Volume instance attached to this domain ''' + assert 'pool' in volume_config, "Pool missing in volume_config" % str( + volume_config) + + if 'name' not in volume_config: + volume_config['name'] = name + pool = self.vm.app.get_pool(volume_config['pool']) + volume = pool.init_volume(self.vm, volume_config) + self.vm.volumes[name] = volume + self.pools[name] = pool + return volume def _migrate_config(self, conf): ''' Migrates from the old config style to new diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py index 20061bf3..66845f56 100644 --- a/qubes/vm/qubesvm.py +++ b/qubes/vm/qubesvm.py @@ -427,14 +427,19 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM): for key, value in node.items(): # pylint: disable=no-member if value == 'True': - self.volume_config[name][key] = True - else: + value = True + try: self.volume_config[name][key] = value + except KeyError: + self.volume_config[name] = {key: value} for name, conf in volume_config.items(): for key, value in conf.items(): # pylint: disable=no-member - self.volume_config[name][key] = value + try: + self.volume_config[name][key] = value + except KeyError: + self.volume_config[name] = {key: value} elif volume_config: raise TypeError( From 1a7f2892d1d52b89cfa7a335d4a92f59ec3b6130 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Fri, 4 Nov 2016 12:49:07 +0100 Subject: [PATCH 7/9] storage/lvm: fix logic regarding snapshots, start, stop etc There are mutiple cases when snapshots are inconsistently created, for example: - "-back" snapshot created from the "new" data, instead of old one - "-snap" created even when volume.snap_on_start=False - probably more Fix this by following volume.snap_on_start and volume.save_on_stop directly, instead of using abstraction of old volume types. QubesOS/qubes-issues#2256 --- qubes/storage/lvm.py | 58 ++++++++++++++++++++------------------------ 1 file changed, 26 insertions(+), 32 deletions(-) diff --git a/qubes/storage/lvm.py b/qubes/storage/lvm.py index e075bb19..a8bf6432 100644 --- a/qubes/storage/lvm.py +++ b/qubes/storage/lvm.py @@ -59,17 +59,18 @@ class ThinPool(qubes.storage.Pool): assert volume.rw, msg assert hasattr(volume, '_vid_snap') - cmd = ['remove', volume.vid + "-back"] - qubes_lvm(cmd, self.log) - cmd = ['clone', volume._vid_snap, volume.vid + "-back"] + try: + cmd = ['remove', volume.vid + "-back"] + qubes_lvm(cmd, self.log) + except qubes.storage.StoragePoolException: + pass + cmd = ['clone', volume.vid, volume.vid + "-back"] qubes_lvm(cmd, self.log) cmd = ['remove', volume.vid] qubes_lvm(cmd, self.log) cmd = ['clone', volume._vid_snap, volume.vid] qubes_lvm(cmd, self.log) - cmd = ['remove', volume._vid_snap] - qubes_lvm(cmd, self.log) @property def config(self): @@ -83,16 +84,16 @@ class ThinPool(qubes.storage.Pool): def create(self, volume): assert volume.vid assert volume.size - if volume.source: - # will clone in start() - return volume - elif not volume._is_volatile: - cmd = [ - 'create', - self._pool_id, - volume.vid.split('/', 1)[1], - str(volume.size) - ] + if volume.save_on_stop: + if volume.source: + cmd = ['clone', str(volume.source), volume.vid] + else: + cmd = [ + 'create', + self._pool_id, + volume.vid.split('/', 1)[1], + str(volume.size) + ] qubes_lvm(cmd, self.log) reset_cache() return volume @@ -173,14 +174,12 @@ class ThinPool(qubes.storage.Pool): if volume.save_on_stop: cmd = ['clone', volume.vid, new_vid] qubes_lvm(cmd, self.log) - - if volume.save_on_stop or volume._is_volatile: cmd = ['remove', volume.vid] qubes_lvm(cmd, self.log) volume.vid = new_vid - if not volume._is_volatile: + if volume.snap_on_start: volume._vid_snap = volume.vid + '-snap' reset_cache() return volume @@ -222,29 +221,24 @@ class ThinPool(qubes.storage.Pool): pass # TODO Should we create a non existing pool? def start(self, volume): - if volume._is_snapshot: - self._snapshot(volume) - elif volume._is_volatile: - self._reset_volume(volume) - else: - if not self.is_dirty(volume): + if volume.snap_on_start: + if not volume.save_on_stop or not self.is_dirty(volume): self._snapshot(volume) + elif not volume.save_on_stop: + self._reset_volume(volume) reset_cache() return volume def stop(self, volume): - if volume.save_on_stop: + if volume.save_on_stop and volume.snap_on_start: self._commit(volume) - if volume._is_snapshot: + if volume.snap_on_start: cmd = ['remove', volume._vid_snap] qubes_lvm(cmd, self.log) - elif volume._is_volatile: + elif not volume.save_on_stop: cmd = ['remove', volume.vid] qubes_lvm(cmd, self.log) - else: - cmd = ['remove', volume._vid_snap] - qubes_lvm(cmd, self.log) reset_cache() return volume @@ -355,7 +349,7 @@ class ThinVolume(qubes.storage.Volume): raise qubes.storage.StoragePoolException(msg) self.path = '/dev/' + self.vid - if not self._is_volatile: + if self.snap_on_start: self._vid_snap = self.vid + '-snap' self._size = size @@ -401,7 +395,7 @@ class ThinVolume(qubes.storage.Volume): ''' Return :py:class:`qubes.devices.BlockDevice` for serialization in the libvirt XML template as . ''' - if not self._is_volatile: + if self.snap_on_start: return qubes.devices.BlockDevice( '/dev/' + self._vid_snap, self.name, self.script, self.rw, self.domain, self.devtype) From b59463e8e89d4cf06dc67956083b7ddb026c8f56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Fri, 4 Nov 2016 13:19:49 +0100 Subject: [PATCH 8/9] qvm-block: fix listing non-internal volumes In case of LVM (at least), "internal" flag is initialized only when listing volume attached to given VM, but not when listing them from the pool. This looks like a limitation (bug?) of pool driver, it looks like much nicer fix is to handle the flag in qvm-block tool (which list VMs volumes anyway), than in LVM storage pool driver (which would need to keep second copy of volumes list - just like file driver). QubesOS/qubes-issues#2256 --- qubes/storage/lvm.py | 3 +++ qubes/tools/qvm_block.py | 11 +++++++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/qubes/storage/lvm.py b/qubes/storage/lvm.py index a8bf6432..933d62f9 100644 --- a/qubes/storage/lvm.py +++ b/qubes/storage/lvm.py @@ -272,6 +272,9 @@ class ThinPool(qubes.storage.Pool): continue if vol_info['pool_lv'] != self.thin_pool: continue + if vid.endswith('-snap'): + # implementation detail volume + continue config = { 'pool': self.name, 'vid': vid, diff --git a/qubes/tools/qvm_block.py b/qubes/tools/qvm_block.py index 9385a009..92019f46 100644 --- a/qubes/tools/qvm_block.py +++ b/qubes/tools/qvm_block.py @@ -115,11 +115,14 @@ def list_volumes(args): for domain in domains: # gather the domain names try: for volume in domain.attached_volumes: - if not args.internal and volume.internal: - continue try: - volume_data = vd_dict[volume.pool][volume.vid] - volume_data.domains += [(domain.name, volume.name)] + if not args.internal and volume.internal: + # some pools (LVM) may set 'internal' flag only when + # listing volumes of specific domain + del vd_dict[volume.pool][volume.vid] + else: + volume_data = vd_dict[volume.pool][volume.vid] + volume_data.domains += [(domain.name, volume.name)] except KeyError: # Skipping volume continue From b011cef8af399d29e6e8e3200fada129c60145d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Fri, 4 Nov 2016 13:30:51 +0100 Subject: [PATCH 9/9] tests/storage: add tests for basic volumes properties Things like if read-only volume is really read-only, volatile is volatile etc. QubesOS/qubes-issues#2256 --- qubes/tests/__init__.py | 1 + qubes/tests/int/storage.py | 256 +++++++++++++++++++++++++++++++++++++ rpm_spec/core-dom0.spec | 1 + 3 files changed, 258 insertions(+) create mode 100644 qubes/tests/int/storage.py diff --git a/qubes/tests/__init__.py b/qubes/tests/__init__.py index c1f932b8..04786f87 100644 --- a/qubes/tests/__init__.py +++ b/qubes/tests/__init__.py @@ -825,6 +825,7 @@ def load_tests(loader, tests, pattern): # pylint: disable=unused-argument for modname in ( # integration tests 'qubes.tests.int.basic', + 'qubes.tests.int.storage', 'qubes.tests.int.devices_pci', 'qubes.tests.int.dom0_update', 'qubes.tests.int.network', diff --git a/qubes/tests/int/storage.py b/qubes/tests/int/storage.py new file mode 100644 index 00000000..181d9438 --- /dev/null +++ b/qubes/tests/int/storage.py @@ -0,0 +1,256 @@ +#!/usr/bin/python2 +# -*- encoding: utf8 -*- +# +# The Qubes OS Project, http://www.qubes-os.org +# +# Copyright (C) 2016 Marek Marczykowski-Górecki +# +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License along +# with this program; if not, write to the Free Software Foundation, Inc., +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +import os + +import shutil + +import qubes.storage.lvm +import qubes.tests +import qubes.tests.storage_lvm +import qubes.vm.appvm + + +class StorageTestMixin(qubes.tests.SystemTestsMixin): + def setUp(self): + super(StorageTestMixin, self).setUp() + self.init_default_template() + self.vm1 = self.app.add_new_vm(qubes.vm.appvm.AppVM, + name=self.make_vm_name('vm1'), + label='red') + self.vm1.create_on_disk() + self.vm2 = self.app.add_new_vm(qubes.vm.appvm.AppVM, + name=self.make_vm_name('vm2'), + label='red') + self.vm2.create_on_disk() + self.pool = None + self.init_pool() + self.app.save() + + def init_pool(self): + ''' Initialize storage pool to be tested, store it in self.pool''' + raise NotImplementedError + + def test_000_volatile(self): + '''Test if volatile volume is really volatile''' + size = 32*1024*1024 + volume_config = { + 'pool': self.pool.name, + 'size': size, + 'internal': False, + 'save_on_stop': False, + 'rw': True, + } + testvol = self.vm1.storage.init_volume('testvol', volume_config) + self.vm1.storage.get_pool(testvol).create(testvol) + self.app.save() + self.vm1.start() + p = self.vm1.run( + 'head -c {} /dev/zero 2>&1 | diff -q /dev/xvde - 2>&1'.format(size), + user='root', passio_popen=True) + stdout, _ = p.communicate() + self.assertEqual(p.returncode, 0, + 'volatile image not clean: {}'.format(stdout)) + self.vm1.run('echo test123 > /dev/xvde', user='root', wait=True) + self.vm1.shutdown(wait=True) + self.vm1.start() + p = self.vm1.run( + 'head -c {} /dev/zero 2>&1 | diff -q /dev/xvde - 2>&1'.format(size), + user='root', passio_popen=True) + stdout, _ = p.communicate() + self.assertEqual(p.returncode, 0, + 'volatile image not volatile: {}'.format(stdout)) + + def test_001_non_volatile(self): + '''Test if non-volatile volume is really non-volatile''' + size = 32*1024*1024 + volume_config = { + 'pool': self.pool.name, + 'size': size, + 'internal': False, + 'save_on_stop': True, + 'rw': True, + } + testvol = self.vm1.storage.init_volume('testvol', volume_config) + self.vm1.storage.get_pool(testvol).create(testvol) + self.app.save() + self.vm1.start() + p = self.vm1.run( + 'head -c {} /dev/zero 2>&1 | diff -q /dev/xvde - 2>&1'.format(size), + user='root', passio_popen=True) + stdout, _ = p.communicate() + self.assertEqual(p.returncode, 0, + 'non-volatile image not clean: {}'.format(stdout)) + self.vm1.run('echo test123 > /dev/xvde', user='root', wait=True) + self.vm1.shutdown(wait=True) + self.vm1.start() + p = self.vm1.run( + 'head -c {} /dev/zero 2>&1 | diff -q /dev/xvde - 2>&1'.format(size), + user='root', passio_popen=True) + stdout, _ = p.communicate() + self.assertNotEqual(p.returncode, 0, + 'non-volatile image volatile: {}'.format(stdout)) + + def test_002_read_only(self): + '''Test read-only volume''' + size = 32 * 1024 * 1024 + volume_config = { + 'pool': self.pool.name, + 'size': size, + 'internal': False, + 'save_on_stop': True, + 'rw': False, + } + testvol = self.vm1.storage.init_volume('testvol', volume_config) + self.vm1.storage.get_pool(testvol).create(testvol) + self.app.save() + self.vm1.start() + p = self.vm1.run( + 'head -c {} /dev/zero 2>&1 | diff -q /dev/xvde - 2>&1'.format(size), + user='root', passio_popen=True) + stdout, _ = p.communicate() + self.assertEqual(p.returncode, 0, + 'non-volatile image not clean: {}'.format(stdout)) + p = self.vm1.run('echo test123 > /dev/xvde', user='root', + passio_popen=True) + p.wait() + self.assertNotEqual(p.returncode, 0, + 'Write to read-only volume unexpectedly succeeded') + p = self.vm1.run( + 'head -c {} /dev/zero 2>&1 | diff -q /dev/xvde - 2>&1'.format(size), + user='root', passio_popen=True) + stdout, _ = p.communicate() + self.assertEqual(p.returncode, 0, + 'read-only volume modified: {}'.format(stdout)) + + def test_003_snapshot(self): + '''Test snapshot volume data propagation''' + size = 128 * 1024 * 1024 + volume_config = { + 'pool': self.pool.name, + 'size': size, + 'internal': False, + 'save_on_stop': True, + 'rw': True, + } + testvol = self.vm1.storage.init_volume('testvol', volume_config) + self.vm1.storage.get_pool(testvol).create(testvol) + volume_config = { + 'pool': self.pool.name, + 'size': size, + 'internal': False, + 'snap_on_start': True, + 'source': testvol.vid, + 'rw': True, + } + testvol_snap = self.vm2.storage.init_volume('testvol', volume_config) + self.vm2.storage.get_pool(testvol_snap).create(testvol_snap) + self.app.save() + self.vm1.start() + self.vm2.start() + p = self.vm1.run( + 'head -c {} /dev/zero 2>&1 | diff -q /dev/xvde - 2>&1'.format(size), + user='root', passio_popen=True) + stdout, _ = p.communicate() + self.assertEqual(p.returncode, 0, + 'origin image not clean: {}'.format(stdout)) + + p = self.vm2.run( + 'head -c {} /dev/zero | diff -q /dev/xvde -'.format(size), + user='root', passio_popen=True) + stdout, _ = p.communicate() + self.assertEqual(p.returncode, 0, + 'snapshot image not clean: {}'.format(stdout)) + + self.vm1.run('echo test123 > /dev/xvde && sync', user='root', wait=True) + p.wait() + self.assertEqual(p.returncode, 0, + 'Write to read-write volume failed') + p = self.vm2.run( + 'head -c {} /dev/zero 2>&1 | diff -q /dev/xvde - 2>&1'.format(size), + user='root', passio_popen=True) + stdout, _ = p.communicate() + self.assertEqual(p.returncode, 0, + 'origin changes propagated to snapshot too early: {}'.format( + stdout)) + self.vm1.shutdown(wait=True) + # after origin shutdown there should be still no change + p = self.vm2.run( + 'head -c {} /dev/zero 2>&1 | diff -q /dev/xvde - 2>&1'.format(size), + user='root', passio_popen=True) + stdout, _ = p.communicate() + self.assertEqual(p.returncode, 0, + 'origin changes propagated to snapshot too early2: {}'.format( + stdout)) + + self.vm2.shutdown(wait=True) + self.vm2.start() + # only after target VM restart changes should be visible + p = self.vm2.run( + 'head -c {} /dev/zero 2>&1 | diff -q /dev/xvde - 2>&1'.format(size), + user='root', passio_popen=True) + stdout, _ = p.communicate() + self.assertNotEqual(p.returncode, 0, + 'origin changes not visible in snapshot: {}'.format(stdout)) + + +class StorageFile(StorageTestMixin, qubes.tests.QubesTestCase): + def init_pool(self): + self.dir_path = '/var/tmp/test-pool' + self.pool = self.app.add_pool(dir_path=self.dir_path, + name='test-pool', driver='file') + os.mkdir(os.path.join(self.dir_path, 'appvms', self.vm1.name)) + os.mkdir(os.path.join(self.dir_path, 'appvms', self.vm2.name)) + + def tearDown(self): + self.app.remove_pool('test-pool') + shutil.rmtree(self.dir_path) + super(StorageFile, self).tearDown() + + +@qubes.tests.storage_lvm.skipUnlessLvmPoolExists +class StorageLVM(StorageTestMixin, qubes.tests.QubesTestCase): + def init_pool(self): + # check if the default LVM Thin pool qubes_dom0/pool00 exists + volume_group, thin_pool = \ + qubes.tests.storage_lvm.DEFAULT_LVM_POOL.split('/', 1) + self.pool = self._find_pool(volume_group, thin_pool) + if not self.pool: + self.pool = self.app.add_pool(**qubes.tests.storage_lvm.POOL_CONF) + self.created_pool = True + + def tearDown(self): + ''' Remove the default lvm pool if it was created only for this test ''' + if self.created_pool: + self.app.remove_pool(self.pool.name) + super(StorageLVM, self).tearDown() + + def _find_pool(self, volume_group, thin_pool): + ''' Returns the pool matching the specified ``volume_group`` & + ``thin_pool``, or None. + ''' + pools = [p for p in self.app.pools + if issubclass(p.__class__, qubes.storage.lvm.ThinPool)] + for pool in pools: + if pool.volume_group == volume_group \ + and pool.thin_pool == thin_pool: + return pool + return None diff --git a/rpm_spec/core-dom0.spec b/rpm_spec/core-dom0.spec index 45fd4012..44d9d739 100644 --- a/rpm_spec/core-dom0.spec +++ b/rpm_spec/core-dom0.spec @@ -318,6 +318,7 @@ fi %{python_sitelib}/qubes/tests/int/dispvm.py* %{python_sitelib}/qubes/tests/int/dom0_update.py* %{python_sitelib}/qubes/tests/int/network.py* +%{python_sitelib}/qubes/tests/int/storage.py* %{python_sitelib}/qubes/tests/int/vm_qrexec_gui.py* %dir %{python_sitelib}/qubes/tests/int/tools