Re: [Kimchi-devel] [RFC: PATCH v1] Prevent disks from being added more than once

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?

>> 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(a)linux.vnet.ibm.com&gt; >> --- >> 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):