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

[Aline Manera]

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})
-- 
2.1.0