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

An update on this one: I had a v2 patch ready to go a few weeks ago, but Aline tested it for me against iscsi, and found another bug that blocks this. I opened Issue 466 for it: https://github.com/kimchi-project/kimchi/issues/466 Royce -- would you be able to fix that so I can get this duplicate disk patch tested? I'm just sitting on this fix in the meantime. This leaves me time to get back to the SMT work, which is fine, but I don't want to let it go too long or a rebase might be a nightmare.

I would offer to do it, but I've got a lot on my plate right now and I think you could probably fix the blocking issue pretty quickly. Thanks! On 10/09/2014 07:48 AM, Aline Manera wrote: > On 10/09/2014 06:04 AM, Royce Lv wrote: >> On 2014年10月03日 03:17, Christy Perez wrote: >>> On 09/29/2014 01:24 AM, Royce Lv wrote: >>>> 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. >>> Oh. Hm. I looked up some distros to see what they ship with. >>> >>> RHEL 6.0: libvirt-0.8.1-27 >>> RHEL 6.1: libvirt-0.8.7-18 >>> RHEL 6.2: libvirt-0.9.4-23 >>> RHEL 6.3: libvirt-0.9.10-21 >>> RHEL 6.4: libvirt-0.10.2-18 >>> RHEL/CentOS 6.5: libvirt-0.10.2-29 >>> >>> RHEL/CentOS 7.0: libvirt-1.1.1-29 >>> >>> Fedora 18: libvirt-0.10.2.2-3 >>> Fedora 19: libvirt-1.0.5.1-1 >>> Fedora 20: llibvirt-1.1.3.1-2 >>> >>> SLES 11 SP2: libvirt-0.9.6-0.13.18 >>> SLES 11 SP3: libvirt-1.0.5.1-0.7.10 >>> >>> OpenSuSE 12.2: libvirt-0.9.11.4-1.2.1 >>> OpenSuSE 12.3: libvirt-1.0.2-1.3.1 >>> >>> >>> OpenSuSE 13.1: libvirt-1.1.2-2.5.1 >>> >>> >>> So, it looks like this isn't possible at all right now, since it would >>> effectively break all of RHEL6. That's a huge bummer because I like the >>> idea of changing to volume and keeping track of which pool a disk is in. >>> >>> Without that, we'll have to change the way the disk is stored in the >>> object store, instead of changing how we look it up. The only way I see >>> is to store the full path instead of pool:image_name. Then (if we do >>> need the pool) we'll have to query the list of pools and find which >>> pool's path matches the image's base path. Storing the whole path has >>> the added benefit of making sure that any disks not in a kimchi pool are >>> also stored, and not added to any VM twice. >>> >>> What do you think? >> Sounds reasonable! Aline, would you pls add this to our release plan? > Done! > > I added a task with this patch title (Prevent disks from being added > more than once to guest) > to https://github.com/kimchi-project/kimchi/wiki/Todo-1.4 and assigned > to Christy. > >>> Thanks! >>> >>> - Christy >>> >>>>>>> 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):