[PATCH v4 0/2] Replace storage volume 'ref_cnt' with 'used_by'
The difference between this and the previous patchset (v3) is: - The UI stays consistent with the previous behavior, i.e., it won't display disks which are used by other VMs. Crístian Deives (2): Replace storage volume 'ref_cnt' with 'used_by' Set 'used_by' to [] when creating some volumes 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 | 25 +++++++-------- 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, 64 insertions(+), 61 deletions(-) -- 2.4.1
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 | 20 ++++-------- 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, 59 insertions(+), 61 deletions(-) diff --git a/docs/API.md b/docs/API.md index 71f2539..e022c9e 100644 --- a/docs/API.md +++ b/docs/API.md @@ -510,9 +510,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 c81cabb..19dfd1e 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -469,13 +469,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 0ab96f1..0be2024 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 dc807e4..ab032e3 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -32,7 +32,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 @@ -158,14 +158,7 @@ class StorageVolumesModel(object): if params.get('upload', False): upload_volumes[vol_path] = {'lock': threading.Lock(), 'offset': 0} - try: - with self.objstore as session: - session.store('storagevolume', vol_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) @@ -310,12 +303,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): @@ -455,10 +448,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 fea1de1..b6aaffb 100644 --- a/tests/test_model_storagevolume.py +++ b/tests/test_model_storagevolume.py @@ -253,7 +253,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 914b602..2411393 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -264,7 +264,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..84edbbc 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.used_by.length == 0 && (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.4.1
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 | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index ab032e3..9165946 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -239,6 +239,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.4.1
participants (2)
-
Aline Manera -
Crístian Deives