[PATCHv2 0/2] Disk attachment
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Fix for ref_cnt for disk attachment. Royce Lv (2): disk attachment: fix ref_cnt to prevent double attach disk attachment: avoid checking cdrom ref_cnt src/kimchi/model/vmstorages.py | 31 ++++++++++++++++++++++++++++++- tests/test_model.py | 1 + 2 files changed, 31 insertions(+), 1 deletion(-) -- 1.8.3.2
From: Royce Lv <lvroyce@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@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}) + 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 + 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}) + 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 -- 1.8.3.2
On 05/13/2014 05:51 PM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@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@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@linux.vnet.ibm.com> IBM Linux Technology Center
From: Royce Lv <lvroyce@linux.vnet.ibm.com> CDROM can be shared by multiple vms, so we shall not check its ref_cnt. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/vmstorages.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index cf9748f..cff3108 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -160,10 +160,11 @@ class VMStoragesModel(object): raise InvalidParameter("KCHVMSTOR0012E") except Exception as e: raise InvalidParameter("KCHVMSTOR0015E", {'error': e}) - if vol_info['ref_cnt'] != 0: + if vol_info['ref_cnt'] != 0 and params['type'] == 'disk': raise InvalidParameter("KCHVMSTOR0016E") params['format'] = vol_info['format'] params['path'] = vol_info['path'] + params['src_type'] = _check_path(params['path']) params.setdefault( 'bus', _get_device_bus(params['type'], dom)) @@ -178,12 +179,15 @@ class VMStoragesModel(object): conn = self.conn.get() dom = conn.lookupByName(vm_name) dom.attachDeviceFlags(dev_xml, libvirt.VIR_DOMAIN_AFFECT_CURRENT) + + except Exception as e: + raise OperationFailed("KCHVMSTOR0008E", {'error': e.message}) + + if params.get('vol'): with self.objstore as session: ref_cnt = session.store('storagevolume', vol_id, {'ref_cnt': vol_info['ref_cnt'] + 1}) - except Exception as e: - raise OperationFailed("KCHVMSTOR0008E", {'error': e.message}) return params['dev'] def _get_storage_device_name(self, vm_name): -- 1.8.3.2
On 05/13/2014 05:51 PM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
CDROM can be shared by multiple vms, so we shall not check its ref_cnt.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/vmstorages.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index cf9748f..cff3108 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -160,10 +160,11 @@ class VMStoragesModel(object): raise InvalidParameter("KCHVMSTOR0012E") except Exception as e: raise InvalidParameter("KCHVMSTOR0015E", {'error': e}) - if vol_info['ref_cnt'] != 0: + if vol_info['ref_cnt'] != 0 and params['type'] == 'disk': raise InvalidParameter("KCHVMSTOR0016E") params['format'] = vol_info['format'] params['path'] = vol_info['path'] + params['src_type'] = _check_path(params['path']) params.setdefault( 'bus', _get_device_bus(params['type'], dom)) @@ -178,12 +179,15 @@ class VMStoragesModel(object): conn = self.conn.get() dom = conn.lookupByName(vm_name) dom.attachDeviceFlags(dev_xml, libvirt.VIR_DOMAIN_AFFECT_CURRENT) + + except Exception as e: + raise OperationFailed("KCHVMSTOR0008E", {'error': e.message}) + + if params.get('vol'): with self.objstore as session: ref_cnt = session.store('storagevolume', vol_id, {'ref_cnt': vol_info['ref_cnt'] + 1}) does this ref_cnt update for CDROM?
- except Exception as e: - raise OperationFailed("KCHVMSTOR0008E", {'error': e.message}) return params['dev']
def _get_storage_device_name(self, vm_name):
-- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center
On 2014年05月15日 22:21, Sheldon wrote:
On 05/13/2014 05:51 PM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
CDROM can be shared by multiple vms, so we shall not check its ref_cnt.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/vmstorages.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index cf9748f..cff3108 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -160,10 +160,11 @@ class VMStoragesModel(object): raise InvalidParameter("KCHVMSTOR0012E") except Exception as e: raise InvalidParameter("KCHVMSTOR0015E", {'error': e}) - if vol_info['ref_cnt'] != 0: + if vol_info['ref_cnt'] != 0 and params['type'] == 'disk': raise InvalidParameter("KCHVMSTOR0016E") params['format'] = vol_info['format'] params['path'] = vol_info['path'] + params['src_type'] = _check_path(params['path']) params.setdefault( 'bus', _get_device_bus(params['type'], dom)) @@ -178,12 +179,15 @@ class VMStoragesModel(object): conn = self.conn.get() dom = conn.lookupByName(vm_name) dom.attachDeviceFlags(dev_xml, libvirt.VIR_DOMAIN_AFFECT_CURRENT) + + except Exception as e: + raise OperationFailed("KCHVMSTOR0008E", {'error': e.message}) + + if params.get('vol'): with self.objstore as session: ref_cnt = session.store('storagevolume', vol_id, {'ref_cnt': vol_info['ref_cnt'] + 1}) does this ref_cnt update for CDROM? For cdrom attach and detach, we used path as parameter. When dealing with disk attach detach, we used pool/vol as parameters
- except Exception as e: - raise OperationFailed("KCHVMSTOR0008E", {'error': e.message}) return params['dev']
def _get_storage_device_name(self, vm_name):
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
CDROM can be shared by multiple vms, so we shall not check its ref_cnt.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/vmstorages.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index cf9748f..cff3108 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -160,10 +160,11 @@ class VMStoragesModel(object): raise InvalidParameter("KCHVMSTOR0012E") except Exception as e: raise InvalidParameter("KCHVMSTOR0015E", {'error': e}) - if vol_info['ref_cnt'] != 0: + if vol_info['ref_cnt'] != 0 and params['type'] == 'disk': now you just support disk and CDROM, will support other type? what about other type?
On 05/13/2014 05:51 PM, lvroyce@linux.vnet.ibm.com wrote: params['type'] != 'CDROM'
raise InvalidParameter("KCHVMSTOR0016E") params['format'] = vol_info['format'] params['path'] = vol_info['path'] + params['src_type'] = _check_path(params['path']) params.setdefault( 'bus', _get_device_bus(params['type'], dom)) @@ -178,12 +179,15 @@ class VMStoragesModel(object): conn = self.conn.get() dom = conn.lookupByName(vm_name) dom.attachDeviceFlags(dev_xml, libvirt.VIR_DOMAIN_AFFECT_CURRENT) + + except Exception as e: + raise OperationFailed("KCHVMSTOR0008E", {'error': e.message}) + + if params.get('vol'): with self.objstore as session: ref_cnt = session.store('storagevolume', vol_id, {'ref_cnt': vol_info['ref_cnt'] + 1})
- except Exception as e: - raise OperationFailed("KCHVMSTOR0008E", {'error': e.message}) return params['dev']
def _get_storage_device_name(self, vm_name):
-- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center
On 2014年05月15日 22:32, Sheldon wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
CDROM can be shared by multiple vms, so we shall not check its ref_cnt.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/vmstorages.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index cf9748f..cff3108 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -160,10 +160,11 @@ class VMStoragesModel(object): raise InvalidParameter("KCHVMSTOR0012E") except Exception as e: raise InvalidParameter("KCHVMSTOR0015E", {'error': e}) - if vol_info['ref_cnt'] != 0: + if vol_info['ref_cnt'] != 0 and params['type'] == 'disk': now you just support disk and CDROM, will support other type? what about other type?
On 05/13/2014 05:51 PM, lvroyce@linux.vnet.ibm.com wrote: params['type'] != 'CDROM' The other types will be rejected by json schema
raise InvalidParameter("KCHVMSTOR0016E") params['format'] = vol_info['format'] params['path'] = vol_info['path'] + params['src_type'] = _check_path(params['path']) params.setdefault( 'bus', _get_device_bus(params['type'], dom)) @@ -178,12 +179,15 @@ class VMStoragesModel(object): conn = self.conn.get() dom = conn.lookupByName(vm_name) dom.attachDeviceFlags(dev_xml, libvirt.VIR_DOMAIN_AFFECT_CURRENT) + + except Exception as e: + raise OperationFailed("KCHVMSTOR0008E", {'error': e.message}) + + if params.get('vol'): with self.objstore as session: ref_cnt = session.store('storagevolume', vol_id, {'ref_cnt': vol_info['ref_cnt'] + 1})
- except Exception as e: - raise OperationFailed("KCHVMSTOR0008E", {'error': e.message}) return params['dev']
def _get_storage_device_name(self, vm_name):
On 05/26/2014 02:33 PM, Royce Lv wrote:
On 2014年05月15日 22:32, Sheldon wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
CDROM can be shared by multiple vms, so we shall not check its ref_cnt.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/vmstorages.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index cf9748f..cff3108 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -160,10 +160,11 @@ class VMStoragesModel(object): raise InvalidParameter("KCHVMSTOR0012E") except Exception as e: raise InvalidParameter("KCHVMSTOR0015E", {'error': e}) - if vol_info['ref_cnt'] != 0: + if vol_info['ref_cnt'] != 0 and params['type'] == 'disk': now you just support disk and CDROM, will support other type? what about other type?
On 05/13/2014 05:51 PM, lvroyce@linux.vnet.ibm.com wrote: params['type'] != 'CDROM' The other types will be rejected by json schema I just care will you support other type?
such as "new_type" add. It will update the json schema and this file? if vol_info['ref_cnt'] != 0 and params['type'] == 'disk' or params['type'] == 'new_type'
raise InvalidParameter("KCHVMSTOR0016E") params['format'] = vol_info['format'] params['path'] = vol_info['path'] + params['src_type'] = _check_path(params['path']) params.setdefault( 'bus', _get_device_bus(params['type'], dom)) @@ -178,12 +179,15 @@ class VMStoragesModel(object): conn = self.conn.get() dom = conn.lookupByName(vm_name) dom.attachDeviceFlags(dev_xml, libvirt.VIR_DOMAIN_AFFECT_CURRENT) + + except Exception as e: + raise OperationFailed("KCHVMSTOR0008E", {'error': e.message}) + + if params.get('vol'): with self.objstore as session: ref_cnt = session.store('storagevolume', vol_id, {'ref_cnt': vol_info['ref_cnt'] + 1})
- except Exception as e: - raise OperationFailed("KCHVMSTOR0008E", {'error': e.message}) return params['dev']
def _get_storage_device_name(self, vm_name):
-- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center
participants (3)
-
lvroyce＠linux.vnet.ibm.com -
Royce Lv
-
Sheldon