[Kimchi-devel] [PATCH v3] Prevent disks from being added twice
Aline Manera
alinefm at linux.vnet.ibm.com
Mon Nov 10 12:48:58 UTC 2014
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.
> 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