Reviewed-by: Daniel Barboza <dhbarboza82@gmail.com> Tested-by: Daniel Barboza <dhbarboza82@gmail.com> On 06/26/2015 05:15 PM, Aline Manera wrote:

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@linux.vnet.ibm.com> --- 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})