From e76372b9349e3f835dc1c11430f85d3baa010d61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Sun, 29 Oct 2017 02:23:00 +0200 Subject: [PATCH 1/7] storage: add size and usage properties to pool object Add Pool.size and Pool.usage to the API. Implement them for LVM and File pools. Add appropriate tests. QubesOS/qubes-issues#3240 --- qubes/storage/__init__.py | 10 ++++++++++ qubes/storage/file.py | 10 ++++++++++ qubes/storage/lvm.py | 18 +++++++++++++++++- qubes/tests/storage_file.py | 15 +++++++++++++++ qubes/tests/storage_lvm.py | 21 +++++++++++++++++++++ 5 files changed, 73 insertions(+), 1 deletion(-) diff --git a/qubes/storage/__init__.py b/qubes/storage/__init__.py index 9dcd7c2a..e5997e7d 100644 --- a/qubes/storage/__init__.py +++ b/qubes/storage/__init__.py @@ -815,6 +815,16 @@ class Pool(object): ''' raise self._not_implemented("get_volume") + @property + def size(self): + ''' Storage pool size in bytes ''' + raise self._not_implemented("size") + + @property + def usage(self): + ''' Space used in the pool, in bytes ''' + raise self._not_implemented("usage") + def _not_implemented(self, method_name): ''' Helper for emitting helpful `NotImplementedError` exceptions ''' msg = "Pool driver {!s} has {!s}() not implemented" diff --git a/qubes/storage/file.py b/qubes/storage/file.py index 8969d591..0dfb8b8f 100644 --- a/qubes/storage/file.py +++ b/qubes/storage/file.py @@ -144,6 +144,16 @@ class FilePool(qubes.storage.Pool): def list_volumes(self): return self._volumes + @property + def size(self): + statvfs = os.statvfs(self.dir_path) + return statvfs.f_frsize * statvfs.f_blocks + + @property + def usage(self): + statvfs = os.statvfs(self.dir_path) + return statvfs.f_frsize * (statvfs.f_blocks - statvfs.f_bfree) + class FileVolume(qubes.storage.Volume): ''' Parent class for the xen volumes implementation which expects a diff --git a/qubes/storage/lvm.py b/qubes/storage/lvm.py index 107e7e37..8563416e 100644 --- a/qubes/storage/lvm.py +++ b/qubes/storage/lvm.py @@ -137,6 +137,22 @@ class ThinPool(qubes.storage.Pool): volumes += [ThinVolume(**config)] return volumes + @property + def size(self): + try: + return qubes.storage.lvm.size_cache[ + self.volume_group + '/' + self.thin_pool]['size'] + except KeyError: + return 0 + + @property + def usage(self): + try: + return qubes.storage.lvm.size_cache[ + self.volume_group + '/' + self.thin_pool]['usage'] + except KeyError: + return 0 + def init_cache(log=logging.getLogger('qubes.storage.lvm')): cmd = ['lvs', '--noheadings', '-o', @@ -159,7 +175,7 @@ def init_cache(log=logging.getLogger('qubes.storage.lvm')): line = line.decode().strip() pool_name, pool_lv, name, size, usage_percent, attr, \ origin = line.split(';', 6) - if '' in [pool_name, pool_lv, name, size, usage_percent]: + if '' in [pool_name, name, size, usage_percent]: continue name = pool_name + "/" + name size = int(size[:-1]) # Remove 'B' suffix diff --git a/qubes/tests/storage_file.py b/qubes/tests/storage_file.py index 19d46e11..94d5d679 100644 --- a/qubes/tests/storage_file.py +++ b/qubes/tests/storage_file.py @@ -369,6 +369,21 @@ class TC_03_FilePool(qubes.tests.QubesTestCase): shutil.rmtree(pool_dir, ignore_errors=True) + def test_003_size(self): + pool = self.app.get_pool(self.POOL_NAME) + with self.assertNotRaises(NotImplementedError): + size = pool.size + statvfs = os.statvfs(self.POOL_DIR) + self.assertEqual(size, statvfs.f_blocks * statvfs.f_frsize) + + def test_004_usage(self): + pool = self.app.get_pool(self.POOL_NAME) + with self.assertNotRaises(NotImplementedError): + usage = pool.usage + statvfs = os.statvfs(self.POOL_DIR) + self.assertEqual(usage, + statvfs.f_frsize * (statvfs.f_blocks - statvfs.f_bfree)) + def test_011_appvm_file_images(self): """ Check if all the needed image files are created for an AppVm""" diff --git a/qubes/tests/storage_lvm.py b/qubes/tests/storage_lvm.py index 18662b95..f6a4be69 100644 --- a/qubes/tests/storage_lvm.py +++ b/qubes/tests/storage_lvm.py @@ -26,6 +26,7 @@ ''' import os +import subprocess import unittest import qubes.tests @@ -158,6 +159,26 @@ class TC_00_ThinPool(ThinPoolBase): self.assertTrue(os.path.exists(path)) volume.remove() + def test_004_size(self): + with self.assertNotRaises(NotImplementedError): + size = self.pool.size + pool_size = subprocess.check_output(['sudo', 'lvs', '--noheadings', + '-o', 'lv_size', + '--units', 'b', self.pool.volume_group + '/' + self.pool.thin_pool]) + self.assertEqual(size, int(pool_size.strip()[:-1])) + + def test_005_usage(self): + with self.assertNotRaises(NotImplementedError): + usage = self.pool.usage + pool_info = subprocess.check_output(['sudo', 'lvs', '--noheadings', + '-o', 'lv_size,data_percent', + '--units', 'b', self.pool.volume_group + '/' + self.pool.thin_pool]) + pool_size, pool_usage = pool_info.strip().split() + pool_size = int(pool_size[:-1]) + pool_usage = float(pool_usage) + self.assertEqual(usage, int(pool_size * pool_usage / 100)) + + @skipUnlessLvmPoolExists class TC_01_ThinPool(ThinPoolBase, qubes.tests.SystemTestCase): ''' Sanity tests for :py:class:`qubes.storage.lvm.ThinPool` ''' From f18f4c9bff57b74ec444086ae599d8555d2860cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Sun, 29 Oct 2017 02:44:02 +0200 Subject: [PATCH 2/7] api/admin: add pool size and usage to admin.pool.Info response QubesOS/qubes-issues#3240 --- qubes/api/admin.py | 13 ++++++++++++- qubes/tests/api_admin.py | 22 ++++++++++++++++++++-- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/qubes/api/admin.py b/qubes/api/admin.py index 25a542fd..9d88ffa6 100644 --- a/qubes/api/admin.py +++ b/qubes/api/admin.py @@ -559,8 +559,19 @@ class QubesAdminAPI(qubes.api.AbstractQubesAPI): self.fire_event_for_permission(pool=pool) + size_info = '' + try: + size_info += 'size={}\n'.format(pool.size) + except NotImplementedError: + pass + try: + size_info += 'usage={}\n'.format(pool.usage) + except NotImplementedError: + pass + return ''.join('{}={}\n'.format(prop, val) - for prop, val in sorted(pool.config.items())) + for prop, val in sorted(pool.config.items())) + \ + size_info @qubes.api.method('admin.pool.Add', scope='global', write=True) diff --git a/qubes/tests/api_admin.py b/qubes/tests/api_admin.py index a3c4b037..ed737ed6 100644 --- a/qubes/tests/api_admin.py +++ b/qubes/tests/api_admin.py @@ -545,11 +545,29 @@ class TC_00_VMs(AdminAPITestCase): def test_150_pool_info(self): self.app.pools = { 'pool1': unittest.mock.Mock(config={ - 'param1': 'value1', 'param2': 'value2'}) + 'param1': 'value1', 'param2': 'value2'}, + usage=102400, + size=204800) } value = self.call_mgmt_func(b'admin.pool.Info', b'dom0', b'pool1') - self.assertEqual(value, 'param1=value1\nparam2=value2\n') + self.assertEqual(value, + 'param1=value1\nparam2=value2\nsize=204800\nusage=102400\n') + self.assertFalse(self.app.save.called) + + def test_151_pool_info_unsupported_size(self): + self.app.pools = { + 'pool1': unittest.mock.Mock(config={ + 'param1': 'value1', 'param2': 'value2'}) + } + type(self.app.pools['pool1']).size = unittest.mock.PropertyMock( + side_effect=NotImplementedError) + type(self.app.pools['pool1']).usage = unittest.mock.PropertyMock( + side_effect=NotImplementedError) + value = self.call_mgmt_func(b'admin.pool.Info', b'dom0', b'pool1') + + self.assertEqual(value, + 'param1=value1\nparam2=value2\n') self.assertFalse(self.app.save.called) @unittest.mock.patch('qubes.storage.pool_drivers') From f3455b5d99cc79c6baa9c6032c8b729b76b1a9a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Sun, 29 Oct 2017 17:35:59 +0100 Subject: [PATCH 3/7] storage/file: fix preserving spareness on volume clone Force creating sparse file, even if source volume is not such file (for example block device). Reported by @na-- QubesOS/qubes-issues#3255 --- qubes/storage/file.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qubes/storage/file.py b/qubes/storage/file.py index 0dfb8b8f..0493aaf9 100644 --- a/qubes/storage/file.py +++ b/qubes/storage/file.py @@ -429,7 +429,7 @@ def copy_file(source, destination): os.makedirs(parent_dir) try: - cmd = ['cp', '--sparse=auto', + cmd = ['cp', '--sparse=always', '--reflink=auto', source, destination] subprocess.check_call(cmd) except subprocess.CalledProcessError: From 439d9b87ff02f9a2f3aa77ad7e96d65cb24b1406 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Sun, 5 Nov 2017 18:02:13 +0100 Subject: [PATCH 4/7] storage/lvm: fix importing different-sized volume from another pool Fixes QubesOS/qubes-issues#3257 --- qubes/storage/lvm.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/qubes/storage/lvm.py b/qubes/storage/lvm.py index 8563416e..59d3ea2a 100644 --- a/qubes/storage/lvm.py +++ b/qubes/storage/lvm.py @@ -358,6 +358,8 @@ class ThinVolume(qubes.storage.Volume): cmd = ['clone', str(src_volume), str(self)] qubes_lvm(cmd, self.log) else: + if src_volume.size != self.size: + self.resize(src_volume.size) src_path = src_volume.export() cmd = ['dd', 'if=' + src_path, 'of=/dev/' + self.vid, 'conv=sparse'] From 0327b6cd9814e80662ba5a44804b097bb0f0ff05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Mon, 6 Nov 2017 15:57:20 +0100 Subject: [PATCH 5/7] api/admin: hide dd statistics in admin.vm.volume.Import call But still show errors, if occurs. --- qubes-rpc/admin.vm.volume.Import | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qubes-rpc/admin.vm.volume.Import b/qubes-rpc/admin.vm.volume.Import index 50a75e30..1903387e 100755 --- a/qubes-rpc/admin.vm.volume.Import +++ b/qubes-rpc/admin.vm.volume.Import @@ -44,7 +44,7 @@ path=$(tail -c +3 "$tmpfile"|cut -d ' ' -f 2) # now process stdin into this path if dd bs=4k of="$path" count="$size" iflag=count_bytes,fullblock \ - conv=sparse,notrunc,nocreat,fdatasync; then + conv=sparse,notrunc,nocreat,fdatasync status=none; then status="ok" else status="fail" From 81f455e15ddba07d667a5b76e0269b2a019d9393 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Mon, 6 Nov 2017 20:52:12 +0100 Subject: [PATCH 6/7] storage/file: move revisions_to_keep restrictions to property setter Do not check for accepted value only in constructor, do that in property setter. This will allow enforcing the limit regardless of how the value was set. This is preparation for dynamic revisions_to_keep change. QubesOS/qubes-issues#3256 --- qubes/storage/file.py | 33 +++++++++++++++++++++++++++------ qubes/tests/storage_file.py | 23 +++++++++++++++++++++++ 2 files changed, 50 insertions(+), 6 deletions(-) diff --git a/qubes/storage/file.py b/qubes/storage/file.py index 0493aaf9..cb5b0b35 100644 --- a/qubes/storage/file.py +++ b/qubes/storage/file.py @@ -54,6 +54,7 @@ class FilePool(qubes.storage.Pool): driver = 'file' def __init__(self, revisions_to_keep=1, dir_path=None, **kwargs): + self._revisions_to_keep = 0 super(FilePool, self).__init__(revisions_to_keep=revisions_to_keep, **kwargs) assert dir_path, "No pool dir_path specified" @@ -85,19 +86,27 @@ class FilePool(qubes.storage.Pool): volume_config['revisions_to_keep'] = 0 except KeyError: pass - finally: - if 'revisions_to_keep' not in volume_config: - volume_config['revisions_to_keep'] = self.revisions_to_keep - if int(volume_config['revisions_to_keep']) > 1: - raise NotImplementedError( - 'FilePool supports maximum 1 volume revision to keep') + if 'revisions_to_keep' not in volume_config: + volume_config['revisions_to_keep'] = self.revisions_to_keep volume_config['pool'] = self volume = FileVolume(**volume_config) self._volumes += [volume] return volume + @property + def revisions_to_keep(self): + return self._revisions_to_keep + + @revisions_to_keep.setter + def revisions_to_keep(self, value): + value = int(value) + if value > 1: + raise NotImplementedError( + 'FilePool supports maximum 1 volume revision to keep') + self._revisions_to_keep = value + def destroy(self): pass @@ -162,12 +171,24 @@ class FileVolume(qubes.storage.Volume): def __init__(self, dir_path, **kwargs): self.dir_path = dir_path assert self.dir_path, "dir_path not specified" + self._revisions_to_keep = 0 super(FileVolume, self).__init__(**kwargs) if self.snap_on_start: img_name = self.source.vid + '-cow.img' self.path_source_cow = os.path.join(self.dir_path, img_name) + @property + def revisions_to_keep(self): + return self._revisions_to_keep + + @revisions_to_keep.setter + def revisions_to_keep(self, value): + if int(value) > 1: + raise NotImplementedError( + 'FileVolume supports maximum 1 volume revision to keep') + self._revisions_to_keep = int(value) + def create(self): assert isinstance(self.size, int) and self.size > 0, \ 'Volume size must be > 0' diff --git a/qubes/tests/storage_file.py b/qubes/tests/storage_file.py index 94d5d679..496c45c5 100644 --- a/qubes/tests/storage_file.py +++ b/qubes/tests/storage_file.py @@ -296,6 +296,22 @@ class TC_01_FileVolumes(qubes.tests.QubesTestCase): expected = vm_dir + '/volatile.img' self.assertVolumePath(vm, 'volatile', expected, rw=True) + def test_010_revisions_to_keep_reject_invalid(self): + ''' Check if TemplateVM volumes are propertly initialized ''' + config = { + 'name': 'root', + 'pool': self.POOL_NAME, + 'save_on_stop': True, + 'rw': True, + 'size': defaults['root_img_size'], + } + vm = qubes.tests.storage.TestVM(self) + volume = self.app.get_pool(self.POOL_NAME).init_volume(vm, config) + self.assertEqual(volume.revisions_to_keep, 1) + with self.assertRaises((NotImplementedError, ValueError)): + volume.revisions_to_keep = 2 + self.assertEqual(volume.revisions_to_keep, 1) + def assertVolumePath(self, vm, dev_name, expected, rw=True): # :pylint: disable=invalid-name volumes = vm.volumes @@ -384,6 +400,13 @@ class TC_03_FilePool(qubes.tests.QubesTestCase): self.assertEqual(usage, statvfs.f_frsize * (statvfs.f_blocks - statvfs.f_bfree)) + def test_005_revisions_to_keep(self): + pool = self.app.get_pool(self.POOL_NAME) + self.assertEqual(pool.revisions_to_keep, 1) + with self.assertRaises((NotImplementedError, ValueError)): + pool.revisions_to_keep = 2 + self.assertEqual(pool.revisions_to_keep, 1) + def test_011_appvm_file_images(self): """ Check if all the needed image files are created for an AppVm""" From c3afdde3ef94e0a0b527d2e88da17d9d3aa3a7be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Tue, 7 Nov 2017 02:34:43 +0100 Subject: [PATCH 7/7] api/admin: add API for changing revisions_to_keep dynamically This one pool/volume property makes sense to change dynamically. There may be more such properties, but lets be on the safe side and take whitelist approach - allow only selected (just one for now), instead of blacklisting any harmful ones. QubesOS/qubes-issues#3256 --- Makefile | 3 ++ qubes/api/admin.py | 40 +++++++++++++++++++++++++ qubes/tests/api_admin.py | 63 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 106 insertions(+) diff --git a/Makefile b/Makefile index 82ea9c2b..066c0044 100644 --- a/Makefile +++ b/Makefile @@ -23,11 +23,13 @@ ADMIN_API_METHODS_SIMPLE = \ admin.pool.List \ admin.pool.ListDrivers \ admin.pool.Remove \ + admin.pool.Set.revisions_to_keep \ admin.pool.volume.Info \ admin.pool.volume.List \ admin.pool.volume.ListSnapshots \ admin.pool.volume.Resize \ admin.pool.volume.Revert \ + admin.pool.volume.Set.revisions_to_keep \ admin.pool.volume.Snapshot \ admin.property.Get \ admin.property.GetDefault \ @@ -96,6 +98,7 @@ ADMIN_API_METHODS_SIMPLE = \ admin.vm.volume.ListSnapshots \ admin.vm.volume.Resize \ admin.vm.volume.Revert \ + admin.vm.volume.Set.revisions_to_keep \ admin.vm.Stats \ $(null) diff --git a/qubes/api/admin.py b/qubes/api/admin.py index 9d88ffa6..a8ffbba9 100644 --- a/qubes/api/admin.py +++ b/qubes/api/admin.py @@ -476,6 +476,25 @@ class QubesAdminAPI(qubes.api.AbstractQubesAPI): return '{} {}'.format(size, path) + @qubes.api.method('admin.vm.volume.Set.revisions_to_keep', + scope='local', write=True) + @asyncio.coroutine + def vm_volume_set_revisions_to_keep(self, untrusted_payload): + assert self.arg in self.dest.volumes.keys() + try: + untrusted_value = int(untrusted_payload.decode('ascii')) + except (UnicodeDecodeError, ValueError): + raise qubes.api.ProtocolError('Invalid value') + del untrusted_payload + assert untrusted_value >= 0 + newvalue = untrusted_value + del untrusted_value + + self.fire_event_for_permission(newvalue=newvalue) + + self.dest.volumes[self.arg].revisions_to_keep = newvalue + self.app.save() + @qubes.api.method('admin.vm.tag.List', no_payload=True, scope='local', read=True) @asyncio.coroutine @@ -621,6 +640,27 @@ class QubesAdminAPI(qubes.api.AbstractQubesAPI): self.app.remove_pool(self.arg) self.app.save() + @qubes.api.method('admin.pool.Set.revisions_to_keep', + scope='global', write=True) + @asyncio.coroutine + def pool_set_revisions_to_keep(self, untrusted_payload): + assert self.dest.name == 'dom0' + assert self.arg in self.app.pools.keys() + pool = self.app.pools[self.arg] + try: + untrusted_value = int(untrusted_payload.decode('ascii')) + except (UnicodeDecodeError, ValueError): + raise qubes.api.ProtocolError('Invalid value') + del untrusted_payload + assert untrusted_value >= 0 + newvalue = untrusted_value + del untrusted_value + + self.fire_event_for_permission(newvalue=newvalue) + + pool.revisions_to_keep = newvalue + self.app.save() + @qubes.api.method('admin.label.List', no_payload=True, scope='global', read=True) @asyncio.coroutine diff --git a/qubes/tests/api_admin.py b/qubes/tests/api_admin.py index ed737ed6..0bee0dff 100644 --- a/qubes/tests/api_admin.py +++ b/qubes/tests/api_admin.py @@ -2275,6 +2275,69 @@ class TC_00_VMs(AdminAPITestCase): self.assertNotIn(dev, self.vm.devices['testclass'].persistent()) self.assertFalse(self.app.save.called) + def test_660_pool_set_revisions_to_keep(self): + self.app.pools['test-pool'] = unittest.mock.Mock() + value = self.call_mgmt_func(b'admin.pool.Set.revisions_to_keep', + b'dom0', b'test-pool', b'2') + self.assertIsNone(value) + self.assertEqual(self.app.pools['test-pool'].mock_calls, []) + self.assertEqual(self.app.pools['test-pool'].revisions_to_keep, 2) + self.app.save.assert_called_once_with() + + def test_661_pool_set_revisions_to_keep_negative(self): + self.app.pools['test-pool'] = unittest.mock.Mock() + with self.assertRaises(AssertionError): + self.call_mgmt_func(b'admin.pool.Set.revisions_to_keep', + b'dom0', b'test-pool', b'-2') + self.assertEqual(self.app.pools['test-pool'].mock_calls, []) + self.assertFalse(self.app.save.called) + + def test_662_pool_set_revisions_to_keep_not_a_number(self): + self.app.pools['test-pool'] = unittest.mock.Mock() + with self.assertRaises(AssertionError): + self.call_mgmt_func(b'admin.pool.Set.revisions_to_keep', + b'dom0', b'test-pool', b'abc') + self.assertEqual(self.app.pools['test-pool'].mock_calls, []) + self.assertFalse(self.app.save.called) + + def test_670_vm_volume_set_revisions_to_keep(self): + self.vm.volumes = unittest.mock.MagicMock() + volumes_conf = { + 'keys.return_value': ['root', 'private', 'volatile', 'kernel'], + } + self.vm.volumes.configure_mock(**volumes_conf) + self.vm.storage = unittest.mock.Mock() + value = self.call_mgmt_func(b'admin.vm.volume.Set.revisions_to_keep', + b'test-vm1', b'private', b'2') + self.assertIsNone(value) + self.assertEqual(self.vm.volumes.mock_calls, + [unittest.mock.call.keys(), + ('__getitem__', ('private',), {})]) + self.assertEqual(self.vm.volumes['private'].revisions_to_keep, 2) + self.app.save.assert_called_once_with() + + def test_671_vm_volume_set_revisions_to_keep_negative(self): + self.vm.volumes = unittest.mock.MagicMock() + volumes_conf = { + 'keys.return_value': ['root', 'private', 'volatile', 'kernel'], + } + self.vm.volumes.configure_mock(**volumes_conf) + self.vm.storage = unittest.mock.Mock() + with self.assertRaises(AssertionError): + self.call_mgmt_func(b'admin.vm.volume.Set.revisions_to_keep', + b'test-vm1', b'private', b'-2') + + def test_672_vm_volume_set_revisions_to_keep_not_a_number(self): + self.vm.volumes = unittest.mock.MagicMock() + volumes_conf = { + 'keys.return_value': ['root', 'private', 'volatile', 'kernel'], + } + self.vm.volumes.configure_mock(**volumes_conf) + self.vm.storage = unittest.mock.Mock() + with self.assertRaises(AssertionError): + self.call_mgmt_func(b'admin.vm.volume.Set.revisions_to_keep', + b'test-vm1', b'private', b'abc') + def test_990_vm_unexpected_payload(self): methods_with_no_payload = [ b'admin.vm.List',