[PATCHv3 0/2] ref_cnt fixes for disk attachment
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Royce Lv (2): disk attachment: fix ref_cnt to prevent double attach disk attachment: limit ref_cnt check to disk attachment 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..4d90265 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: + 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: + 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
Reviewed-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> On 05/26/2014 04:37 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..4d90265 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: + 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: + 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
-- 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. We limit ref_cnt checking just to disk attachment. 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 4d90265..8a35a79 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: 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
Reviewed-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> On 05/26/2014 04:37 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. We limit ref_cnt checking just to disk attachment.
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 4d90265..8a35a79 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: 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 (2)
-
lvroyce＠linux.vnet.ibm.com -
Sheldon