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

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.

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 \

+ 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:

+ 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):