[Kimchi-devel] [PATCH v3] Prevent disks from being added twice
Christy Perez
christy at linux.vnet.ibm.com
Wed Nov 5 23:08:29 UTC 2014
Signed-off-by: Christy Perez <christy at linux.vnet.ibm.com>
---
src/kimchi/model/diskutils.py | 75 ++++++++++++++++++++++++++++++++++++++
src/kimchi/model/storagevolumes.py | 51 +++++++++-----------------
src/kimchi/model/vmstorages.py | 70 +++++++++++++++++++++++++++++++----
3 files changed, 154 insertions(+), 42 deletions(-)
create mode 100644 src/kimchi/model/diskutils.py
diff --git a/src/kimchi/model/diskutils.py b/src/kimchi/model/diskutils.py
new file mode 100644
index 0000000..1d575cb
--- /dev/null
+++ b/src/kimchi/model/diskutils.py
@@ -0,0 +1,75 @@
+#
+# Project Kimchi
+#
+# Copyright IBM, Corp. 2014
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2.1 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+
+from kimchi.exception import OperationFailed, NotFoundError
+from kimchi.model.vms import VMModel, VMsModel
+from kimchi.utils import kimchi_log
+from kimchi.xmlutils.disk import get_vm_disk_info, get_vm_disks
+
+"""
+ Functions that multiple storage-related models (e.g. VMStoragesModel,
+ VolumesModel) will need.
+"""
+
+
+def get_disk_ref_cnt(objstore, conn, path):
+ try:
+ with objstore as session:
+ try:
+ ref_cnt = session.get('storagevolume', path)['ref_cnt']
+ except NotFoundError:
+ kimchi_log.info('Volume %s not found in obj store.' % path)
+ ref_cnt = 0
+ # try to find this volume in existing vm
+ vms_list = VMsModel.get_vms(conn)
+ for vm in vms_list:
+ dom = VMModel.get_vm(vm, conn)
+ storages = get_vm_disks(dom)
+ for disk in storages.keys():
+ d_info = get_vm_disk_info(dom, disk)
+ if path == d_info['path']:
+ ref_cnt = ref_cnt + 1
+ try:
+ session.store('storagevolume', path,
+ {'ref_cnt': ref_cnt})
+ except Exception as e:
+ # Let the exception be raised. If we allow disks'
+ # ref_cnts 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'
+ ' objectstore due error: %s',
+ e.message)
+ raise OperationFailed('KCHVOL0017E',
+ {'err': e.message})
+ except Exception as e:
+ # This exception is going to catch errors returned by 'with',
+ # 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
+
+
+def set_disk_ref_cnt(objstore, path, new_count):
+ try:
+ with objstore as session:
+ session.store('storagevolume', path, {'ref_cnt': new_count})
+ 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 9ff43e6..e7e0708 100644
--- a/src/kimchi/model/storagevolumes.py
+++ b/src/kimchi/model/storagevolumes.py
@@ -28,11 +28,10 @@
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.storagepools import StoragePoolModel
from kimchi.model.tasks import TaskModel
-from kimchi.model.vms import VMsModel, VMModel
from kimchi.utils import add_task, kimchi_log
-from kimchi.xmlutils.disk import get_vm_disk_info, get_vm_disks
from kimchi.xmlutils.utils import xpath_get_text
@@ -161,7 +160,6 @@ def _create_volume_with_capacity(self, cb, params):
params.setdefault('format', 'qcow2')
name = params['name']
- vol_id = '%s:%s' % (pool_name, name)
try:
pool = StoragePoolModel.get_storagepool(pool_name, self.conn)
xml = vol_xml % params
@@ -176,9 +174,12 @@ def _create_volume_with_capacity(self, cb, params):
{'name': name, 'pool': pool,
'err': e.get_error_message()})
+ path = StoragePoolModel(
+ conn=self.conn, objstore=self.objstore).lookup(pool_name)['path']
+
try:
with self.objstore as session:
- session.store('storagevolume', vol_id, {'ref_cnt': 0})
+ session.store('storagevolume', 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
@@ -252,34 +253,6 @@ def _get_storagevolume(self, poolname, name):
else:
raise
- def _get_ref_cnt(self, pool, name, path):
- vol_id = '%s:%s' % (pool, name)
- try:
- with self.objstore as session:
- try:
- ref_cnt = session.get('storagevolume', vol_id)['ref_cnt']
- except NotFoundError:
- # Fix storage volume created outside kimchi scope
- ref_cnt = 0
- # try to find this volume in exsisted vm
- vms = VMsModel.get_vms(self.conn)
- for vm in vms:
- dom = VMModel.get_vm(vm, self.conn)
- storages = get_vm_disks(dom)
- for disk in storages.keys():
- d_info = get_vm_disk_info(dom, disk)
- if path == d_info['path']:
- ref_cnt = ref_cnt + 1
- session.store('storagevolume', vol_id,
- {'ref_cnt': ref_cnt})
- except Exception as e:
- # This exception is going to catch errors returned by 'with',
- # 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
-
def lookup(self, pool, name):
vol = self._get_storagevolume(pool, name)
path = vol.path()
@@ -292,7 +265,7 @@ def lookup(self, pool, name):
# infomation. When there is no format information, we assume
# it's 'raw'.
fmt = 'raw'
- ref_cnt = self._get_ref_cnt(pool, name, path)
+ ref_cnt = get_disk_ref_cnt(self.objstore, self.conn, path)
res = dict(type=VOLUME_TYPE_MAP[info[0]],
capacity=info[1],
allocation=info[2],
@@ -312,9 +285,19 @@ def lookup(self, pool, name):
res.update(
dict(os_distro=os_distro, os_version=os_version, path=path,
bootable=bootable))
-
return res
+ def lookup_by_path(self, path):
+ try:
+ conn = self.conn.get()
+ pool = conn.storageVolLookupByPath(
+ path).storagePoolLookupByVolume()
+ pool_name = pool.name()
+ vol_name = os.path.basename(path)
+ return self.lookup(pool_name, vol_name)
+ except Exception:
+ return None
+
def wipe(self, pool, name):
volume = self._get_storagevolume(pool, name)
try:
diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py
index 790766c..6bec768 100644
--- a/src/kimchi/model/vmstorages.py
+++ b/src/kimchi/model/vmstorages.py
@@ -27,6 +27,8 @@
from kimchi.model.storagevolumes import StorageVolumeModel
from kimchi.model.utils import check_remote_disk_path, get_vm_config_flag
from kimchi.osinfo import lookup
+from kimchi.model.diskutils import get_disk_ref_cnt, set_disk_ref_cnt
+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
@@ -73,6 +75,7 @@ def _get_available_bus_address(self, bus_type, vm_name):
return dict(address=address)
def create(self, vm_name, params):
+ vol_model = None
# Path will never be blank due to API.json verification.
# There is no need to cover this case here.
if not ('vol' in params) ^ ('path' in params):
@@ -98,9 +101,9 @@ def create(self, vm_name, params):
if params.get('vol'):
try:
pool = params['pool']
- vol_info = StorageVolumeModel(
- conn=self.conn,
- objstore=self.objstore).lookup(pool, params['vol'])
+ vol_model = StorageVolumeModel(conn=self.conn,
+ objstore=self.objstore)
+ vol_info = vol_model.lookup(pool, params['vol'])
except KeyError:
raise InvalidParameter("KCHVMSTOR0012E")
except Exception as e:
@@ -135,6 +138,14 @@ def create(self, vm_name, params):
dom.attachDeviceFlags(xml, get_vm_config_flag(dom, 'all'))
except Exception as e:
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
+ # 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)
+
return dev
def get_list(self, vm_name):
@@ -145,6 +156,7 @@ def get_list(self, vm_name):
class VMStorageModel(object):
def __init__(self, **kargs):
self.conn = kargs['conn']
+ self.objstore = kargs['objstore']
def lookup(self, vm_name, dev_name):
# Retrieve disk xml and format return dict
@@ -152,28 +164,46 @@ def lookup(self, vm_name, dev_name):
return get_vm_disk_info(dom, dev_name)
def delete(self, vm_name, dev_name):
- # Get storage device xml
- dom = VMModel.get_vm(vm_name, self.conn)
+ conn = self.conn.get()
+
try:
bus_type = self.lookup(vm_name, dev_name)['bus']
+ dom = conn.lookupByName(vm_name)
except NotFoundError:
raise
- dom = VMModel.get_vm(vm_name, self.conn)
if (bus_type not in HOTPLUG_TYPE and
DOM_STATE_MAP[dom.info()[0]] != 'shutoff'):
raise InvalidOperation('KCHVMSTOR0011E')
try:
- conn = self.conn.get()
- dom = conn.lookupByName(vm_name)
disk = get_device_node(dom, dev_name)
+ path = get_vm_disk_info(dom, dev_name)['path']
+ if path is None or len(path) < 1:
+ 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()
+ if path is not None:
+ ref_cnt = get_disk_ref_cnt(self.objstore, self.conn, path)
+ else:
+ kimchi_log.error("Unable to decrement volume ref_cnt 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)
+ else:
+ kimchi_log.error("Unable to decrement %s:%s ref_cnt on delete."
+ % (vm_name, dev_name))
+
def update(self, vm_name, dev_name, params):
+ old_disk_ref_cnt = None
+ new_disk_ref_cnt = None
+
dom = VMModel.get_vm(vm_name, self.conn)
dev_info = self.lookup(vm_name, dev_name)
@@ -181,6 +211,18 @@ def update(self, vm_name, dev_name, params):
raise InvalidOperation("KCHVMSTOR0006E")
params['path'] = check_remote_disk_path(params.get('path', ''))
+
+ old_disk_path = dev_info['path']
+ new_disk_path = params['path']
+ 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(
+ self.objstore, self.conn, old_disk_path)
+ if new_disk_path is not '':
+ new_disk_ref_cnt = get_disk_ref_cnt(
+ self.objstore, self.conn, new_disk_path)
+
dev_info.update(params)
dev, xml = get_disk_xml(dev_info)
@@ -188,4 +230,16 @@ def update(self, vm_name, dev_name, params):
dom.updateDeviceFlags(xml, get_vm_config_flag(dom, 'all'))
except Exception as e:
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)
+ except Exception as e:
+ kimchi_log.error("Unable to update dev ref_cnt on update due to"
+ " %s:" % e.message)
return dev
--
1.9.3
More information about the Kimchi-devel
mailing list