Re: [Kimchi-devel] [PATCH] Bug fix: Allow creating a pool using an existing path

In the newer libvirt version, virStoragePool.build() raises an error when the target path already exists. To avoid errors related to that, make sure that exception is properly handled. Also add a test case to cover this scenario. Signed-off-by: Aline Manera <alinefm(a)linux.vnet.ibm.com&gt; --- src/kimchi/model/storagepools.py | 33 +++++++++++++++++++++++---------- tests/test_model_storagepool.py | 12 +++++++++++- 2 files changed, 34 insertions(+), 11 deletions(-) diff --git a/src/kimchi/model/storagepools.py b/src/kimchi/model/storagepools.py index f022deb..aa075cd 100644 --- a/src/kimchi/model/storagepools.py +++ b/src/kimchi/model/storagepools.py @@ -101,9 +101,6 @@ class StoragePoolsModel(object): xml = ET.tostring(pool) try: pool = conn.storagePoolDefineXML(xml, 0) - # Add build step to make sure target directory created - pool.build(libvirt.VIR_STORAGE_POOL_BUILD_NEW) - pool.setAutostart(1) except libvirt.libvirtError, e: msg = "Fatal: Unable to create storage pool %s. " msg += error_msg @@ -111,6 +108,17 @@ class StoragePoolsModel(object): kimchi_log.error("Details: %s", e.message) sys.exit(1) + # Build and set autostart value to pool + # Ignore error as the pool was already successfully created + try: + # Add build step to make sure target directory created + # The build process may fail when the pool directory + # already exists on system + pool.build(libvirt.VIR_STORAGE_POOL_BUILD_NEW) + pool.setAutostart(1) + except: + pass + if pool.isActive() == 0: try: pool.create(0) @@ -182,18 +190,23 @@ class StoragePoolsModel(object): return name pool = conn.storagePoolDefineXML(xml, 0) + except libvirt.libvirtError as e: + kimchi_log.error("Problem creating Storage Pool: %s", e) + raise OperationFailed("KCHPOOL0007E", + {'name': name, 'err': e.get_error_message()}) + + # Build and set autostart value to pool + # Ignore error as the pool was already successfully created + # The build process fails when the pool directory already exists + try: if params['type'] in ['logical', 'dir', 'netfs', 'scsi']: pool.build(libvirt.VIR_STORAGE_POOL_BUILD_NEW) - # autostart dir, logical, netfs and scsi storage pools created - # from kimchi pool.setAutostart(1) else: - # disable autostart for others pool.setAutostart(0) - except libvirt.libvirtError as e: - kimchi_log.error("Problem creating Storage Pool: %s", e) - raise OperationFailed("KCHPOOL0007E", - {'name': name, 'err': e.get_error_message()}) + except: + pass + if params['type'] == 'netfs': output, error, returncode = run_command(['setsebool', '-P', 'virt_use_nfs=1']) diff --git a/tests/test_model_storagepool.py b/tests/test_model_storagepool.py index eabf875..70f3c7b 100644 --- a/tests/test_model_storagepool.py +++ b/tests/test_model_storagepool.py @@ -20,6 +20,7 @@ import json import os +import tempfile import unittest from functools import partial @@ -65,7 +66,7 @@ class StoragepoolTests(unittest.TestCase): self.assertIn('default', [pool['name'] for pool in storagepools]) with RollbackContext() as rollback: - # Now add a couple of StoragePools to the mock model + # Now add a couple of storage pools for i in xrange(3): name = u'kīмсhī-storagepool-%i' % i req = json.dumps({'name': name, 'type': 'dir', @@ -97,6 +98,15 @@ class StoragepoolTests(unittest.TestCase): pools = json.loads(self.request('/storagepools').read()) self.assertEquals(len(storagepools) + 3, len(pools)) + # Create a pool with an existing path + tmp_path = tempfile.mkdtemp(dir='/var/lib/kimchi') + rollback.prependDefer(os.rmdir, tmp_path) + req = json.dumps({'name': 'existing_path', 'type': 'dir', + 'path': tmp_path}) + resp = self.request('/storagepools', req, 'POST') + rollback.prependDefer(model.storagepool_delete, 'existing_path') + self.assertEquals(201, resp.status) + # Reserved pool return 400 req = json.dumps({'name': 'kimchi_isos', 'type': 'dir', 'path': '/var/lib/libvirt/images/%i' % i})