Re: [Kimchi-devel] [PATCH v3] Prevent disks from being added twice

Signed-off-by: Christy Perez <christy(a)linux.vnet.ibm.com&gt; --- src/kimchi/model/diskutils.py | 75 ++++++++++++++++++++++++++++++++++++++ src/kimchi/model/storagevolumes.py | 51 +++++++++----------------- src/kimchi/model/vmstorages.py | 70 +++++++++++++++++++++++++++++++---- 3 files changed, 154 insertions(+), 42 deletions(-) create mode 100644 src/kimchi/model/diskutils.py diff --git a/src/kimchi/model/diskutils.py b/src/kimchi/model/diskutils.py new file mode 100644 index 0000000..1d575cb --- /dev/null +++ b/src/kimchi/model/diskutils.py @@ -0,0 +1,75 @@ +# +# Project Kimchi +# +# Copyright IBM, Corp. 2014 +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + + +from kimchi.exception import OperationFailed, NotFoundError +from kimchi.model.vms import VMModel, VMsModel +from kimchi.utils import kimchi_log +from kimchi.xmlutils.disk import get_vm_disk_info, get_vm_disks + +""" + Functions that multiple storage-related models (e.g. VMStoragesModel, + VolumesModel) will need. +""" + + +def get_disk_ref_cnt(objstore, conn, path): + try: + with objstore as session: + try: + ref_cnt = session.get('storagevolume', path)['ref_cnt'] + except NotFoundError: + kimchi_log.info('Volume %s not found in obj store.' % path) + ref_cnt = 0 + # try to find this volume in existing vm + vms_list = VMsModel.get_vms(conn) + for vm in vms_list: + dom = VMModel.get_vm(vm, conn) + storages = get_vm_disks(dom) + for disk in storages.keys(): + d_info = get_vm_disk_info(dom, disk) + if path == d_info['path']: + ref_cnt = ref_cnt + 1 + try: + session.store('storagevolume', path, + {'ref_cnt': ref_cnt}) + except Exception as e: + # Let the exception be raised. If we allow disks' + # ref_cnts to be out of sync, data corruption could + # occour if a disk is added to two guests + # unknowingly. + kimchi_log.error('Unable to store storage volume id in' + ' objectstore due error: %s', + e.message) + raise OperationFailed('KCHVOL0017E', + {'err': e.message}) + except Exception as e: + # This exception is going to catch errors returned by 'with', + # specially ones generated by 'session.store'. It is outside + # to avoid conflict with the __exit__ function of 'with' + raise OperationFailed('KCHVOL0017E', {'err': e.message}) + return ref_cnt + + +def set_disk_ref_cnt(objstore, path, new_count): + try: + with objstore as session: + session.store('storagevolume', path, {'ref_cnt': new_count}) + except Exception as e: + raise OperationFailed('KCHVOL0017E', {'err': e.message}) diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index 9ff43e6..e7e0708 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -28,11 +28,10 @@ from kimchi.exception import InvalidOperation, InvalidParameter, IsoFormatError from kimchi.exception import MissingParameter, NotFoundError, OperationFailed from kimchi.isoinfo import IsoImage +from kimchi.model.diskutils import get_disk_ref_cnt from kimchi.model.storagepools import StoragePoolModel from kimchi.model.tasks import TaskModel -from kimchi.model.vms import VMsModel, VMModel from kimchi.utils import add_task, kimchi_log -from kimchi.xmlutils.disk import get_vm_disk_info, get_vm_disks from kimchi.xmlutils.utils import xpath_get_text @@ -161,7 +160,6 @@ def _create_volume_with_capacity(self, cb, params): params.setdefault('format', 'qcow2') name = params['name'] - vol_id = '%s:%s' % (pool_name, name) try: pool = StoragePoolModel.get_storagepool(pool_name, self.conn) xml = vol_xml % params @@ -176,9 +174,12 @@ def _create_volume_with_capacity(self, cb, params): {'name': name, 'pool': pool, 'err': e.get_error_message()}) + path = StoragePoolModel( + conn=self.conn, objstore=self.objstore).lookup(pool_name)['path'] + try: with self.objstore as session: - session.store('storagevolume', vol_id, {'ref_cnt': 0}) + session.store('storagevolume', path, {'ref_cnt': 0}) except Exception as e: # If the storage volume was created flawlessly, then lets hide this # error to avoid more error in the VM creation process @@ -252,34 +253,6 @@ def _get_storagevolume(self, poolname, name): else: raise - def _get_ref_cnt(self, pool, name, path): - vol_id = '%s:%s' % (pool, name) - try: - with self.objstore as session: - try: - ref_cnt = session.get('storagevolume', vol_id)['ref_cnt'] - except NotFoundError: - # Fix storage volume created outside kimchi scope - ref_cnt = 0 - # try to find this volume in exsisted vm - vms = VMsModel.get_vms(self.conn) - for vm in vms: - dom = VMModel.get_vm(vm, self.conn) - storages = get_vm_disks(dom) - for disk in storages.keys(): - d_info = get_vm_disk_info(dom, disk) - if path == d_info['path']: - ref_cnt = ref_cnt + 1 - session.store('storagevolume', vol_id, - {'ref_cnt': ref_cnt}) - except Exception as e: - # This exception is going to catch errors returned by 'with', - # specially ones generated by 'session.store'. It is outside - # to avoid conflict with the __exit__ function of 'with' - raise OperationFailed('KCHVOL0017E', {'err': e.message}) - - return ref_cnt - def lookup(self, pool, name): vol = self._get_storagevolume(pool, name) path = vol.path() @@ -292,7 +265,7 @@ def lookup(self, pool, name): # infomation. When there is no format information, we assume # it's 'raw'. fmt = 'raw' - ref_cnt = self._get_ref_cnt(pool, name, path) + ref_cnt = get_disk_ref_cnt(self.objstore, self.conn, path) res = dict(type=VOLUME_TYPE_MAP[info[0]], capacity=info[1], allocation=info[2], @@ -312,9 +285,19 @@ def lookup(self, pool, name): res.update( dict(os_distro=os_distro, os_version=os_version, path=path, bootable=bootable)) - return res

+ def lookup_by_path(self, path): + try: + conn = self.conn.get() + pool = conn.storageVolLookupByPath( + path).storagePoolLookupByVolume() + pool_name = pool.name() + vol_name = os.path.basename(path) + return self.lookup(pool_name, vol_name) + except Exception: + return None +

def wipe(self, pool, name): volume = self._get_storagevolume(pool, name) try: diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index 790766c..6bec768 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -27,6 +27,8 @@ from kimchi.model.storagevolumes import StorageVolumeModel from kimchi.model.utils import check_remote_disk_path, get_vm_config_flag from kimchi.osinfo import lookup +from kimchi.model.diskutils import get_disk_ref_cnt, set_disk_ref_cnt +from kimchi.utils import kimchi_log from kimchi.xmlutils.disk import get_device_node, get_disk_xml from kimchi.xmlutils.disk import get_vm_disk_info, get_vm_disks @@ -73,6 +75,7 @@ def _get_available_bus_address(self, bus_type, vm_name): return dict(address=address) def create(self, vm_name, params): + vol_model = None # Path will never be blank due to API.json verification. # There is no need to cover this case here. if not ('vol' in params) ^ ('path' in params): @@ -98,9 +101,9 @@ def create(self, vm_name, params): if params.get('vol'): try: pool = params['pool'] - vol_info = StorageVolumeModel( - conn=self.conn, - objstore=self.objstore).lookup(pool, params['vol']) + vol_model = StorageVolumeModel(conn=self.conn, + objstore=self.objstore) + vol_info = vol_model.lookup(pool, params['vol']) except KeyError: raise InvalidParameter("KCHVMSTOR0012E") except Exception as e: @@ -135,6 +138,14 @@ def create(self, vm_name, params): dom.attachDeviceFlags(xml, get_vm_config_flag(dom, 'all')) except Exception as e: raise OperationFailed("KCHVMSTOR0008E", {'error': e.message}) + + # Don't put a try-block here. Let the exception be raised. If we + # allow disks ref_cnts to be out of sync, data corruption could + # occour if a disk is added to two guests unknowingly. + if params.get('vol'): + set_disk_ref_cnt(self.objstore, params['path'], + vol_info['ref_cnt'] + 1) + return dev def get_list(self, vm_name): @@ -145,6 +156,7 @@ def get_list(self, vm_name): 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 @@ -152,28 +164,46 @@ def lookup(self, vm_name, dev_name): return get_vm_disk_info(dom, dev_name) def delete(self, vm_name, dev_name): - # Get storage device xml - dom = VMModel.get_vm(vm_name, self.conn) + conn = self.conn.get() + try: bus_type = self.lookup(vm_name, dev_name)['bus'] + dom = conn.lookupByName(vm_name) except NotFoundError: raise - dom = VMModel.get_vm(vm_name, self.conn) if (bus_type not in HOTPLUG_TYPE and DOM_STATE_MAP[dom.info()[0]] != 'shutoff'): raise InvalidOperation('KCHVMSTOR0011E') try: - conn = self.conn.get() - dom = conn.lookupByName(vm_name) disk = get_device_node(dom, dev_name) + path = get_vm_disk_info(dom, dev_name)['path'] + if path is None or len(path) < 1: + path = self.lookup(vm_name, dev_name)['path'] + # This has to be done before it's detached. If it wasn't + # in the obj store, its ref count would have been updated + # by get_disk_ref_cnt() + if path is not None: + ref_cnt = get_disk_ref_cnt(self.objstore, self.conn, path) + else: + kimchi_log.error("Unable to decrement volume ref_cnt on" + " delete because no path could be found.") dom.detachDeviceFlags(etree.tostring(disk), get_vm_config_flag(dom, 'all')) except Exception as e: raise OperationFailed("KCHVMSTOR0010E", {'error': e.message}) + if ref_cnt is not None and ref_cnt > 0: + set_disk_ref_cnt(self.objstore, path, ref_cnt - 1) + else: + kimchi_log.error("Unable to decrement %s:%s ref_cnt on delete." + % (vm_name, dev_name)) + def update(self, vm_name, dev_name, params): + old_disk_ref_cnt = None + new_disk_ref_cnt = None + dom = VMModel.get_vm(vm_name, self.conn) dev_info = self.lookup(vm_name, dev_name) @@ -181,6 +211,18 @@ def update(self, vm_name, dev_name, params): raise InvalidOperation("KCHVMSTOR0006E") params['path'] = check_remote_disk_path(params.get('path', '')) + + old_disk_path = dev_info['path'] + new_disk_path = params['path'] + if new_disk_path != old_disk_path: + # An empty path means a CD-ROM was empty or ejected: + if old_disk_path is not '': + old_disk_ref_cnt = get_disk_ref_cnt( + self.objstore, self.conn, old_disk_path) + if new_disk_path is not '': + new_disk_ref_cnt = get_disk_ref_cnt( + self.objstore, self.conn, new_disk_path) + dev_info.update(params) dev, xml = get_disk_xml(dev_info) @@ -188,4 +230,16 @@ def update(self, vm_name, dev_name, params): dom.updateDeviceFlags(xml, get_vm_config_flag(dom, 'all')) except Exception as e: raise OperationFailed("KCHVMSTOR0009E", {'error': e.message}) + + try: + if old_disk_ref_cnt is not None and \ + old_disk_ref_cnt > 0: + set_disk_ref_cnt(self.objstore, old_disk_path, + old_disk_ref_cnt - 1) + if new_disk_ref_cnt is not None: + set_disk_ref_cnt(self.objstore, new_disk_path, + new_disk_ref_cnt + 1) + except Exception as e: + kimchi_log.error("Unable to update dev ref_cnt on update due to" + " %s:" % e.message) return dev