[Kimchi-devel] [PATCH v2 2/4] Replace storage volume 'ref_cnt' with 'used_by'
Crístian Deives
cristiandeives at gmail.com
Thu May 7 14:57:53 UTC 2015
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 +-
9 files changed, 58 insertions(+), 60 deletions(-)
diff --git a/docs/API.md b/docs/API.md
index 922ded1..bbd5eb3 100644
--- a/docs/API.md
+++ b/docs/API.md
@@ -502,9 +502,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 1d575cb..3734633 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 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')
--
2.1.0
More information about the Kimchi-devel
mailing list