[Kimchi-devel] [PATCH v3] Prevent disks from being added twice
Christy Perez
christy at linux.vnet.ibm.com
Mon Nov 10 15:56:22 UTC 2014
On 11/10/2014 06:48 AM, Aline Manera wrote:
>
> Just a minor comment below:
>
> On 11/05/2014 09:08 PM, Christy Perez wrote:
>> Signed-off-by: Christy Perez <christy at linux.vnet.ibm.com>
>> ---
>> 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.
Oops. I ended up not needing that. I can re-send the patch without it.
>
>> 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
>
More information about the Kimchi-devel
mailing list