Make add_pool/remove_pool coroutines, allow Pool.{setup,destroy} as coroutines

Pool setup/destroy may be a time consuming operation, allow them to be
asynchronous. Fortunately add_pool and remove_pool are used only through
Admin API, so the change does not require modification of other
components.

qubes/api/admin.py

@@ -666,7 +666,8 @@ class QubesAdminAPI(qubes.api.AbstractQubesAPI):
         self.fire_event_for_permission(name=pool_name,
             pool_config=pool_config)
 
-        self.app.add_pool(name=pool_name, driver=self.arg, **pool_config)
+        yield from self.app.add_pool(name=pool_name, driver=self.arg,
+            **pool_config)
         self.app.save()
 
     @qubes.api.method('admin.pool.Remove', no_payload=True,
@@ -678,7 +679,7 @@ class QubesAdminAPI(qubes.api.AbstractQubesAPI):
 
         self.fire_event_for_permission()
 
-        self.app.remove_pool(self.arg)
+        yield from self.app.remove_pool(self.arg)
         self.app.save()
 
     @qubes.api.method('admin.pool.Set.revisions_to_keep',

qubes/app.py

@@ -34,6 +34,7 @@ import time
 import traceback
 import uuid
 
+import asyncio
 import jinja2
 import libvirt
 import lxml.etree
@@ -1207,6 +1208,7 @@ class Qubes(qubes.PropertyHolder):
         for pool in self.pools.values():
             pool.setup()
 
+    @asyncio.coroutine
     def add_pool(self, name, **kwargs):
         """ Add a storage pool to config."""
 
@@ -1216,16 +1218,21 @@ class Qubes(qubes.PropertyHolder):
 
         kwargs['name'] = name
         pool = self._get_pool(**kwargs)
-        pool.setup()
+        ret = pool.setup()
+        if asyncio.iscoroutine(ret):
+            yield from ret
         self.pools[name] = pool
         return pool
 
+    @asyncio.coroutine
     def remove_pool(self, name):
         """ Remove a storage pool from config file.  """
         try:
             pool = self.pools[name]
             del self.pools[name]
-            pool.destroy()
+            ret = pool.destroy()
+            if asyncio.iscoroutine(ret):
+                yield from ret
         except KeyError:
             return
 

qubes/storage/__init__.py

@@ -782,6 +782,8 @@ class Pool:
     def destroy(self):
         ''' Called when removing the pool. Use this for implementation specific
             clean up.
+
+            This can be implemented as a coroutine.
         '''
         raise self._not_implemented("destroy")
 
@@ -793,6 +795,8 @@ class Pool:
     def setup(self):
         ''' Called when adding a pool to the system. Use this for implementation
             specific set up.
+
+            This can be implemented as a coroutine.
         '''
         raise self._not_implemented("setup")
 

qubes/tests/__init__.py

@@ -786,7 +786,8 @@ class SystemTestCase(QubesTestCase):
                 format(DEFAULT_LVM_POOL))
         self.pool = self._find_pool(volume_group, thin_pool)
         if not self.pool:
-            self.pool = self.app.add_pool(**POOL_CONF)
+            self.pool = self.loop.run_until_complete(
+                self.app.add_pool(**POOL_CONF))
             self.created_pool = True
 
     def _remove_vm_qubes(self, vm):

qubes/tests/api_admin.py

@@ -68,7 +68,8 @@ class AdminAPITestCase(qubes.tests.QubesTestCase):
         app.default_template = 'test-template'
         with qubes.tests.substitute_entry_points('qubes.storage',
                 'qubes.tests.storage'):
-            app.add_pool('test', driver='test')
+            self.loop.run_until_complete(
+                app.add_pool('test', driver='test'))
         app.default_pool = 'varlibqubes'
         app.save = unittest.mock.Mock()
         self.vm = app.add_new_vm('AppVM', label='red', name='test-vm1',
@@ -636,7 +637,7 @@ class TC_00_VMs(AdminAPITestCase):
                 'driver2': ['param3', 'param4']
             }[driver]
 
-        self.app.add_pool = unittest.mock.Mock()
+        add_pool_mock, self.app.add_pool = self.coroutine_mock()
 
         value = self.call_mgmt_func(b'admin.pool.Add', b'dom0', b'driver1',
             b'name=test-pool\nparam1=some-value\n')
@@ -644,7 +645,7 @@ class TC_00_VMs(AdminAPITestCase):
         self.assertEqual(mock_drivers.mock_calls, [unittest.mock.call()])
         self.assertEqual(mock_parameters.mock_calls,
             [unittest.mock.call('driver1')])
-        self.assertEqual(self.app.add_pool.mock_calls,
+        self.assertEqual(add_pool_mock.mock_calls,
             [unittest.mock.call(name='test-pool', driver='driver1',
                 param1='some-value')])
         self.assertTrue(self.app.save.called)
@@ -664,14 +665,14 @@ class TC_00_VMs(AdminAPITestCase):
                 'driver2': ['param3', 'param4']
             }[driver]
 
-        self.app.add_pool = unittest.mock.Mock()
+        add_pool_mock, self.app.add_pool = self.coroutine_mock()
 
         with self.assertRaises(qubes.api.PermissionDenied):
             self.call_mgmt_func(b'admin.pool.Add', b'dom0',
                 b'no-such-driver', b'name=test-pool\nparam1=some-value\n')
         self.assertEqual(mock_drivers.mock_calls, [unittest.mock.call()])
         self.assertEqual(mock_parameters.mock_calls, [])
-        self.assertEqual(self.app.add_pool.mock_calls, [])
+        self.assertEqual(add_pool_mock.mock_calls, [])
         self.assertFalse(self.app.save.called)
 
 
@@ -690,7 +691,7 @@ class TC_00_VMs(AdminAPITestCase):
                 'driver2': ['param3', 'param4']
             }[driver]
 
-        self.app.add_pool = unittest.mock.Mock()
+        add_pool_mock, self.app.add_pool = self.coroutine_mock()
 
         with self.assertRaises(qubes.api.PermissionDenied):
             self.call_mgmt_func(b'admin.pool.Add', b'dom0',
@@ -698,7 +699,7 @@ class TC_00_VMs(AdminAPITestCase):
         self.assertEqual(mock_drivers.mock_calls, [unittest.mock.call()])
         self.assertEqual(mock_parameters.mock_calls,
             [unittest.mock.call('driver1')])
-        self.assertEqual(self.app.add_pool.mock_calls, [])
+        self.assertEqual(add_pool_mock.mock_calls, [])
         self.assertFalse(self.app.save.called)
 
     @unittest.mock.patch('qubes.storage.pool_drivers')
@@ -716,14 +717,14 @@ class TC_00_VMs(AdminAPITestCase):
                 'driver2': ['param3', 'param4']
             }[driver]
 
-        self.app.add_pool = unittest.mock.Mock()
+        add_pool_mock, self.app.add_pool = self.coroutine_mock()
 
         with self.assertRaises(qubes.api.PermissionDenied):
             self.call_mgmt_func(b'admin.pool.Add', b'dom0',
                 b'driver1', b'param1=value\nparam2=some-value\n')
         self.assertEqual(mock_drivers.mock_calls, [unittest.mock.call()])
         self.assertEqual(mock_parameters.mock_calls, [])
-        self.assertEqual(self.app.add_pool.mock_calls, [])
+        self.assertEqual(add_pool_mock.mock_calls, [])
         self.assertFalse(self.app.save.called)
 
     @unittest.mock.patch('qubes.storage.pool_drivers')
@@ -741,14 +742,14 @@ class TC_00_VMs(AdminAPITestCase):
                 'driver2': ['param3', 'param4']
             }[driver]
 
-        self.app.add_pool = unittest.mock.Mock()
+        add_pool_mock, self.app.add_pool = self.coroutine_mock()
 
         with self.assertRaises(qubes.api.PermissionDenied):
             self.call_mgmt_func(b'admin.pool.Add', b'dom0',
                 b'driver1', b'name=file\nparam1=value\nparam2=some-value\n')
         self.assertEqual(mock_drivers.mock_calls, [unittest.mock.call()])
         self.assertEqual(mock_parameters.mock_calls, [])
-        self.assertEqual(self.app.add_pool.mock_calls, [])
+        self.assertEqual(add_pool_mock.mock_calls, [])
         self.assertFalse(self.app.save.called)
 
     @unittest.mock.patch('qubes.storage.pool_drivers')
@@ -767,14 +768,14 @@ class TC_00_VMs(AdminAPITestCase):
                 'driver2': ['param3', 'param4']
             }[driver]
 
-        self.app.add_pool = unittest.mock.Mock()
+        add_pool_mock, self.app.add_pool = self.coroutine_mock()
 
         with self.assertRaises(qubes.api.PermissionDenied):
             self.call_mgmt_func(b'admin.pool.Add', b'dom0',
                 b'driver1', b'name=test-pool\nparam 1=value\n_param2\n')
         self.assertEqual(mock_drivers.mock_calls, [unittest.mock.call()])
         self.assertEqual(mock_parameters.mock_calls, [])
-        self.assertEqual(self.app.add_pool.mock_calls, [])
+        self.assertEqual(add_pool_mock.mock_calls, [])
         self.assertFalse(self.app.save.called)
 
     def test_170_pool_remove(self):
@@ -783,10 +784,10 @@ class TC_00_VMs(AdminAPITestCase):
             'lvm': unittest.mock.Mock(),
             'test-pool': unittest.mock.Mock(),
         }
-        self.app.remove_pool = unittest.mock.Mock()
+        remove_pool_mock, self.app.remove_pool = self.coroutine_mock()
         value = self.call_mgmt_func(b'admin.pool.Remove', b'dom0', b'test-pool')
         self.assertIsNone(value)
-        self.assertEqual(self.app.remove_pool.mock_calls,
+        self.assertEqual(remove_pool_mock.mock_calls,
             [unittest.mock.call('test-pool')])
         self.assertTrue(self.app.save.called)
 
@@ -796,11 +797,11 @@ class TC_00_VMs(AdminAPITestCase):
             'lvm': unittest.mock.Mock(),
             'test-pool': unittest.mock.Mock(),
         }
-        self.app.remove_pool = unittest.mock.Mock()
+        remove_pool_mock, self.app.remove_pool = self.coroutine_mock()
         with self.assertRaises(qubes.api.PermissionDenied):
             self.call_mgmt_func(b'admin.pool.Remove', b'dom0',
                 b'no-such-pool')
-        self.assertEqual(self.app.remove_pool.mock_calls, [])
+        self.assertEqual(remove_pool_mock.mock_calls, [])
         self.assertFalse(self.app.save.called)
 
     def test_180_label_list(self):
@@ -1170,6 +1171,14 @@ class TC_00_VMs(AdminAPITestCase):
     def dummy_coro(self, *args, **kwargs):
         pass
 
+    def coroutine_mock(self):
+        func_mock = unittest.mock.Mock()
+
+        @asyncio.coroutine
+        def coroutine_mock(*args, **kwargs):
+            return func_mock(*args, **kwargs)
+        return func_mock, coroutine_mock
+
     @unittest.mock.patch('qubes.storage.Storage.create')
     def test_330_vm_create_standalone(self, storage_mock):
         storage_mock.side_effect = self.dummy_coro

qubes/tests/integ/backup.py

@@ -526,8 +526,9 @@ class TC_00_Backup(BackupTestsMixin, qubes.tests.SystemTestCase):
             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.pool = self.loop.run_until_complete(
+                self.app.add_pool(
+                    **qubes.tests.storage_lvm.POOL_CONF))
             self.created_pool = True
         vms = self.create_backup_vms(pool=self.pool)
         try:
@@ -546,8 +547,9 @@ class TC_00_Backup(BackupTestsMixin, qubes.tests.SystemTestCase):
             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.pool = self.loop.run_until_complete(
+                self.app.add_pool(
+                    **qubes.tests.storage_lvm.POOL_CONF))
             self.created_pool = True
         vms = self.create_backup_vms()
         try:

qubes/tests/integ/basic.py

@@ -302,8 +302,8 @@ class TC_00_Basic(qubes.tests.SystemTestCase):
         pool_path = tempfile.mkdtemp(
             prefix='qubes-pool-', dir='/var/tmp')
         self.addCleanup(shutil.rmtree, pool_path)
-        pool = self.app.add_pool('test-filep', dir_path=pool_path,
-            driver='file')
+        pool = self.loop.run_until_complete(
+            self.app.add_pool('test-filep', dir_path=pool_path, driver='file'))
 
         self.vm = self.app.add_new_vm(qubes.vm.appvm.AppVM,
             name=vmname, template=self.app.default_template,

qubes/tests/integ/storage.py

@@ -312,22 +312,23 @@ class StorageTestMixin(object):
 class StorageFile(StorageTestMixin, qubes.tests.SystemTestCase):
     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')
+        self.pool = self.loop.run_until_complete(
+            self.app.add_pool(dir_path=self.dir_path,
+                name='test-pool', driver='file'))
         os.makedirs(os.path.join(self.dir_path, 'appvms', self.vm1.name),
             exist_ok=True)
         os.makedirs(os.path.join(self.dir_path, 'appvms', self.vm2.name),
             exist_ok=True)
 
     def tearDown(self):
-        self.app.remove_pool('test-pool')
+        self.loop.run_until_complete(self.app.remove_pool('test-pool'))
         shutil.rmtree(self.dir_path)
         super(StorageFile, self).tearDown()
 
 
 class StorageReflinkMixin(StorageTestMixin):
     def tearDown(self):
-        self.app.remove_pool(self.pool.name)
+        self.loop.run_until_complete(self.app.remove_pool(self.pool.name))
         super().tearDown()
 
     def init_pool(self, fs_type, **kwargs):
@@ -335,8 +336,9 @@ class StorageReflinkMixin(StorageTestMixin):
         dir_path = os.path.join('/var/tmp', name)
         qubes.tests.storage_reflink.mkdir_fs(dir_path, fs_type,
                                              cleanup_via=self.addCleanup)
-        self.pool = self.app.add_pool(name=name, dir_path=dir_path,
-                                      driver='file-reflink', **kwargs)
+        self.pool = self.loop.run_until_complete(
+            self.app.add_pool(name=name, dir_path=dir_path,
+                              driver='file-reflink', **kwargs))
 
 class StorageReflinkOnBtrfs(StorageReflinkMixin, qubes.tests.SystemTestCase):
     def init_pool(self):
@@ -355,13 +357,14 @@ class StorageLVM(StorageTestMixin, qubes.tests.SystemTestCase):
             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.pool = self.loop.run_until_complete(
+                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)
+            self.loop.run_until_complete(self.app.remove_pool(self.pool.name))
         super(StorageLVM, self).tearDown()
 
     def _find_pool(self, volume_group, thin_pool):

qubes/tests/storage.py

@@ -123,15 +123,16 @@ class TC_00_Pool(SystemTestCase):
         pool_name = 'asdjhrp89132'
 
         # make sure it's really does not exist
-        self.app.remove_pool(pool_name)
+        self.loop.run_until_complete(self.app.remove_pool(pool_name))
         self.assertFalse(self.assertPoolExists(pool_name))
 
-        self.app.add_pool(name=pool_name,
+        self.loop.run_until_complete(
+            self.app.add_pool(name=pool_name,
                           driver='file',
-                          dir_path='/tmp/asdjhrp89132')
+                          dir_path='/tmp/asdjhrp89132'))
         self.assertTrue(self.assertPoolExists(pool_name))
 
-        self.app.remove_pool(pool_name)
+        self.loop.run_until_complete(self.app.remove_pool(pool_name))
         self.assertFalse(self.assertPoolExists(pool_name))
 
     def assertPoolExists(self, pool):

qubes/tests/storage_file.py

@@ -114,13 +114,13 @@ class TC_01_FileVolumes(qubes.tests.QubesTestCase):
         """ Add a test file based storage pool """
         super(TC_01_FileVolumes, self).setUp()
         self.app = TestApp()
-        self.app.add_pool(**self.POOL_CONF)
+        self.loop.run_until_complete(self.app.add_pool(**self.POOL_CONF))
         self.app.default_pool = self.app.get_pool(self.POOL_NAME)
         self.app.create_dummy_template()
 
     def tearDown(self):
         """ Remove the file based storage pool after testing """
-        self.app.remove_pool("test-pool")
+        self.loop.run_until_complete(self.app.remove_pool("test-pool"))
         self.app.cleanup()
         self.app.close()
         del self.app
@@ -388,12 +388,12 @@ class TC_03_FilePool(qubes.tests.QubesTestCase):
         self.base_dir_patch2.start()
         self.base_dir_patch3.start()
         self.app = TestApp()
-        self.app.add_pool(**self.POOL_CONFIG)
+        self.loop.run_until_complete(self.app.add_pool(**self.POOL_CONFIG))
         self.app.create_dummy_template()
 
     def tearDown(self):
         """ Remove the file based storage pool after testing """
-        self.app.remove_pool("test-pool")
+        self.loop.run_until_complete(self.app.remove_pool("test-pool"))
         self.app.cleanup()
         self.app.close()
         del self.app
@@ -419,7 +419,8 @@ class TC_03_FilePool(qubes.tests.QubesTestCase):
 
         self.assertFalse(os.path.exists(pool_dir))
 
-        self.app.add_pool(name=pool_name, dir_path=pool_dir, driver='file')
+        self.loop.run_until_complete(
+            self.app.add_pool(name=pool_name, dir_path=pool_dir, driver='file'))
 
         self.assertTrue(os.path.exists(pool_dir))
         self.assertTrue(os.path.exists(appvms_dir))

qubes/tests/storage_kernels.py

@@ -74,13 +74,13 @@ class TC_01_KernelVolumes(qubes.tests.QubesTestCase):
         super(TC_01_KernelVolumes, self).setUp()
         self.app = TestApp()
         self.app.create_dummy_template()
-        self.app.add_pool(**self.POOL_CONF)
+        self.loop.run_until_complete(self.app.add_pool(**self.POOL_CONF))
         os.makedirs(self.POOL_DIR + '/dummy', exist_ok=True)
         open('/tmp/test-pool/dummy/modules.img', 'w').close()
 
     def tearDown(self):
         """ Remove the file based storage pool after testing """
-        self.app.remove_pool("test-pool")
+        self.loop.run_until_complete(self.app.remove_pool("test-pool"))
         self.app.cleanup()
         self.app.close()
         del self.app
@@ -231,11 +231,11 @@ class TC_03_KernelPool(qubes.tests.QubesTestCase):
         open(os.path.join(dummy_kernel, 'vmlinuz'), 'w').close()
         open(os.path.join(dummy_kernel, 'modules.img'), 'w').close()
         open(os.path.join(dummy_kernel, 'initramfs'), 'w').close()
-        self.app.add_pool(**self.POOL_CONFIG)
+        self.loop.run_until_complete(self.app.add_pool(**self.POOL_CONFIG))
 
     def tearDown(self):
         """ Remove the file based storage pool after testing """
-        self.app.remove_pool("test-pool")
+        self.loop.run_until_complete(self.app.remove_pool("test-pool"))
         self.app.cleanup()
         self.app.close()
         del self.app

qubes/tests/storage_lvm.py

@@ -75,13 +75,14 @@ class ThinPoolBase(qubes.tests.QubesTestCase):
         volume_group, thin_pool = DEFAULT_LVM_POOL.split('/', 1)
         self.pool = self._find_pool(volume_group, thin_pool)
         if not self.pool:
-            self.pool = self.app.add_pool(**POOL_CONF)
+            self.pool = self.loop.run_until_complete(
+                self.app.add_pool(**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)
+            self.loop.run_until_complete(self.app.remove_pool(self.pool.name))
         super(ThinPoolBase, self).tearDown()
 
 
@@ -1052,7 +1053,8 @@ class TC_02_StorageHelpers(ThinPoolBase):
             'driver': 'file',
             'dir_path': subdir
         }
-        pool2 = self.app.add_pool(**file_pool_config)
+        pool2 = self.loop.run_until_complete(
+            self.app.add_pool(**file_pool_config))
         pool = qubes.storage.search_pool_containing_dir(
             self.app.pools.values(), subdir)
         self.assertEqual(pool, pool2)