[Kimchi-devel] [PATCH 6/8 - v5] Remove 'stogarepool' referencies from Templates code backend

Rodrigo Trujillo rodrigo.trujillo at linux.vnet.ibm.com
Wed Dec 2 10:53:26 UTC 2015


It is not necessary to pass the parameter 'storagepool' when create a
new template or update one. User is going to set a pool/name for each
disk instead.
This patch remove the legacy code related to 'storagepool' in templates.
It also fixes minor issues found and improve some checkings.

Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo at linux.vnet.ibm.com>
---
 src/wok/plugins/kimchi/API.json              |  12 ----
 src/wok/plugins/kimchi/control/templates.py  |   1 -
 src/wok/plugins/kimchi/docs/API.md           |   3 -
 src/wok/plugins/kimchi/i18n.py               |   3 +-
 src/wok/plugins/kimchi/model/storagepools.py |  10 +--
 src/wok/plugins/kimchi/model/templates.py    | 103 ++++++++++++++-------------
 src/wok/plugins/kimchi/utils.py              |   2 +-
 src/wok/plugins/kimchi/vmtemplate.py         |  52 ++++++--------
 8 files changed, 83 insertions(+), 103 deletions(-)

diff --git a/src/wok/plugins/kimchi/API.json b/src/wok/plugins/kimchi/API.json
index 265ae36..2b64d07 100644
--- a/src/wok/plugins/kimchi/API.json
+++ b/src/wok/plugins/kimchi/API.json
@@ -512,12 +512,6 @@
                     "minItems": 1,
                     "uniqueItems": true
                 },
-                "storagepool": {
-                    "description": "Location of the storage pool",
-                    "type": "string",
-                    "pattern": "^/plugins/kimchi/storagepools/[^/]+/?$",
-                    "error": "KCHTMPL0015E"
-                },
                 "networks": {
                     "description": "list of which networks will be assigned to the new VM.",
                     "type": "array",
@@ -695,12 +689,6 @@
                     "minItems": 1,
                     "uniqueItems": true
                 },
-                "storagepool": {
-                    "description": "Location of the storage pool",
-                    "type": "string",
-                    "pattern": "^/plugins/kimchi/storagepools/[^/]+/?$",
-                    "error": "KCHTMPL0015E"
-                },
                 "networks": {
                     "description": "list of which networks will be assigned to the new VM.",
                     "type": "array",
diff --git a/src/wok/plugins/kimchi/control/templates.py b/src/wok/plugins/kimchi/control/templates.py
index fc58815..4cd70c2 100644
--- a/src/wok/plugins/kimchi/control/templates.py
+++ b/src/wok/plugins/kimchi/control/templates.py
@@ -50,7 +50,6 @@ class Template(Resource):
             'memory': self.info['memory'],
             'cdrom': self.info.get('cdrom', None),
             'disks': self.info['disks'],
-            'storagepool': self.info['storagepool'],
             'networks': self.info['networks'],
             'folder': self.info.get('folder', []),
             'graphics': self.info['graphics'],
diff --git a/src/wok/plugins/kimchi/docs/API.md b/src/wok/plugins/kimchi/docs/API.md
index 8ef48d6..1c4ee50 100644
--- a/src/wok/plugins/kimchi/docs/API.md
+++ b/src/wok/plugins/kimchi/docs/API.md
@@ -281,8 +281,6 @@ Represents a snapshot of the Virtual Machine's primary monitor.
     * memory *(optional)*: The amount of memory assigned to the VM.
       Default is 1024M.
     * cdrom *(optional)*: A volume name or URI to an ISO image.
-    * storagepool *(optional)*: URI of the storagepool.
-      Default is '/storagepools/default'
     * networks *(optional)*: list of networks will be assigned to the new VM.
       Default is '[default]'
     * disks *(optional)*: An array of requested disks with the following optional fields
@@ -420,7 +418,6 @@ A interface represents available network interface on VM.
     * cpus: The number of CPUs assigned to the VM
     * memory: The amount of memory assigned to the VM
     * cdrom: A volume name or URI to an ISO image
-    * storagepool: URI of the storagepool where template allocates vm storage.
     * networks *(optional)*: list of networks will be assigned to the new VM.
     * disks: An array of requested disks with the following optional fields
       (either *size* or *volume* must be specified):
diff --git a/src/wok/plugins/kimchi/i18n.py b/src/wok/plugins/kimchi/i18n.py
index 7154a62..cf67085 100644
--- a/src/wok/plugins/kimchi/i18n.py
+++ b/src/wok/plugins/kimchi/i18n.py
@@ -128,7 +128,6 @@ messages = {
     "KCHVM0069E": _("Password field must be a string."),
     "KCHVM0070E": _("Error creating local host ssh rsa key of user 'root'."),
     "KCHVM0071E": _("Memory value %(mem)s must be aligned to %(alignment)sMiB."),
-    "KCHVM0072E": _("Template given has multiple disks assigned to different types of storage pools, conflicting with storage pool provided."),
     "KCHVM0073E": _("Unable to update the following parameters while the VM is offline: %(params)s"),
     "KCHVM0074E": _("Unable to update the following parameters while the VM is online: %(params)s"),
     "KCHVM0075E": _("Cannot change VCPU value because '%(vm)s' has a topology defined - sockets: %(sockets)s, cores: %(cores)s, threads: %(threads)s."),
@@ -179,6 +178,8 @@ messages = {
     "KCHTMPL0025E": _("When specifying CPU topology, VCPUs must be a product of sockets, cores, and threads."),
     "KCHTMPL0026E": _("When specifying CPU topology, each element must be an integer greater than zero."),
     "KCHTMPL0027E": _("Invalid disk image format. Valid formats: bochs, cloop, cow, dmg, qcow, qcow2, qed, raw, vmdk, vpc."),
+    "KCHTMPL0028E": _("When setting template disks, following parameters are required: 'index', 'pool name', 'format', 'size' or 'volume' (for scsi/iscsi pools)"),
+    "KCHTMPL0029E": _("Disk format must be 'raw', for logical, iscsi, and scsi pools."),
 
     "KCHPOOL0001E": _("Storage pool %(name)s already exists"),
     "KCHPOOL0002E": _("Storage pool %(name)s does not exist"),
diff --git a/src/wok/plugins/kimchi/model/storagepools.py b/src/wok/plugins/kimchi/model/storagepools.py
index ec866c5..ddfa7fa 100644
--- a/src/wok/plugins/kimchi/model/storagepools.py
+++ b/src/wok/plugins/kimchi/model/storagepools.py
@@ -34,7 +34,6 @@ from wok.plugins.kimchi.model.host import DeviceModel
 from wok.plugins.kimchi.model.libvirtstoragepool import StoragePoolDef
 from wok.plugins.kimchi.osinfo import defaults as tmpl_defaults
 from wok.plugins.kimchi.scan import Scanner
-from wok.plugins.kimchi.utils import pool_name_from_uri
 
 
 ISO_POOL_NAME = u'kimchi_isos'
@@ -72,7 +71,7 @@ class StoragePoolsModel(object):
     def _check_default_pools(self):
         pools = {}
 
-        default_pool = tmpl_defaults['storagepool']
+        default_pool = tmpl_defaults['disks'][0]['pool']['name']
         default_pool = default_pool.split('/')[-1]
 
         pools[default_pool] = {}
@@ -446,9 +445,10 @@ class StoragePoolModel(object):
             templates = session.get_list('template')
             for tmpl in templates:
                 t_info = session.get('template', tmpl)
-                t_pool = pool_name_from_uri(t_info['storagepool'])
-                if t_pool == pool_name:
-                    return True
+                for disk in t_info['disks']:
+                    t_pool = disk['pool']['name']
+                    if t_pool == pool_name:
+                        return True
             return False
 
     def deactivate(self, name):
diff --git a/src/wok/plugins/kimchi/model/templates.py b/src/wok/plugins/kimchi/model/templates.py
index eae2714..7b1412b 100644
--- a/src/wok/plugins/kimchi/model/templates.py
+++ b/src/wok/plugins/kimchi/model/templates.py
@@ -22,7 +22,7 @@ import libvirt
 import os
 import stat
 
-from wok.exception import InvalidOperation, InvalidParameter
+from wok.exception import InvalidOperation, InvalidParameter, MissingParameter
 from wok.exception import NotFoundError, OperationFailed
 from wok.utils import probe_file_permission_as_user, run_setfacl_set_attr
 from wok.xmlutils.utils import xpath_get_text
@@ -34,6 +34,43 @@ from wok.plugins.kimchi.utils import pool_name_from_uri
 from wok.plugins.kimchi.vmtemplate import VMTemplate
 
 
+def _check_disks_params(params, conn, objstore):
+    if 'disks' not in params:
+            return
+
+    basic_disk = ['index', 'format', 'pool', 'size']
+    ro_disk = ['index', 'format', 'pool', 'volume']
+    base_disk = ['index', 'base', 'pool']
+    for disk in params['disks']:
+        keys = sorted(disk.keys())
+        if ((keys == sorted(basic_disk)) or (keys == sorted(ro_disk)) or
+                (keys == sorted(base_disk))):
+            pass
+        else:
+            raise MissingParameter('KCHTMPL0028E')
+
+        if disk.get('pool', {}).get('name') is None:
+            raise MissingParameter('KCHTMPL0028E')
+
+        pool_uri = disk['pool']['name']
+        try:
+            pool_name = pool_name_from_uri(pool_uri)
+            conn.get().storagePoolLookupByName(pool_name.encode("utf-8"))
+        except Exception:
+            raise InvalidParameter("KCHTMPL0004E",
+                                   {'pool': pool_uri,
+                                    'template': pool_name})
+        if 'volume' in disk:
+            kwargs = {'conn': conn, 'objstore': objstore}
+            storagevolumes = __import__(
+                "wok.plugins.kimchi.model.storagevolumes", fromlist=[''])
+            pool_volumes = storagevolumes.StorageVolumesModel(
+                **kwargs).get_list(pool_name)
+            if disk['volume'] not in pool_volumes:
+                raise InvalidParameter("KCHTMPL0019E", {'pool': pool_name,
+                                       'volume': disk['volume']})
+
+
 class TemplatesModel(object):
     def __init__(self, **kargs):
         self.objstore = kargs['objstore']
@@ -54,6 +91,9 @@ class TemplatesModel(object):
                                            {'filename': iso, 'user': user,
                                             'err': excp})
 
+        # Check disks information
+        _check_disks_params(params, self.conn, self.objstore)
+
         cpu_info = params.get('cpu_info')
         if cpu_info:
             topology = cpu_info.get('topology')
@@ -71,19 +111,6 @@ class TemplatesModel(object):
                     check_topology(params['cpus'], topology)
 
         conn = self.conn.get()
-        pool_uri = params.get(u'storagepool', '')
-        if pool_uri:
-            try:
-                pool_name = pool_name_from_uri(pool_uri)
-                pool = conn.storagePoolLookupByName(pool_name.encode("utf-8"))
-            except Exception:
-                raise InvalidParameter("KCHTMPL0004E", {'pool': pool_uri,
-                                                        'template': name})
-
-            tmp_volumes = [disk['volume'] for disk in params.get('disks', [])
-                           if 'volume' in disk]
-            self.template_volume_validate(tmp_volumes, pool)
-
         for net_name in params.get(u'networks', []):
             try:
                 conn.networkLookupByName(net_name.encode('utf-8'))
@@ -112,27 +139,22 @@ class TemplatesModel(object):
         with self.objstore as session:
             return session.get_list('template')
 
-    def template_volume_validate(self, tmp_volumes, pool):
+    def template_volume_validate(self, volume, pool):
         kwargs = {'conn': self.conn, 'objstore': self.objstore}
         pool_type = xpath_get_text(pool.XMLDesc(0), "/pool/@type")[0]
         pool_name = unicode(pool.name(), 'utf-8')
 
-        # as we discussion, we do not mix disks from 2 different types of
-        # storage pools, for instance: we do not create a template with 2
-        # disks, where one comes from a SCSI pool and other is a .img in
-        # a DIR pool.
         if pool_type in ['iscsi', 'scsi']:
-            if not tmp_volumes:
+            if not volume:
                 raise InvalidParameter("KCHTMPL0018E")
 
             storagevolumes = __import__(
                 "wok.plugins.kimchi.model.storagevolumes", fromlist=[''])
             pool_volumes = storagevolumes.StorageVolumesModel(
                 **kwargs).get_list(pool_name)
-            vols = set(tmp_volumes) - set(pool_volumes)
-            if vols:
+            if volume not in pool_volumes:
                 raise InvalidParameter("KCHTMPL0019E", {'pool': pool_name,
-                                                        'volume': vols})
+                                                        'volume': volume})
 
 
 class TemplateModel(object):
@@ -146,17 +168,14 @@ class TemplateModel(object):
         with objstore as session:
             params = session.get('template', name)
         if overrides:
-            params.update(overrides)
-            if 'storagepool' in params:
-                libvVMT = LibvirtVMTemplate(params, conn=conn)
-                poolType = libvVMT._get_storage_type(params['storagepool'])
+            if 'storagepool' in overrides:
                 for i, disk in enumerate(params['disks']):
-                    if disk['pool']['type'] != poolType:
-                        raise InvalidOperation('KCHVM0072E')
-                    else:
-                        params['disks'][i]['pool']['name'] = \
-                            params['storagepool']
+                    params['disks'][i]['pool']['name'] = \
+                        overrides['storagepool']
+                del overrides['storagepool']
+            params.update(overrides)
 
+        _check_disks_params(params, conn, objstore)
         return LibvirtVMTemplate(params, False, conn)
 
     def lookup(self, name):
@@ -193,27 +212,15 @@ class TemplateModel(object):
         new_t = copy.copy(old_t)
         new_t.update(params)
 
+        # Check disks information
+        _check_disks_params(new_t, self.conn, self.objstore)
+
         if not self._validate_updated_cpu_params(new_t):
             raise InvalidParameter('KCHTMPL0025E')
 
-        ident = name
-
-        conn = self.conn.get()
-        pool_uri = new_t.get(u'storagepool', '')
-
-        if pool_uri:
-            try:
-                pool_name = pool_name_from_uri(pool_uri)
-                pool = conn.storagePoolLookupByName(pool_name.encode("utf-8"))
-            except Exception:
-                raise InvalidParameter("KCHTMPL0004E", {'pool': pool_uri,
-                                                        'template': name})
-            tmp_volumes = [disk['volume'] for disk in new_t.get('disks', [])
-                           if 'volume' in disk]
-            self.templates.template_volume_validate(tmp_volumes, pool)
-
         for net_name in params.get(u'networks', []):
             try:
+                conn = self.conn.get()
                 conn.networkLookupByName(net_name.encode('utf-8'))
             except Exception:
                 raise InvalidParameter("KCHTMPL0003E", {'network': net_name,
diff --git a/src/wok/plugins/kimchi/utils.py b/src/wok/plugins/kimchi/utils.py
index 31def2e..a5f5e97 100644
--- a/src/wok/plugins/kimchi/utils.py
+++ b/src/wok/plugins/kimchi/utils.py
@@ -38,7 +38,7 @@ def _uri_to_name(collection, uri):
     expr = '/plugins/kimchi/%s/(.*?)$' % collection
     m = re.match(expr, uri)
     if not m:
-        raise InvalidParameter("KCHUTILS0001E", {'uri': uri})
+        raise InvalidParameter("WOKUTILS0001E", {'uri': uri})
     return m.group(1)
 
 
diff --git a/src/wok/plugins/kimchi/vmtemplate.py b/src/wok/plugins/kimchi/vmtemplate.py
index 08adf4c..62d0e1a 100644
--- a/src/wok/plugins/kimchi/vmtemplate.py
+++ b/src/wok/plugins/kimchi/vmtemplate.py
@@ -76,36 +76,23 @@ class VMTemplate(object):
             graphics.update(graph_args)
             args['graphics'] = graphics
 
-        # Provide compatibility with old template version and sets data to
-        # correct output format if necessary
-        def _fix_disk_compatibilities(disks):
-            for i, disk in enumerate(disks):
-                if 'pool' not in disk:
-                    disk['pool'] = {'name': self.info['storagepool']}
-                if (isinstance(disk['pool'], str) or
-                        isinstance(disk['pool'], unicode)):
-                    disk['pool'] = {'name': disk['pool']}
-                disk['pool']['type'] = \
-                    self._get_storage_type(disk['pool']['name'])
-
-        if self.info.get('disks') is not None:
-            _fix_disk_compatibilities(self.info['disks'])
-        if args.get('disks') is not None:
-            _fix_disk_compatibilities(args['disks'])
-
-        # Merge disks dict with disks provided by user
-        default_disk = self.info['disks'][0]
-        for i, d in enumerate(args.get('disks', [])):
-            disk = dict(default_disk)
-            disk['index'] = i
-            disk.update(d)
-            if disk['pool']['type'] in ['logical', 'iscsi', 'scsi']:
-                disk['format'] = 'raw'
-            args['disks'][i] = disk
-
         # Override template values according to 'args'
         self.info.update(args)
 
+        # Find pool type for each disk
+        disks = self.info.get('disks')
+        for disk in disks:
+            pool_name = disk.get('pool', {}).get('name')
+            if pool_name is None:
+                raise MissingParameter('KCHTMPL0029E')
+
+            pool_type = self._get_storage_type(disk['pool']['name'])
+            disk['pool']['type'] = pool_type
+
+            if pool_type in ['logical', 'iscsi', 'scsi']:
+                if disk['format'] != 'raw':
+                    raise InvalidParameter('KCHTMPL0029E')
+
     def _get_os_info(self, args, scan):
         distro = version = 'unknown'
 
@@ -197,9 +184,9 @@ class VMTemplate(object):
         for index, disk in enumerate(self.info['disks']):
             params = dict(base_disk_params)
             params['format'] = disk['format']
+            params['index'] = index
             params.update(locals().get('%s_disk_params' %
                                        disk['pool']['type'], {}))
-            params['index'] = index
 
             volume = disk.get('volume')
             if volume is not None:
@@ -459,10 +446,11 @@ class VMTemplate(object):
             invalid['networks'] = invalid_networks
 
         # validate storagepools integrity
-        pool_uri = self.info['storagepool']
-        pool_name = pool_name_from_uri(pool_uri)
-        if pool_name not in self._get_all_storagepools_name():
-            invalid['storagepools'] = [pool_name]
+        for disk in self.info['disks']:
+            pool_uri = disk['pool']['name']
+            pool_name = pool_name_from_uri(pool_uri)
+            if pool_name not in self._get_all_storagepools_name():
+                invalid['storagepools'] = [pool_name]
 
         # validate iso integrity
         # FIXME when we support multiples cdrom devices
-- 
2.1.0




More information about the Kimchi-devel mailing list