On 12/05/2015 11:58, CrÃstian Deives wrote:
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):
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
The UI code does not comply the backend code. I'd suggest to change the UI to use the used_by info instead of ref_cnt and in a separated patch allow user to attach a disk independent of the used_by value. But again, we need to warn the user when a volume in use is selected.