[Kimchi-devel] [PATCH v4] Prevent disks from being added twice

Christy Perez christy at linux.vnet.ibm.com
Tue Nov 11 17:43:04 UTC 2014


Signed-off-by: Christy Perez <christy at linux.vnet.ibm.com>
---
 src/kimchi/model/diskutils.py      | 75 ++++++++++++++++++++++++++++++++++++++
 src/kimchi/model/storagevolumes.py | 38 +++----------------
 src/kimchi/model/vmstorages.py     | 70 +++++++++++++++++++++++++++++++----
 3 files changed, 143 insertions(+), 40 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 d610059..79845b5 100644
--- a/src/kimchi/model/storagevolumes.py
+++ b/src/kimchi/model/storagevolumes.py
@@ -30,6 +30,7 @@
 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
@@ -163,7 +164,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
@@ -178,9 +178,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
@@ -256,34 +259,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()
@@ -296,7 +271,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],
@@ -316,7 +291,6 @@ def lookup(self, pool, name):
             res.update(
                 dict(os_distro=os_distro, os_version=os_version, path=path,
                      bootable=bootable))
-
         return res
 
     def wipe(self, pool, name):
diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py
index 790766c..d01939e 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