[Kimchi-devel] [PATCH v3 3/5] Replace storage volume 'ref_cnt' with 'used_by'
Aline Manera
alinefm at linux.vnet.ibm.com
Fri May 15 15:27:33 UTC 2015
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 at 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.
More information about the Kimchi-devel
mailing list