[Kimchi-devel] [RFC: PATCH v1] Prevent disks from being added more than once
Royce Lv
lvroyce at linux.vnet.ibm.com
Fri Sep 26 09:48:02 UTC 2014
On 2014年09月25日 06:29, Christy Perez wrote:
> The concept of a ref count for disks had been added, but this count
> was dependant upon the pool name for a disk. However, the pool name
> wasn't being stored in the VM XML so it wasn't possible to retreive
> the pool for the disk in order to update the ref count.
Thanks for the fix, I noticed this for a while but always forget to
document.
I think use 'pool' and 'vol' is a nice way, still we need to check if
this works well in all distros for all storage types(iscsi, logical etc)
after all according to libvirt doc, this has been introduced in 1.0.5,
and 'mode' attr in 1.1.1,
not sure for powerkvm and rhel it can work.
If we are going to adopt current way, we can just update the ref count
when adding a new disk to vm(as it is chosen from pool),
the def create() delete() in vmstorages.py needs to change.
Though path is stored rather than pool in vm xml, we did a path query
for all existent disk, to guarantee the right ref count is recorded.
So things remaining are updating this ref count when volume's deleted
from and added to vm.
>
> To fix this, this patch changes the way that disks are added when
> created in libvirt storage pools. Instead of files, they are treated
> as volumes in pools.
>
> Note: Because of this modification, a small inconsistency in the API,
> which states that the pool for a vm's "storages" is returned, is
> corrected. The pool is returned as an empty string if the disk or
> volume was not create from a pool (e.g. a remote ISO).
>
> Signed-off-by: Christy Perez <christy at linux.vnet.ibm.com>
> ---
> src/kimchi/model/storagevolumes.py | 13 ++++++--
> src/kimchi/model/vmstorages.py | 68 +++++++++++++++++++++++++++++++++++---
> src/kimchi/vmdisks.py | 33 ++++++++++++++----
> src/kimchi/vmtemplate.py | 41 ++++++++++++++++-------
> 4 files changed, 129 insertions(+), 26 deletions(-)
>
> diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py
> index f9f226f..af97748 100644
> --- a/src/kimchi/model/storagevolumes.py
> +++ b/src/kimchi/model/storagevolumes.py
> @@ -249,19 +249,26 @@ def _get_ref_cnt(self, pool, name, path):
> try:
> ref_cnt = session.get('storagevolume', vol_id)['ref_cnt']
> except NotFoundError:
> + kimchi_log.info('Volume %s not found in obj store.' % vol_id)
> # Fix storage volume created outside kimchi scope
> ref_cnt = 0
> - # try to find this volume in exsisted vm
> + # try to find this volume in exsisting vm
> vms = VMsModel.get_vms(self.conn)
> for vm in vms:
> dom = VMModel.get_vm(vm, self.conn)
> storages = get_vm_disk_list(dom)
> for disk in storages:
> - d_info = get_vm_disk(dom, disk)
> + d_info = get_vm_disk(dom, disk, self.conn)
> if path == d_info['path']:
> ref_cnt = ref_cnt + 1
> - session.store('storagevolume', vol_id,
> + try:
> + session.store('storagevolume', vol_id,
> {'ref_cnt': ref_cnt})
> + except Exception as e:
> + # Just log an error if the obj store fails:
> + kimchi_log.warning('Unable to store storage volume id in '
> + 'objectstore due error: %s', 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
> diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py
> index 4d6c0b2..1b2c0fe 100644
> --- a/src/kimchi/model/vmstorages.py
> +++ b/src/kimchi/model/vmstorages.py
> @@ -32,7 +32,7 @@
> from kimchi.model.vms import DOM_STATE_MAP, VMModel
> from kimchi.model.storagevolumes import StorageVolumeModel
> from kimchi.model.utils import get_vm_config_flag
> -from kimchi.utils import check_url_path
> +from kimchi.utils import check_url_path, kimchi_log
> from kimchi.osinfo import lookup
> from kimchi.vmdisks import get_device_xml, get_vm_disk, get_vm_disk_list
> from kimchi.vmdisks import DEV_TYPE_SRC_ATTR_MAP
> @@ -73,6 +73,11 @@ def _get_storage_xml(params, ignore_source=False):
> source = E.source(protocol=output.scheme, name=output.path)
> source.append(host)
> disk.append(source)
> + elif src_type == 'volume':
> + source = E.source()
> + source.set('pool', params.get('pool'))
> + source.set('volume', params.get('vol'))
> + disk.append(source)
> else:
> # Fixing source attribute
> source = E.source()
> @@ -138,6 +143,8 @@ def create(self, vm_name, params):
> # Path will never be blank due to API.json verification.
> # There is no need to cover this case here.
> params['format'] = 'raw'
> + vol_info = None
> + vol_store_name = None
> if not ('vol' in params) ^ ('path' in params):
> raise InvalidParameter("KCHVMSTOR0017E")
> if params.get('vol'):
> @@ -146,6 +153,7 @@ def create(self, vm_name, params):
> vol_info = StorageVolumeModel(
> conn=self.conn,
> objstore=self.objstore).lookup(pool, params['vol'])
> + vol_store_name = '%s:%s' % (pool, params['vol'])
> except KeyError:
> raise InvalidParameter("KCHVMSTOR0012E")
> except Exception as e:
> @@ -158,6 +166,10 @@ def create(self, vm_name, params):
> params['format'] = vol_info['format']
> params['path'] = vol_info['path']
> params['src_type'] = _check_path(params['path'])
> + if params['src_type'] is 'file' and params.get('pool') and \
I think if it is 'block' probably we still want to use 'volume'
> + params.get('vol'):
> + params['src_type'] = 'volume'
> + # change from 'file' to volume if in a pool ^^
> if (params['bus'] not in HOTPLUG_TYPE
> and DOM_STATE_MAP[dom.info()[0]] != 'shutoff'):
> raise InvalidOperation('KCHVMSTOR0011E')
> @@ -171,6 +183,16 @@ def create(self, vm_name, params):
> dom.attachDeviceFlags(dev_xml, get_vm_config_flag(dom, 'all'))
> except Exception as e:
> raise OperationFailed("KCHVMSTOR0008E", {'error': e.message})
> +
> + try:
I suggest we wrap a helper function to update ref_cnt rather than spread
this code everywhere.
> + new_ref_cnt = vol_info['ref_cnt'] + 1
> + with self.objstore as session:
> + session.store('storagevolume', vol_store_name, {'ref_cnt': new_ref_cnt})
> + except Exception as e:
> + # Just log an error if the obj store fails:
> + kimchi_log.warning('Unable to store storage volume id in '
> + 'objectstore due error: %s', e.message)
> +
> return params['dev']
>
> def _get_storage_device_name(self, vm_name, params):
> @@ -192,25 +214,41 @@ def get_list(self, vm_name):
> dom = VMModel.get_vm(vm_name, self.conn)
> return get_vm_disk_list(dom)
>
> -
> 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
> dom = VMModel.get_vm(vm_name, self.conn)
> - return get_vm_disk(dom, dev_name)
> + disk = get_vm_disk(dom, dev_name, self.conn)
> + # Also include the pool and volume
> + pool_name = ''
> + vol_name = ''
> + try:
> + pool_name = disk.get('pool')
> + vol_name = disk.get('volume')
> + except Exception as e:
> + kimchi_log.warning('VMDisk lookup error: %s' % e.message)
> +
> + disk['pool'] = pool_name
> + disk['volume'] = vol_name
> + return disk
>
> def delete(self, vm_name, dev_name):
> # Get storage device xml
> dom = VMModel.get_vm(vm_name, self.conn)
> + disk_info = None
> + disk_xml = None
> + pool_name = ''
> + vol_name = ''
> try:
> - bus_type = self.lookup(vm_name, dev_name)['bus']
> + disk_info = self.lookup(vm_name, dev_name)
> + bus_type = disk_info['bus']
> 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')
> @@ -224,10 +262,30 @@ def delete(self, vm_name, dev_name):
> except Exception as e:
> raise OperationFailed("KCHVMSTOR0010E", {'error': e.message})
>
> + # Get the pool and vol of a disk so we can
> + # decrement its ref_cnt in the obj store
> + pool_name = disk_info.get('pool')
> + vol_name = disk_info.get('volume')
> +
> + if(pool_name and vol_name):
> + try:
> + vol_id = '%s:%s' % (pool_name, vol_name)
> + with self.objstore as session:
> + vol = session.get('storagevolume', vol_id)
> + if vol:
> + ref_cnt = vol['ref_cnt']
> + if ref_cnt > 0:
> + ref_cnt = ref_cnt - 1
> + session.store('storagevolume', vol_id, {'ref_cnt': ref_cnt})
> + except Exception as e:
> + kimchi_log.warning('Unable to decrement volume ref count due to %s.'
> + % e.message)
> +
> def update(self, vm_name, dev_name, params):
> path = params.get('path')
> if path and len(path) != 0:
> params['src_type'] = _check_path(path)
> + # change from 'file' to 'volume' if pool ^^
> ignore_source = False
> else:
> params['src_type'] = 'file'
> diff --git a/src/kimchi/vmdisks.py b/src/kimchi/vmdisks.py
> index 6012ada..185a499 100644
> --- a/src/kimchi/vmdisks.py
> +++ b/src/kimchi/vmdisks.py
> @@ -17,6 +17,8 @@
> # License along with this library; if not, write to the Free Software
> # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>
> +import libvirt
> +import os
> from lxml import objectify
>
> from kimchi.exception import NotFoundError
> @@ -34,37 +36,56 @@ def get_device_xml(dom, dev_name):
> return None
> return disk[0]
>
> +def get_pool_xml(conn, pool_name):
> + conn = conn.get()
> + return objectify.fromstring(conn.\
> + storagePoolLookupByName(pool_name).XMLDesc(0))
>
> -def get_vm_disk(dom, dev_name):
> +def get_vm_disk(dom, dev_name, conn):
> # Retrieve disk xml and format return dict
> disk = get_device_xml(dom, dev_name)
> if disk is None:
> raise NotFoundError(
> "KCHVMSTOR0007E",
> {'dev_name': dev_name, 'vm_name': dom.name()})
> - path = ""
> + path = ''
> dev_bus = 'ide'
> + pool_name = ''
> + vol_name = ''
> try:
> source = disk.source
> if source is not None:
> - src_type = disk.attrib['type']
> + src_type = disk.get('type')
> if src_type == 'network':
> host = source.host
> path = (source.attrib['protocol'] + '://' +
> host.attrib['name'] + ':' +
> host.attrib['port'] + source.attrib['name'])
> + elif src_type == 'volume':
> + pool_name = disk.source.get('pool')
> + pool = get_pool_xml(conn, pool_name)
> + pool_path = str(pool.target.path)
> + vol_name = disk.source.attrib['volume']
> + path = os.path.join(pool_path, vol_name)
> else:
> path = source.attrib[DEV_TYPE_SRC_ATTR_MAP[src_type]]
> # Retrieve storage bus type
> dev_bus = disk.target.attrib['bus']
> - except:
> - pass
> + except Exception as e:
> + kimchi_log.warning("get_vm_disk exception: %s " % e.message)
> +
> dev_type = disk.attrib['device']
> +
> + # Pool and volume may be empty strings, if the user passed a disk name.
> + # If the disk was from a storage pool, let's use that so we can keep track
> + # of its ref_cnt in the objects store.
> return {'dev': dev_name,
> 'type': dev_type,
> 'path': path,
> 'format': disk.driver.attrib['type'],
> - 'bus': dev_bus}
> + 'bus': dev_bus,
> + 'pool': pool_name,
> + 'volume': vol_name}
>
>
> def get_vm_disk_list(dom):
> diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py
> index 5f22db9..1940911 100644
> --- a/src/kimchi/vmtemplate.py
> +++ b/src/kimchi/vmtemplate.py
> @@ -187,24 +187,41 @@ def _get_cdrom_xml(self, libvirt_stream_protocols, qemu_stream_dns):
> return network_file % (params)
>
> def _get_disks_xml(self, vm_uuid):
> - storage_path = self._get_storage_path()
> + def TYPE(v):
> + return {'type': v}
> +
> + #storage_path = self._get_storage_path()
> + _pool = pool_name_from_uri(self.info['storagepool'])
> ret = ""
> for i, disk in enumerate(self.info['disks']):
> index = disk.get('index', i)
> - volume = "%s-%s.img" % (vm_uuid, index)
> - src = os.path.join(storage_path, volume)
> + _volume = "%s-%s.img" % (vm_uuid, index)
> + #src = os.path.join(storage_path, volume)
> dev = "%s%s" % (self._bus_to_dev[self.info['disk_bus']],
> string.lowercase[index])
> fmt = 'raw' if self._get_storage_type() in ['logical'] else 'qcow2'
> - params = {'src': src, 'dev': dev, 'bus': self.info['disk_bus'],
> - 'type': fmt}
> - ret += """
> - <disk type='file' device='disk'>
> - <driver name='qemu' type='%(type)s' cache='none'/>
> - <source file='%(src)s' />
> - <target dev='%(dev)s' bus='%(bus)s' />
> - </disk>
> - """ % params
> + #params = {'src': src, 'dev': dev, 'bus': self.info['disk_bus'],
> + # 'type': fmt}
> + #ret += """
> + #<disk type='file' device='disk'>
> + # <driver name='qemu' type='%(type)s' cache='none'/>
> + # <source file='%(src)s' />
> + # <target dev='%(dev)s' bus='%(bus)s' />
> + #</disk>
> + #""" % params
> +
> + # Instead do:
> + #<disk type='volume'>
> + # <driver name='qemu' type='%(type)s' cache='none'/>
> + # <source pool='%(pool)s' volume='%(volume)s' />
> + # <target dev='%(dev)s' bus='%(bus)s />
> + #</disk>
> + disk = E.disk(
> + E.driver(TYPE(fmt), name='qemu', cache='none'),
> + E.source(pool=_pool, volume=_volume),
> + E.target(dev=dev, bus=self.info['disk_bus']),
> + TYPE('volume'))
> + ret += etree.tostring(disk)
> return ret
>
> def _get_graphics_xml(self, params):
More information about the Kimchi-devel
mailing list