[Kimchi-devel] [RFC: PATCH v1] Prevent disks from being added more than once
Royce Lv
lvroyce at linux.vnet.ibm.com
Mon Sep 29 06:24:51 UTC 2014
On 2014年09月27日 00:09, Christy Perez wrote:
> Thanks Royce! A few questions below...
>
> On 09/26/2014 04:48 AM, Royce Lv wrote:
>> 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.
> Absolutely. That's the big reason I sent this as an RFC. I have tested
> it on x86 on Fedora, and powerKVM, but only with file-based storage.
> There's a lot more testing to do, and I wanted to make sure before I get
> too deep into that, that no one objected to any of the changes I'd made.
>
>> 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.
> I'm not sure I follow. It sounds like you're explaining how things
> worked before my patch, but also making suggestions for what else I
> might change? Can you clarify?
I mean if you find we cannot use 'volume' attrib in all distro, what
changes do we need to fix ref_cnt thing in current "path" based solution.
>
>>> 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'
> I'll look into that. Thanks.
>
>>> + 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.
> Same. Thanks.
>
>>> + 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