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

[Aline Manera]

On 02/26/2014 09:11 AM, Royce Lv wrote:
> On 2014年02月25日 22:38, Aline Manera wrote:
>> 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. See lines up:
> - for path in paths:
> - vol = conn.storageVolLookupByPath(path)
> - vol.delete(0)
> -
> When delete vm, clear the volume delete part, because if the volumes 
> of vm are based on scsi pool and we are trying to delete them, this 
> will result in error.
> This is a workaround, as I said in commit msg:
> this will be fixed by volume reference information tracking.

Don't you only remove this part only for READONLY_POOL_TYPE?
When storage pool is a dir, we should keep deleting the storage volume 
while deleting the vm

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