[Kimchi-devel] [PATCHv2 1/3] Fix vm creation storage rollback clean

[Aline Manera]

On 02/25/2014 04:07 AM, lvroyce at linux.vnet.ibm.com wrote:
> From: Royce Lv <lvroyce at linux.vnet.ibm.com>
>
> For pools which not allowed create and delete volume,
> if create vm fails, vm storage volume should not be destoryed.
> Put the constant in config.py to avoid circular import.
> For storage volume deletion, work around it by delete vol clear part,
> in the future, this will be fixed by volume reference information tracking.
>
> Signed-off-by: Royce Lv <lvroyce at linux.vnet.ibm.com>
> ---
>   src/kimchi/config.py.in |  3 +++
>   src/kimchi/model/vms.py | 14 ++++++--------
>   tests/test_model.py     |  2 +-
>   3 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/src/kimchi/config.py.in b/src/kimchi/config.py.in
> index 32d61c6..00aab27 100644
> --- a/src/kimchi/config.py.in
> +++ b/src/kimchi/config.py.in
> @@ -35,6 +35,9 @@ from kimchi.xmlutils import xpath_get_text
>
>   DEFAULT_LOG_LEVEL = "debug"
>
> +# Storage pool constant for read-only pool types
> +READONLY_POOL_TYPE = ['iscsi', 'scsi', 'mpath']
> +
>
>   def get_session_path():
>       return os.path.join(paths.state_dir, 'sessions')
> diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py
> index b482e80..4744bb6 100644
> --- a/src/kimchi/model/vms.py
> +++ b/src/kimchi/model/vms.py
> @@ -30,6 +30,7 @@ from cherrypy.process.plugins import BackgroundTask
>
>   from kimchi import vnc
>   from kimchi import xmlutils
> +from kimchi.config import READONLY_POOL_TYPE
>   from kimchi.exception import InvalidOperation, InvalidParameter
>   from kimchi.exception import MissingParameter, NotFoundError, OperationFailed
>   from kimchi.model.config import CapabilitiesModel
> @@ -186,7 +187,7 @@ class VMsModel(object):
>           # If storagepool is SCSI, volumes will be LUNs and must be passed by
>           # the user from UI or manually.
>           vol_list = []
> -        if t._get_storage_type() == 'scsi':
> +        if t._get_storage_type() in READONLY_POOL_TYPE:
>               if not params.get('volumes'):
>                   raise MissingParameter('KCHVM0017E')
>               else:
> @@ -218,9 +219,10 @@ class VMsModel(object):
>           try:
>               conn.defineXML(xml.encode('utf-8'))
>           except libvirt.libvirtError as e:
> -            for v in vol_list:
> -                vol = conn.storageVolLookupByPath(v['path'])
> -                vol.delete(0)
> +            if t._get_storage_type() not in READONLY_POOL_TYPE:
> +                for v in vol_list:
> +                    vol = conn.storageVolLookupByPath(v['path'])
> +                    vol.delete(0)
>               raise OperationFailed("KCHVM0007E", {'name': name,
>                                                    'err': e.get_error_message()})
>
> @@ -357,10 +359,6 @@ class VMModel(object):
>
>               dom.undefine()
>
> -            for path in paths:
> -                vol = conn.storageVolLookupByPath(path)
> -                vol.delete(0)
> -
>               with self.objstore as session:
>                   session.delete('vm', dom.UUIDString(), ignore_missing=True)
>
> diff --git a/tests/test_model.py b/tests/test_model.py
> index 0dbc279..b0039a3 100644
> --- a/tests/test_model.py
> +++ b/tests/test_model.py
> @@ -245,7 +245,7 @@ class ModelTests(unittest.TestCase):
>               vm_info = inst.vm_lookup(params['name'])
>               disk_path = '/var/lib/libvirt/images/%s-0.img' % vm_info['uuid']
>               self.assertTrue(os.access(disk_path, os.F_OK))
> -        self.assertFalse(os.access(disk_path, os.F_OK))
> +        self.assertTrue(os.access(disk_path, os.F_OK))

I don't understand this change.
After the rollback block the vm will be deleted and the disk_path should 
not exist anymore.

>
>       @unittest.skipUnless(utils.running_as_root(), 'Must be run as root')
>       def test_storagepool(self):