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

Just a minor comment below: On 11/05/2014 09:08 PM, Christy Perez wrote: > 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 > + What is this function proposal? I don't see any use of it in this patch.

> 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