[Kimchi-devel] [PATCHv2 1/2] disk attachment: fix ref_cnt to prevent double attach

[Sheldon]

On 05/13/2014 05:51 PM, lvroyce at linux.vnet.ibm.com wrote:
> From: Royce Lv <lvroyce at linux.vnet.ibm.com>
>
> Ref count is not updated when attach and detach,
> this will introduce double attachment.
> Fix and update the test case.
>
> Signed-off-by: Royce Lv <lvroyce at linux.vnet.ibm.com>
> ---
>   src/kimchi/model/vmstorages.py | 25 +++++++++++++++++++++++++
>   tests/test_model.py            |  1 +
>   2 files changed, 26 insertions(+)
>
> diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py
> index ecc20cf..cf9748f 100644
> --- a/src/kimchi/model/vmstorages.py
> +++ b/src/kimchi/model/vmstorages.py
> @@ -155,6 +155,7 @@ class VMStoragesModel(object):
>                   vol_info = StorageVolumeModel(
>                       conn=self.conn,
>                       objstore=self.objstore).lookup(pool, params['vol'])
> +                vol_id = '%s:%s' % (pool, params['vol'])
>               except KeyError:
>                   raise InvalidParameter("KCHVMSTOR0012E")
>               except Exception as e:
> @@ -177,6 +178,10 @@ class VMStoragesModel(object):
>               conn = self.conn.get()
>               dom = conn.lookupByName(vm_name)
>               dom.attachDeviceFlags(dev_xml, libvirt.VIR_DOMAIN_AFFECT_CURRENT)
> +            with self.objstore as session:
> +                ref_cnt = session.store('storagevolume', vol_id,
> +                                        {'ref_cnt': vol_info['ref_cnt'] + 1})
> +
I'm afraid ref_cnt will not pass pyplakes.
>           except Exception as e:
>               raise OperationFailed("KCHVMSTOR0008E", {'error': e.message})
>           return params['dev']
> @@ -200,6 +205,7 @@ class VMStoragesModel(object):
>   class VMStorageModel(object):
>       def __init__(self, **kargs):
>           self.conn = kargs['conn']
> +        self.objstore = kargs['objstore']
>
>       def lookup(self, vm_name, dev_name):
>           # Retrieve disk xml and format return dict
> @@ -208,6 +214,7 @@ class VMStorageModel(object):
>
>       def delete(self, vm_name, dev_name):
>           # Get storage device xml
> +        vol = None
>           dom = VMModel.get_vm(vm_name, self.conn)
>           try:
>               bus_type = self.lookup(vm_name, dev_name)['bus']
> @@ -218,6 +225,17 @@ class VMStorageModel(object):
>           if (bus_type not in HOTPLUG_TYPE and
>                   DOM_STATE_MAP[dom.info()[0]] != 'shutoff'):
>               raise InvalidOperation('KCHVMSTOR0011E')
> +        disk_info = get_vm_disk(dom, dev_name)
> +        try:
> +            conn = self.conn.get()
> +            vol = conn.storageVolLookupByPath(disk_info['path'])
> +            pool = vol.storagePoolLookupByVolume().name()
> +            vol_info = StorageVolumeModel(
> +                conn=self.conn,
> +                objstore=self.objstore).lookup(pool, vol.name())
> +        except libvirt.libvirtError:
> +            # disk does not exist in any stoage pool
does that means user can still use this disk to create a guest?
> +            pass
>
>           try:
>               conn = self.conn.get()
> @@ -228,6 +246,13 @@ class VMStorageModel(object):
>           except Exception as e:
>               raise OperationFailed("KCHVMSTOR0010E", {'error': e.message})
>
> +        if vol:
> +            # Handle ref_cnt after detach to avoid failure rollback
> +            vol_id = "%s:%s" % (pool, vol.name())
> +            with self.objstore as session:
> +                ref_cnt = session.store('storagevolume', vol_id,
> +                                        {'ref_cnt': vol_info['ref_cnt'] - 1})
> +
ditto
>       def update(self, vm_name, dev_name, params):
>           params['src_type'] = _check_path(params['path'])
>           dom = VMModel.get_vm(vm_name, self.conn)
> diff --git a/tests/test_model.py b/tests/test_model.py
> index e3dff95..9f43164 100644
> --- a/tests/test_model.py
> +++ b/tests/test_model.py
> @@ -242,6 +242,7 @@ class ModelTests(unittest.TestCase):
>
>               # Cold plug and unplug a disk
>               disk = _attach_disk()
> +            self.assertRaises(InvalidParameter, _attach_disk)
>               inst.vmstorage_delete(vm_name, disk)
>
>               # Hot plug a disk


-- 
Thanks and best regards!

Sheldon Feng(冯少合)<shaohef at linux.vnet.ibm.com>
IBM Linux Technology Center