When a storage volume is created with the parameter 'capacity', the parameter 'ref_cnt' is set on the storage pool path instead of the storage volume path. 'ref_cnt's are only related to storage volumes, not pools. Use the storage volume path instead of the storage pool path when setting 'ref_cnt' on a new volume. Signed-off-by: Crístian Deives <cristiandeives@gmail.com> --- src/kimchi/model/storagevolumes.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index f02efca..cf4611d 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -182,12 +182,14 @@ class StorageVolumesModel(object): {'name': name, 'pool': pool, 'err': e.get_error_message()}) - path = StoragePoolModel( - conn=self.conn, objstore=self.objstore).lookup(pool_name)['path'] + vol_info = StorageVolumeModel(conn=self.conn, + objstore=self.objstore).lookup(pool_name, + name) try: with self.objstore as session: - session.store('storagevolume', path, {'ref_cnt': 0}) + session.store('storagevolume', vol_info['path'], + {'ref_cnt': 0}) except Exception as e: # If the storage volume was created flawlessly, then lets hide this # error to avoid more error in the VM creation process -- 2.1.0

Signed-off-by: Crístian Deives <cristiandeives@gmail.com> --- src/kimchi/model/diskutils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/kimchi/model/diskutils.py b/src/kimchi/model/diskutils.py index 1d575cb..97fc9d9 100644 --- a/src/kimchi/model/diskutils.py +++ b/src/kimchi/model/diskutils.py @@ -34,7 +34,7 @@ def get_disk_ref_cnt(objstore, conn, path): with objstore as session: try: ref_cnt = session.get('storagevolume', path)['ref_cnt'] - except NotFoundError: + except (KeyError, NotFoundError): kimchi_log.info('Volume %s not found in obj store.' % path) ref_cnt = 0 # try to find this volume in existing vm -- 2.1.0

Instead of storing the number of VMs which use each storage volume ('ref_cnt'), we should store the names of the VMs which use each volume ('used_by'). That new variable can provide the same information to the user while also providing more details. Use the variable 'used_by' to store the names of the VMs which use a storage volume instead of 'ref_cnt' which stores the number of VMs which use a volume. Signed-off-by: Crístian Deives <cristiandeives@gmail.com> --- docs/API.md | 4 +-- src/kimchi/control/storagevolumes.py | 2 +- src/kimchi/mockmodel.py | 4 +-- src/kimchi/model/diskutils.py | 18 +++++------ src/kimchi/model/storagevolumes.py | 22 +++++-------- src/kimchi/model/vms.py | 14 ++++++--- src/kimchi/model/vmstorages.py | 50 ++++++++++++++++-------------- tests/test_model_storagevolume.py | 2 +- tests/test_rest.py | 2 +- ui/js/src/kimchi.guest_storage_add.main.js | 2 +- ui/js/src/kimchi.storage_main.js | 2 +- 11 files changed, 60 insertions(+), 62 deletions(-) diff --git a/docs/API.md b/docs/API.md index 62e4063..b5d34d7 100644 --- a/docs/API.md +++ b/docs/API.md @@ -509,9 +509,7 @@ A interface represents available network interface on VM. * os_distro *(optional)*: os distribution of the volume, for iso volume only. * os_version *(optional)*: os version of the volume, for iso volume only. * bootable *(optional)*: True if iso image is bootable and not corrupted. - * ref_cnt: Number of vms which used this volume, - 0 for volumes which are available for attachment. - >1 indicate number of vms used this volume. + * used_by: Name of vms which use this volume. * **DELETE**: Remove the Storage Volume * **POST**: *See Storage Volume Actions* diff --git a/src/kimchi/control/storagevolumes.py b/src/kimchi/control/storagevolumes.py index 9f5fcea..54fa263 100644 --- a/src/kimchi/control/storagevolumes.py +++ b/src/kimchi/control/storagevolumes.py @@ -56,7 +56,7 @@ class StorageVolume(Resource): 'capacity': self.info['capacity'], 'allocation': self.info['allocation'], 'path': self.info['path'], - 'ref_cnt': self.info['ref_cnt'], + 'used_by': self.info['used_by'], 'format': self.info['format']} for key in ('os_version', 'os_distro', 'bootable', 'base'): diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index ae97354..ecfd5bc 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -455,13 +455,13 @@ class MockStorageVolumes(object): 'allocation': 512, 'type': 'block', 'path': base_path + '1', - 'ref_cnt': 0}, + 'used_by': []}, 'unit:0:0:2': {'capacity': 2048, 'format': 'unknown', 'allocation': 512, 'type': 'block', 'path': base_path + '2', - 'ref_cnt': 0}} + 'used_by': []}} class MockPartitions(object): diff --git a/src/kimchi/model/diskutils.py b/src/kimchi/model/diskutils.py index 97fc9d9..566cc3a 100644 --- a/src/kimchi/model/diskutils.py +++ b/src/kimchi/model/diskutils.py @@ -29,14 +29,14 @@ from kimchi.xmlutils.disk import get_vm_disk_info, get_vm_disks """ -def get_disk_ref_cnt(objstore, conn, path): +def get_disk_used_by(objstore, conn, path): try: with objstore as session: try: - ref_cnt = session.get('storagevolume', path)['ref_cnt'] + used_by = session.get('storagevolume', path)['used_by'] except (KeyError, NotFoundError): kimchi_log.info('Volume %s not found in obj store.' % path) - ref_cnt = 0 + used_by = [] # try to find this volume in existing vm vms_list = VMsModel.get_vms(conn) for vm in vms_list: @@ -45,13 +45,13 @@ def get_disk_ref_cnt(objstore, conn, path): for disk in storages.keys(): d_info = get_vm_disk_info(dom, disk) if path == d_info['path']: - ref_cnt = ref_cnt + 1 + used_by.append(vm) try: session.store('storagevolume', path, - {'ref_cnt': ref_cnt}) + {'used_by': used_by}) except Exception as e: # Let the exception be raised. If we allow disks' - # ref_cnts to be out of sync, data corruption could + # used_by to be out of sync, data corruption could # occour if a disk is added to two guests # unknowingly. kimchi_log.error('Unable to store storage volume id in' @@ -64,12 +64,12 @@ def get_disk_ref_cnt(objstore, conn, path): # specially ones generated by 'session.store'. It is outside # to avoid conflict with the __exit__ function of 'with' raise OperationFailed('KCHVOL0017E', {'err': e.message}) - return ref_cnt + return used_by -def set_disk_ref_cnt(objstore, path, new_count): +def set_disk_used_by(objstore, path, new_used_by): try: with objstore as session: - session.store('storagevolume', path, {'ref_cnt': new_count}) + session.store('storagevolume', path, {'used_by': new_used_by}) except Exception as e: raise OperationFailed('KCHVOL0017E', {'err': e.message}) diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index cf4611d..56525d1 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -31,7 +31,7 @@ from kimchi.config import READONLY_POOL_TYPE from kimchi.exception import InvalidOperation, InvalidParameter, IsoFormatError from kimchi.exception import MissingParameter, NotFoundError, OperationFailed from kimchi.isoinfo import IsoImage -from kimchi.model.diskutils import get_disk_ref_cnt +from kimchi.model.diskutils import get_disk_used_by, set_disk_used_by from kimchi.model.storagepools import StoragePoolModel from kimchi.model.tasks import TaskModel from kimchi.utils import add_task, get_next_clone_name, get_unique_file_name @@ -148,6 +148,7 @@ class StorageVolumesModel(object): # Refresh to make sure volume can be found in following lookup StoragePoolModel.get_storagepool(pool_name, self.conn).refresh(0) + cb('OK', True) def _create_volume_with_capacity(self, cb, params): @@ -186,15 +187,7 @@ class StorageVolumesModel(object): objstore=self.objstore).lookup(pool_name, name) - try: - with self.objstore as session: - session.store('storagevolume', vol_info['path'], - {'ref_cnt': 0}) - except Exception as e: - # If the storage volume was created flawlessly, then lets hide this - # error to avoid more error in the VM creation process - kimchi_log.warning('Unable to store storage volume id in ' - 'objectstore due error: %s', e.message) + set_disk_used_by(self.objstore, vol_info['path'], []) cb('', True) @@ -339,12 +332,12 @@ class StorageVolumeModel(object): else: fmt = 'iso' - ref_cnt = get_disk_ref_cnt(self.objstore, self.conn, path) + used_by = get_disk_used_by(self.objstore, self.conn, path) res = dict(type=VOLUME_TYPE_MAP[info[0]], capacity=info[1], allocation=info[2], path=path, - ref_cnt=ref_cnt, + used_by=used_by, format=fmt) if fmt == 'iso': if os.path.islink(path): @@ -484,10 +477,9 @@ class StorageVolumeModel(object): 'pool': orig_pool_name, 'err': e.get_error_message()}) + new_vol = self.lookup(new_pool_name, new_vol_name) cb('adding volume to the object store') - new_vol_id = '%s:%s' % (new_pool_name, new_vol_name) - with self.objstore as session: - session.store('storagevolume', new_vol_id, {'ref_cnt': 0}) + set_disk_used_by(self.objstore, new_vol['path'], []) cb('OK', True) diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py index f0182b9..ed1500e 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -368,6 +368,8 @@ class VMModel(object): with self.objstore as session: session.delete('storagevolume', path) + domain_name = xpath_get_text(xml, XPATH_DOMAIN_NAME)[0] + for i, path in enumerate(all_paths): try: vir_orig_vol = vir_conn.storageVolLookupByPath(path) @@ -376,7 +378,6 @@ class VMModel(object): orig_pool_name = vir_pool.name().decode('utf-8') orig_vol_name = vir_orig_vol.name().decode('utf-8') except libvirt.libvirtError, e: - domain_name = xpath_get_text(xml, XPATH_DOMAIN_NAME)[0] raise OperationFailed('KCHVM0035E', {'name': domain_name, 'err': e.message}) @@ -437,9 +438,10 @@ class VMModel(object): xml = xml_item_update(xml, XPATH_DOMAIN_DISK_BY_FILE % path, new_vol['path'], 'file') - # set the new disk's ref_cnt + # set the new disk's used_by with self.objstore as session: - session.store('storagevolume', new_vol['path'], {'ref_cnt': 1}) + session.store('storagevolume', new_vol['path'], + {'used_by': [domain_name]}) rollback.prependDefer(_delete_disk_from_objstore, new_vol['path']) # remove the new volume should an error occur later @@ -946,8 +948,10 @@ class VMModel(object): try: with self.objstore as session: if path in session.get_list('storagevolume'): - n = session.get('storagevolume', path)['ref_cnt'] - session.store('storagevolume', path, {'ref_cnt': n-1}) + used_by = session.get('storagevolume', path)['used_by'] + used_by.remove(name) + session.store('storagevolume', path, + {'used_by': used_by}) except Exception as e: raise OperationFailed('KCHVOL0017E', {'err': e.message}) diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index 87c6b3d..d3d4a8b 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -28,7 +28,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.osinfo import lookup -from kimchi.model.diskutils import get_disk_ref_cnt, set_disk_ref_cnt +from kimchi.model.diskutils import get_disk_used_by, set_disk_used_by from kimchi.utils import kimchi_log from kimchi.xmlutils.disk import get_device_node, get_disk_xml from kimchi.xmlutils.disk import get_vm_disk_info, get_vm_disks @@ -111,7 +111,7 @@ class VMStoragesModel(object): raise InvalidParameter("KCHVMSTOR0012E") except Exception as e: raise InvalidParameter("KCHVMSTOR0015E", {'error': e}) - if vol_info['ref_cnt'] != 0: + if len(vol_info['used_by']) != 0: raise InvalidParameter("KCHVMSTOR0016E") valid_format = { @@ -142,11 +142,12 @@ class VMStoragesModel(object): raise OperationFailed("KCHVMSTOR0008E", {'error': e.message}) # Don't put a try-block here. Let the exception be raised. If we - # allow disks ref_cnts to be out of sync, data corruption could + # allow disks used_by to be out of sync, data corruption could # occour if a disk is added to two guests unknowingly. if params.get('vol'): - set_disk_ref_cnt(self.objstore, params['path'], - vol_info['ref_cnt'] + 1) + used_by = vol_info['used_by'] + used_by.append(vm_name) + set_disk_used_by(self.objstore, params['path'], used_by) return dev @@ -186,26 +187,27 @@ class VMStorageModel(object): path = self.lookup(vm_name, dev_name)['path'] # This has to be done before it's detached. If it wasn't # in the obj store, its ref count would have been updated - # by get_disk_ref_cnt() + # by get_disk_used_by() if path is not None: - ref_cnt = get_disk_ref_cnt(self.objstore, self.conn, path) + used_by = get_disk_used_by(self.objstore, self.conn, path) else: - kimchi_log.error("Unable to decrement volume ref_cnt on" + kimchi_log.error("Unable to decrement volume used_by on" " delete because no path could be found.") dom.detachDeviceFlags(etree.tostring(disk), get_vm_config_flag(dom, 'all')) except Exception as e: raise OperationFailed("KCHVMSTOR0010E", {'error': e.message}) - if ref_cnt is not None and ref_cnt > 0: - set_disk_ref_cnt(self.objstore, path, ref_cnt - 1) + if used_by is not None and vm_name in used_by: + used_by.remove(vm_name) + set_disk_used_by(self.objstore, path, used_by) else: - kimchi_log.error("Unable to decrement %s:%s ref_cnt on delete." + kimchi_log.error("Unable to update %s:%s used_by on delete." % (vm_name, dev_name)) def update(self, vm_name, dev_name, params): - old_disk_ref_cnt = None - new_disk_ref_cnt = None + old_disk_used_by = None + new_disk_used_by = None dom = VMModel.get_vm(vm_name, self.conn) @@ -219,10 +221,10 @@ class VMStorageModel(object): if new_disk_path != old_disk_path: # An empty path means a CD-ROM was empty or ejected: if old_disk_path is not '': - old_disk_ref_cnt = get_disk_ref_cnt( + old_disk_used_by = get_disk_used_by( self.objstore, self.conn, old_disk_path) if new_disk_path is not '': - new_disk_ref_cnt = get_disk_ref_cnt( + new_disk_used_by = get_disk_used_by( self.objstore, self.conn, new_disk_path) dev_info.update(params) @@ -234,14 +236,16 @@ class VMStorageModel(object): raise OperationFailed("KCHVMSTOR0009E", {'error': e.message}) try: - if old_disk_ref_cnt is not None and \ - old_disk_ref_cnt > 0: - set_disk_ref_cnt(self.objstore, old_disk_path, - old_disk_ref_cnt - 1) - if new_disk_ref_cnt is not None: - set_disk_ref_cnt(self.objstore, new_disk_path, - new_disk_ref_cnt + 1) + if old_disk_used_by is not None and \ + vm_name in old_disk_used_by: + old_disk_used_by.remove(vm_name) + set_disk_used_by(self.objstore, old_disk_path, + old_disk_used_by) + if new_disk_used_by is not None: + new_disk_used_by.append(vm_name) + set_disk_used_by(self.objstore, new_disk_path, + new_disk_used_by) except Exception as e: - kimchi_log.error("Unable to update dev ref_cnt on update due to" + kimchi_log.error("Unable to update dev used_by on update due to" " %s:" % e.message) return dev diff --git a/tests/test_model_storagevolume.py b/tests/test_model_storagevolume.py index a3c3ce3..ca4ebc1 100644 --- a/tests/test_model_storagevolume.py +++ b/tests/test_model_storagevolume.py @@ -195,7 +195,7 @@ class StorageVolumeTests(unittest.TestCase): self.assertEquals(200, resp.status) keys = [u'name', u'type', u'capacity', u'allocation', u'path', - u'ref_cnt', u'format'] + u'used_by', u'format'] for vol in json.loads(resp.read()): resp = self.request(uri + '/' + vol['name']) self.assertEquals(200, resp.status) diff --git a/tests/test_rest.py b/tests/test_rest.py index c2ebd88..e43b327 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -265,7 +265,7 @@ class RestTests(unittest.TestCase): resp = self.request(vol_uri % vm['uuid']) vol = json.loads(resp.read()) self.assertEquals(1 << 30, vol['capacity']) - self.assertEquals(1, vol['ref_cnt']) + self.assertEquals(['test-vm'], vol['used_by']) # Start the VM resp = self.request('/vms/test-vm/start', '{}', 'POST') diff --git a/ui/js/src/kimchi.guest_storage_add.main.js b/ui/js/src/kimchi.guest_storage_add.main.js index 5df5625..768f1a5 100644 --- a/ui/js/src/kimchi.guest_storage_add.main.js +++ b/ui/js/src/kimchi.guest_storage_add.main.js @@ -81,7 +81,7 @@ kimchi.guest_storage_add_main = function() { if (result.length) { $.each(result, function(index, value) { // Only unused volume can be attached - if ((value.ref_cnt == 0) && (value.type != 'file' || validVolType[selectType].test(value.format))) { + if (value.type != 'file' || validVolType[selectType].test(value.format)) { options.push({ label: value.name, value: value.name diff --git a/ui/js/src/kimchi.storage_main.js b/ui/js/src/kimchi.storage_main.js index 9a45b65..5b5f7b3 100644 --- a/ui/js/src/kimchi.storage_main.js +++ b/ui/js/src/kimchi.storage_main.js @@ -237,7 +237,7 @@ kimchi.doListVolumes = function(poolObj) { ongoingVolumes.push(volumeName) var volume = { poolName: poolName, - ref_cnt: 0, + used_by: [], capacity: 0, name: volumeName, format: '', -- 2.1.0

As discussed in an e-mail thread (http://lists.ovirt.org/pipermail/kimchi-devel/2015-March/010154.html), we should stop blocking the user from creating VM storages when the storage has already been added to some VM. The UI may display a warning when that cases happens so the user should decide what to do. Remove check on 'used_by' when creating a VM storage. Signed-off-by: Crístian Deives <cristiandeives@gmail.com> --- src/kimchi/i18n.py | 1 - src/kimchi/model/vmstorages.py | 2 -- tests/test_model.py | 6 ++++++ 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index 18e84bc..6b4726c 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -285,7 +285,6 @@ messages = { "KCHVMSTOR0013E": _("Specify path to update virtual machine disk"), "KCHVMSTOR0014E": _("Controller type %(type)s limitation of %(limit)s devices reached"), "KCHVMSTOR0015E": _("Cannot retrieve disk path information for given pool/volume: %(error)s"), - "KCHVMSTOR0016E": _("Volume already in use by other virtual machine."), "KCHVMSTOR0017E": _("Only one of path or pool/volume can be specified to add a new virtual machine disk"), "KCHVMSTOR0018E": _("Volume chosen with format %(format)s does not fit in the storage type %(type)s"), diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index d3d4a8b..87cd6c8 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -111,8 +111,6 @@ class VMStoragesModel(object): raise InvalidParameter("KCHVMSTOR0012E") except Exception as e: raise InvalidParameter("KCHVMSTOR0015E", {'error': e}) - if len(vol_info['used_by']) != 0: - raise InvalidParameter("KCHVMSTOR0016E") valid_format = { "disk": ["raw", "bochs", "qcow", "qcow2", "qed", "vmdk"], diff --git a/tests/test_model.py b/tests/test_model.py index 88c020e..be00fe1 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -449,6 +449,12 @@ class ModelTests(unittest.TestCase): inst.vm_start(vm_name) disk = _attach_disk() + # make sure a disk can be attached more than once + prev_count += 1 + dup_disk = _attach_disk() + inst.vmstorage_delete(vm_name, dup_disk) + prev_count -= 1 + # VM disk still there after powered off inst.vm_poweroff(vm_name) disk_info = inst.vmstorage_lookup(vm_name, disk) -- 2.1.0

When a storage volume is created, its 'used_by' field should be set to empty. However, some functions don't set that field. Set the field 'used_by' to [] when creating a storage volume by 'url' or 'file'. Signed-off-by: Crístian Deives <cristiandeives@gmail.com> --- src/kimchi/model/storagevolumes.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index 56525d1..d3a9ad9 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -149,6 +149,8 @@ class StorageVolumesModel(object): # Refresh to make sure volume can be found in following lookup StoragePoolModel.get_storagepool(pool_name, self.conn).refresh(0) + set_disk_used_by(self.objstore, file_path, []) + cb('OK', True) def _create_volume_with_capacity(self, cb, params): @@ -268,6 +270,11 @@ class StorageVolumesModel(object): finally: os.remove(file_path) + vol_info = StorageVolumeModel(conn=self.conn, + objstore=self.objstore).lookup(pool_name, + name) + set_disk_used_by(self.objstore, vol_info['path'], []) + cb('OK', True) def get_list(self, pool_name): -- 2.1.0