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

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(a)linux.vnet.ibm.com&gt; --- API.json | 12 ---- control/templates.py | 1 - docs/API.md | 3 - i18n.py | 3 +- model/storagepools.py | 10 +-- model/templates.py | 103 ++++++++++++++------------- utils.py | 2 +- vmtemplate.py | 52 ++++++-------- 8 files changed, 83 insertions(+), 103 deletions(-) diff --git a/API.json b/src/wok/plugins/kimchi/API.json index 265ae36..2b64d07 100644 --- a/API.json +++ b/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/control/templates.py b/src/wok/plugins/kimchi/control/templates.py index fc58815..4cd70c2 100644 --- a/control/templates.py +++ b/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/docs/API.md b/src/wok/plugins/kimchi/docs/API.md index 8ef48d6..1c4ee50 100644 --- a/docs/API.md +++ b/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/i18n.py b/src/wok/plugins/kimchi/i18n.py index 7154a62..cf67085 100644 --- a/i18n.py +++ b/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/model/storagepools.py b/src/wok/plugins/kimchi/model/storagepools.py index ec866c5..ddfa7fa 100644 --- a/model/storagepools.py +++ b/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/model/templates.py b/src/wok/plugins/kimchi/model/templates.py index eae2714..7b1412b 100644 --- a/model/templates.py +++ b/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/utils.py b/src/wok/plugins/kimchi/utils.py index 31def2e..a5f5e97 100644 --- a/utils.py +++ b/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/vmtemplate.py b/src/wok/plugins/kimchi/vmtemplate.py index 08adf4c..62d0e1a 100644 --- a/vmtemplate.py +++ b/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