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

[Daniel Henrique Barboza]

Reviewed-by: Daniel Barboza <dhbarboza82 at gmail.com>
Tested-by: Daniel Barboza <dhbarboza82 at 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 at 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})