[Kimchi-devel] [PATCH] Issue #992 : Create template on s390x without libvirt storage
Archana Singh
archus at linux.vnet.ibm.com
Tue Aug 30 17:39:15 UTC 2016
Hi Harshal,
Some inline comments below. Please feel free to reply back if it does
not make sense to you.
Thanks,
Archana Singh
On 08/30/2016 06:39 PM, harshalp at linux.vnet.ibm.com wrote:
> From: Harshal Patil <harshalp at linux.vnet.ibm.com>
>
> This patch adds support for creating templates on s390x arch
> without using libvirt related storage calls
>
> Signed-off-by: Harshal Patil <harshalp at linux.vnet.ibm.com>
> ---
> docs/API.md | 2 +
> i18n.py | 2 +
> model/storagepools.py | 15 +++++--
> model/storagevolumes.py | 10 +++--
> model/templates.py | 56 +++++++++++++++++++++-----
> model/vms.py | 38 ++++++++++++------
> osinfo.py | 47 ++++++++++++++++++++--
> utils.py | 13 ++++++
> vmtemplate.py | 105 ++++++++++++++++++++++++++++++------------------
> 9 files changed, 213 insertions(+), 75 deletions(-)
>
> diff --git a/docs/API.md b/docs/API.md
> index 7bd677f..d8d191a 100644
> --- a/docs/API.md
> +++ b/docs/API.md
> @@ -427,6 +427,7 @@ A interface represents available network interface on VM.
> over current will be used exclusively for memory hotplug
> * cdrom: A volume name or URI to an ISO image
> * storagepool: URI of the storagepool where template allocates vm storage.
> + * path : Storage path to store virtual disks without libvirt
> * 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):
> @@ -481,6 +482,7 @@ A interface represents available network interface on VM.
> * format: Format of the image. Valid formats: qcow, qcow2, qed, raw, vmdk, vpc.
> * pool: Storage pool information
> * name: URI of the storagepool where template allocates vm disk.
> + * path (optional): Either pool or path to store the virtual disks should be specified
> * graphics *(optional)*: A dict of graphics paramenters of this template
> * type: The type of graphics. It can be VNC or spice or None.
> * vnc: Graphical display using the Virtual Network
> diff --git a/i18n.py b/i18n.py
> index e8d9c05..8e55e01 100644
> --- a/i18n.py
> +++ b/i18n.py
> @@ -190,6 +190,8 @@ messages = {
> "KCHTMPL0031E": _("Memory value (%(mem)sMiB) must be equal or lesser than maximum memory value (%(maxmem)sMiB)"),
> "KCHTMPL0032E": _("Unable to update template due error: %(err)s"),
> "KCHTMPL0033E": _("Parameter 'disks' requires at least one disk object"),
> + "KCHTMPL0034E": _("Storage without libvirt pool is not supported on this architecture"),
> + "KCHTMPL0035E": _("Error while creating the virtual disk for the guest. Details: %(err)s"),
>
> "KCHPOOL0001E": _("Storage pool %(name)s already exists"),
> "KCHPOOL0002E": _("Storage pool %(name)s does not exist"),
> diff --git a/model/storagepools.py b/model/storagepools.py
> index a2dbaec..cbd756b 100644
> --- a/model/storagepools.py
> +++ b/model/storagepools.py
> @@ -32,7 +32,7 @@ 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
> +from wok.plugins.kimchi.utils import pool_name_from_uri, is_s390x
>
>
> ISO_POOL_NAME = u'kimchi_isos'
> @@ -56,6 +56,7 @@ STORAGE_SOURCES = {'netfs': {'addr': '/pool/source/host/@name',
>
>
> class StoragePoolsModel(object):
> +
> def __init__(self, **kargs):
> self.conn = kargs['conn']
> self.objstore = kargs['objstore']
> @@ -71,6 +72,11 @@ class StoragePoolsModel(object):
> def _check_default_pools(self):
> pools = {}
>
> + if 'disks' not in tmpl_defaults or len(tmpl_defaults['disks']) == 0 \
> + or (not tmpl_defaults.get('disks')[0].get(
> + 'pool') and is_s390x()): # s390x specific handling
Can we have some more details in comments on s390x checks?
> + return
> + tmpl_defaults.get('disks')
> default_pool = tmpl_defaults['disks'][0]['pool']['name']
> default_pool = default_pool.split('/')[-1]
>
> @@ -436,9 +442,10 @@ class StoragePoolModel(object):
> for tmpl in templates:
> t_info = session.get('template', tmpl)
> for disk in t_info['disks']:
> - t_pool = disk['pool']['name']
> - if pool_name_from_uri(t_pool) == pool_name:
> - return True
> + if 'pool' in disk:
> + t_pool = disk['pool']['name']
> + if pool_name_from_uri(t_pool) == pool_name:
> + return True
> return False
>
> def deactivate(self, name):
> diff --git a/model/storagevolumes.py b/model/storagevolumes.py
> index 7b2272b..1e4a2af 100644
> --- a/model/storagevolumes.py
> +++ b/model/storagevolumes.py
> @@ -41,8 +41,7 @@ from wok.plugins.kimchi.kvmusertests import UserTests
> from wok.plugins.kimchi.model.diskutils import get_disk_used_by
> from wok.plugins.kimchi.model.diskutils import set_disk_used_by
> from wok.plugins.kimchi.model.storagepools import StoragePoolModel
> -from wok.plugins.kimchi.utils import get_next_clone_name
> -
> +from wok.plugins.kimchi.utils import get_next_clone_name, is_s390x
>
> VOLUME_TYPE_MAP = {0: 'file',
> 1: 'block',
> @@ -275,8 +274,11 @@ class StorageVolumeModel(object):
> self.task = TaskModel(**kargs)
> self.storagevolumes = StorageVolumesModel(**kargs)
> self.storagepool = StoragePoolModel(**kargs)
> - if self.conn.get() is not None:
> - self.libvirt_user = UserTests().probe_user()
> + if is_s390x(): # s390x specific handling
> + if self.conn.get() is not None:
> + self.libvirt_user = UserTests().probe_user()
> + else:
> + self.libvirt_user = None
> else:
> self.libvirt_user = None
In case of architecture other than s390x also UserTests().probe_user()
is called incase self.conn.get() is not None, otherwise
self.libvirt_user is assigned to None. Any specific reason of having
architecture check and doing the same thing?
>
> diff --git a/model/templates.py b/model/templates.py
> index a299c85..4edd98e 100644
> --- a/model/templates.py
> +++ b/model/templates.py
> @@ -28,7 +28,8 @@ import urlparse
>
> from wok.exception import InvalidOperation, InvalidParameter
> from wok.exception import NotFoundError, OperationFailed
> -from wok.utils import probe_file_permission_as_user, run_setfacl_set_attr
> +from wok.utils import probe_file_permission_as_user, run_command
> +from wok.utils import run_setfacl_set_attr
> from wok.xmlutils.utils import xpath_get_text
>
> from wok.plugins.kimchi.config import get_kimchi_version
> @@ -47,6 +48,7 @@ if os.uname()[4] in ['ppc', 'ppc64', 'ppc64le']:
>
>
> class TemplatesModel(object):
> +
> def __init__(self, **kargs):
> self.objstore = kargs['objstore']
> self.conn = kargs['conn']
> @@ -142,7 +144,7 @@ class TemplatesModel(object):
> get_kimchi_version())
> except InvalidOperation:
> raise
> - except Exception, e:
> + except Exception as e:
> raise OperationFailed('KCHTMPL0020E', {'err': e.message})
>
> return name
> @@ -171,6 +173,7 @@ class TemplatesModel(object):
>
>
> class TemplateModel(object):
> +
> def __init__(self, **kargs):
> self.objstore = kargs['objstore']
> self.conn = kargs['conn']
> @@ -198,10 +201,9 @@ class TemplateModel(object):
> # set default name
> subfixs = [v[len(name):] for v in self.templates.get_list()
> if v.startswith(name)]
> - indexs = [int(v.lstrip("-clone")) for v in subfixs
> - if v.startswith("-clone") and
> - v.lstrip("-clone").isdigit()]
> - indexs.sort()
> + indexs = sorted([int(v.lstrip("-clone")) for v in subfixs
> + if v.startswith("-clone") and
> + v.lstrip("-clone").isdigit()])
> index = "1" if not indexs else str(indexs[-1] + 1)
> clone_name = name + "-clone" + index
>
> @@ -267,7 +269,7 @@ class TemplateModel(object):
>
> except InvalidOperation:
> raise
> - except Exception, e:
> + except Exception as e:
> raise OperationFailed('KCHTMPL0032E', {'err': e.message})
>
> return params['name']
> @@ -290,7 +292,7 @@ def validate_memory(memory):
> # memory limit
> if (current > (MAX_MEM_LIM >> 10)) or (maxmem > (MAX_MEM_LIM >> 10)):
> raise InvalidParameter("KCHVM0079E",
> - {'value': str(MAX_MEM_LIM / (1024**3))})
> + {'value': str(MAX_MEM_LIM / (1024 ** 3))})
> if (current > host_memory) or (maxmem > host_memory):
> raise InvalidParameter("KCHVM0078E", {'memHost': host_memory})
>
> @@ -316,6 +318,7 @@ def validate_memory(memory):
>
>
> class LibvirtVMTemplate(VMTemplate):
> +
> def __init__(self, args, scan=False, conn=None):
> self.conn = conn
> netboot = True if 'netboot' in args.keys() else False
> @@ -398,16 +401,47 @@ class LibvirtVMTemplate(VMTemplate):
> raise NotFoundError("KCHVOL0002E", {'name': vol,
> 'pool': pool})
>
> + def _create_disk_image(self, format_type, path, capacity):
This should not be part of libvirtVMTemplate class as name says I guess
we should not have any non libvirt related handling here. What do you think?
> + """
> + Create a disk image for the Guest
> + Args:
> + format: Format of the storage. e.g. qcow2
> + path: Path where the virtual disk will be created
> + capacity: Capacity of the virtual disk in GBs
> +
> + Returns:
> +
> + """
> + out, err, rc = run_command(
> + ["/usr/bin/qemu-img", "create", "-f", format_type, "-o",
> + "preallocation=metadata", path, str(capacity) + "G"])
str(capacity) will throw error in case of unicode characters. I will
suggest to ensure that its integer and use encode or decode methods to
do str conversion.
Does wok_log is taken care inside run_command, if not we need to take
care of that also.
> +
> + if rc != 0:
> + raise OperationFailed("KCHTMPL0035E", {'err': err})
> +
> + return
> +
> def fork_vm_storage(self, vm_uuid):
> # Provision storages:
> vol_list = self.to_volume_list(vm_uuid)
> try:
> for v in vol_list:
> - pool = self._get_storage_pool(v['pool'])
> - # outgoing text to libvirt, encode('utf-8')
> - pool.createXML(v['xml'].encode('utf-8'), 0)
> + if v['pool'] is not None:
If 'pool' key will not present, it will throw key error. Is this
expected to be the behaviour?
Are we saying that key should be there but value can be None?
> + pool = self._get_storage_pool(v['pool'])
> + # outgoing text to libvirt, encode('utf-8')
> + pool.createXML(v['xml'].encode('utf-8'), 0)
> + else:
> + capacity = v['capacity']
> + format_type = v['format']
> + path = v['path']
> + self._create_disk_image(
> + format_type=format_type,
> + path=path,
> + capacity=capacity)
I think as name suggested to_volume_list, it should look correct from
naming point of view as iterating to volume and handing non volume
related stuff. What do you think?
> +
> except libvirt.libvirtError as e:
> raise OperationFailed("KCHVMSTOR0008E", {'error': e.message})
> +
> return vol_list
>
> def set_cpu_info(self):
> diff --git a/model/vms.py b/model/vms.py
> index 3380278..5d1eebb 100644
> --- a/model/vms.py
> +++ b/model/vms.py
> @@ -60,13 +60,13 @@ from wok.plugins.kimchi.model.utils import remove_metadata_node
> from wok.plugins.kimchi.model.utils import set_metadata_node
> from wok.plugins.kimchi.osinfo import defaults, MEM_DEV_SLOTS
> from wok.plugins.kimchi.screenshot import VMScreenshot
> -from wok.plugins.kimchi.utils import get_next_clone_name
> +from wok.plugins.kimchi.utils import get_next_clone_name, is_s390x
> from wok.plugins.kimchi.utils import template_name_from_uri
> from wok.plugins.kimchi.xmlutils.bootorder import get_bootorder_node
> from wok.plugins.kimchi.xmlutils.bootorder import get_bootmenu_node
> from wok.plugins.kimchi.xmlutils.cpu import get_topology_xml
> from wok.plugins.kimchi.xmlutils.disk import get_vm_disk_info, get_vm_disks
> -from utils import has_cpu_numa, set_numa_memory
> +from .utils import has_cpu_numa, set_numa_memory
>
>
> DOM_STATE_MAP = {0: 'nostate',
> @@ -114,6 +114,7 @@ vm_locks = {}
>
>
> class VMsModel(object):
> +
> def __init__(self, **kargs):
> self.conn = kargs['conn']
> self.objstore = kargs['objstore']
> @@ -231,6 +232,7 @@ class VMsModel(object):
>
>
> class VMModel(object):
> +
> def __init__(self, **kargs):
> self.conn = kargs['conn']
> self.objstore = kargs['objstore']
> @@ -373,7 +375,7 @@ class VMModel(object):
> vir_dom = self.get_vm(name, self.conn)
> flags = libvirt.VIR_DOMAIN_XML_SECURE
> xml = vir_dom.XMLDesc(flags).decode('utf-8')
> - except libvirt.libvirtError, e:
> + except libvirt.libvirtError as e:
> raise OperationFailed('KCHVM0035E', {'name': name,
> 'err': e.message})
>
> @@ -407,7 +409,7 @@ class VMModel(object):
> vir_conn = self.conn.get()
> dom = vir_conn.defineXML(xml)
> self._update_metadata_name(dom, nonascii_name)
> - except libvirt.libvirtError, e:
> + except libvirt.libvirtError as e:
> raise OperationFailed('KCHVM0035E', {'name': name,
> 'err': e.message})
>
> @@ -480,7 +482,7 @@ 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:
> + except libvirt.libvirtError as e:
> raise OperationFailed('KCHVM0035E', {'name': domain_name,
> 'err': e.message})
>
> @@ -1366,7 +1368,7 @@ class VMModel(object):
> # "OperationFailed" in that case.
> try:
> snapshot_names = self.vmsnapshots.get_list(name)
> - except OperationFailed, e:
> + except OperationFailed as e:
> wok_log.error('cannot list snapshots: %s; '
> 'skipping snapshot deleting...' % e.message)
> else:
> @@ -1394,6 +1396,14 @@ class VMModel(object):
> except libvirt.libvirtError as e:
> wok_log.error('Unable to get storage volume by path: %s' %
> e.message)
Below changes will not get called if libvirtError will be thrown for the
path given. I guess we need to ensure that even if libvirt pool and
volume does not exit for path provided below gets executed for s390x.
> + try:
> + if is_s390x(): # s390x specific handling
> + if os.path.exists(path):
> + os.remove(path)
> + except Exception as e:
> + wok_log.error('Unable to delete storage path: %s' %
> + e.message)
> +
> except Exception as e:
> raise OperationFailed('KCHVOL0017E', {'err': e.message})
>
> @@ -1538,7 +1548,7 @@ class VMModel(object):
> raise OperationFailed("KCHVM0081E",
> {'dir': serialconsole.BASE_DIRECTORY})
>
> - websocket.add_proxy_token(name.encode('utf-8')+'-console',
> + websocket.add_proxy_token(name.encode('utf-8') + '-console',
> os.path.join(serialconsole.BASE_DIRECTORY,
> name.encode('utf-8')), True)
>
> @@ -1596,7 +1606,7 @@ class VMModel(object):
>
> try:
> vir_dom.suspend()
> - except libvirt.libvirtError, e:
> + except libvirt.libvirtError as e:
> raise OperationFailed('KCHVM0038E', {'name': name,
> 'err': e.message})
>
> @@ -1617,7 +1627,7 @@ class VMModel(object):
>
> try:
> vir_dom.resume()
> - except libvirt.libvirtError, e:
> + except libvirt.libvirtError as e:
> raise OperationFailed('KCHVM0040E', {'name': name,
> 'err': e.message})
>
> @@ -1657,7 +1667,7 @@ class VMModel(object):
> 'destarch': dest_arch
> }
> )
> - except Exception, e:
> + except Exception as e:
> raise OperationFailed("KCHVM0066E", {'error': e.message})
>
> finally:
> @@ -1768,7 +1778,7 @@ class VMModel(object):
> ssh_client,
> id_rsa_data
> )
> - except Exception, e:
> + except Exception as e:
> raise OperationFailed(
> "KCHVM0068E",
> {'host': remote_host, 'user': user, 'error': e.message}
> @@ -1833,7 +1843,7 @@ class VMModel(object):
> conn = self.conn.get()
> vol_obj = conn.storageVolLookupByPath(disk_path)
> return vol_obj.info()[1]
> - except Exception, e:
> + except Exception as e:
> raise OperationFailed(
> "KCHVM0062E",
> {'path': disk_path, 'error': e.message}
> @@ -1949,6 +1959,7 @@ class VMModel(object):
>
>
> class VMScreenshotModel(object):
> +
> def __init__(self, **kargs):
> self.objstore = kargs['objstore']
> self.conn = kargs['conn']
> @@ -1995,6 +2006,7 @@ class VMScreenshotModel(object):
>
>
> class LibvirtVMScreenshot(VMScreenshot):
> +
> def __init__(self, vm_uuid, conn):
> VMScreenshot.__init__(self, vm_uuid)
> self.conn = conn
> @@ -2004,7 +2016,7 @@ class LibvirtVMScreenshot(VMScreenshot):
> fd = opaque
> os.write(fd, buf)
>
> - fd = os.open(thumbnail, os.O_WRONLY | os.O_TRUNC | os.O_CREAT, 0644)
> + fd = os.open(thumbnail, os.O_WRONLY | os.O_TRUNC | os.O_CREAT, 0o644)
> try:
> conn = self.conn.get()
> dom = conn.lookupByUUIDString(self.vm_uuid)
> diff --git a/osinfo.py b/osinfo.py
> index 2c59312..524b3bd 100644
> --- a/osinfo.py
> +++ b/osinfo.py
> @@ -146,6 +146,12 @@ def _get_tmpl_defaults():
> 'maxmemory': _get_default_template_mem()}
> tmpl_defaults['storage']['disk.0'] = {'size': 10, 'format': 'qcow2',
> 'pool': 'default'}
> + is_on_s390x = True if _get_arch() == 's390x' else False
> +
> + if is_on_s390x:
> + tmpl_defaults['storage']['disk.0']['path'] = '/var/lib/libvirt/images/'
> + del tmpl_defaults['storage']['disk.0']['pool']
> +
> tmpl_defaults['processor']['vcpus'] = 1
> tmpl_defaults['processor']['maxvcpus'] = 1
> tmpl_defaults['graphics'] = {'type': 'vnc', 'listen': '127.0.0.1'}
> @@ -153,7 +159,12 @@ def _get_tmpl_defaults():
> default_config = ConfigObj(tmpl_defaults)
>
> # Load template configuration file
> - config_file = os.path.join(kimchiPaths.sysconf_dir, 'template.conf')
> + if is_on_s390x:
> + config_file = os.path.join(
> + kimchiPaths.sysconf_dir,
> + 'template_s390x.conf')
Not sure, but should we handle scenario where in case s390x specific
arch conf file is not present just go with template.conf?
> + else:
> + config_file = os.path.join(kimchiPaths.sysconf_dir, 'template.conf')
> config = ConfigObj(config_file)
>
> # Merge default configuration with file configuration
> @@ -174,11 +185,39 @@ def _get_tmpl_defaults():
> # Parse storage section to get disks values
> storage_section = default_config.pop('storage')
> defaults['disks'] = []
> - for disk in storage_section.keys():
> +
> + pool_exists = False
> + path_exists = False
> + for index, disk in enumerate(storage_section.keys()):
> data = storage_section[disk]
> data['index'] = int(disk.split('.')[1])
> - data['pool'] = {"name": '/plugins/kimchi/storagepools/' +
> - storage_section[disk].pop('pool')}
> + # Right now 'Path' is only supported on s390x
> + if storage_section[disk].get('path') and is_on_s390x:
> + path_exists = True
> + data['path'] = storage_section[disk].pop('path')
> + if 'size' not in storage_section[disk]:
> + data['size'] = tmpl_defaults['storage']['disk.0']['size']
> + else:
> + data['size'] = storage_section[disk].pop('size')
> +
> + if 'format' not in storage_section[disk]:
> + data['format'] = tmpl_defaults['storage']['disk.0']['format']
> + else:
> + data['format'] = storage_section[disk].pop('format')
> +
> + if storage_section[disk].get('pool'):
> + pool_exists = True
> + data['pool'] = {"name": '/plugins/kimchi/storagepools/' +
> + storage_section[disk].pop('pool')}
For one disk either it can be pool or path, both should not be added. Do
you agree?
I guess getting pool should be in else of first if.
> +
> + # If both pool and path don't exist, pick the defaults
> + if index == len(storage_section.keys()) - 1 and \
> + (not pool_exists and not path_exists):
> + if is_on_s390x: # Special handling for s390x
> + data['path'] = tmpl_defaults['storage']['disk.0']['path']
> + else:
> + data['pool'] = {"name": '/plugins/kimchi/storagepools/default'}
> +
> defaults['disks'].append(data)
>
> # Parse processor section to get vcpus and cpu_topology values
> diff --git a/utils.py b/utils.py
> index 26d3cf6..b8f81ca 100644
> --- a/utils.py
> +++ b/utils.py
> @@ -24,6 +24,7 @@ import platform
> import re
> import sqlite3
> import time
> +import os
> import urllib2
> from httplib import HTTPConnection, HTTPException
> from urlparse import urlparse
> @@ -272,3 +273,15 @@ def is_libvirtd_up():
>
> output, error, rc = run_command(cmd, silent=True)
> return True if output == 'active\n' else False
> +
> +
> +def is_s390x():
> + """
> + Check if current arch is 's390x'
> + Returns:
> +
> + """
do we need new line in comment?
> + if os.uname()[4] == 's390x':
> + return True
> +
> + return False
> diff --git a/vmtemplate.py b/vmtemplate.py
> index 7ac0541..c0d51a5 100644
> --- a/vmtemplate.py
> +++ b/vmtemplate.py
> @@ -31,7 +31,8 @@ from wok.exception import MissingParameter, OperationFailed
> from wok.plugins.kimchi import imageinfo
> from wok.plugins.kimchi import osinfo
> from wok.plugins.kimchi.isoinfo import IsoImage
> -from wok.plugins.kimchi.utils import check_url_path, pool_name_from_uri
> +from wok.plugins.kimchi.utils import check_url_path, is_s390x
> +from wok.plugins.kimchi.utils import pool_name_from_uri
> from wok.plugins.kimchi.xmlutils.bootorder import get_bootorder_xml
> from wok.plugins.kimchi.xmlutils.cpu import get_cpu_xml
> from wok.plugins.kimchi.xmlutils.disk import get_disk_xml
> @@ -42,6 +43,7 @@ from wok.plugins.kimchi.xmlutils.serial import get_serial_xml
>
>
> class VMTemplate(object):
> +
> def __init__(self, args, scan=False, netboot=False):
> """
> Construct a VM Template from a widely variable amount of information.
> @@ -95,35 +97,49 @@ class VMTemplate(object):
> disks = self.info.get('disks')
>
> basic_disk = ['index', 'format', 'pool', 'size']
> + basic_path_disk = ['index', 'format', 'path', 'size']
> ro_disk = ['index', 'format', 'pool', 'volume']
> base_disk = ['index', 'base', 'pool', 'size', 'format']
>
> for index, disk in enumerate(disks):
> disk_info = dict(default_disk)
> -
> - pool = disk.get('pool', default_disk['pool'])
> - pool_type = self._get_storage_type(pool['name'])
> -
> - if pool_type in ['iscsi', 'scsi']:
> - disk_info = {'index': 0, 'format': 'raw', 'volume': None}
> -
> - disk_info.update(disk)
> - pool_name = disk_info.get('pool', {}).get('name')
> - if pool_name is None:
> - raise MissingParameter('KCHTMPL0028E')
> -
> - keys = sorted(disk_info.keys())
> - if ((keys != sorted(basic_disk)) and (keys != sorted(ro_disk)) and
> - (keys != sorted(base_disk))):
> - raise MissingParameter('KCHTMPL0028E')
> -
> - if pool_type in ['logical', 'iscsi', 'scsi']:
> - if disk_info['format'] != 'raw':
> - raise InvalidParameter('KCHTMPL0029E')
> -
> - disk_info['pool']['type'] = pool_type
> - disk_info['index'] = disk_info.get('index', index)
> - self.info['disks'][index] = disk_info
> + if disk.get('pool'):
> + pool = disk.get('pool', default_disk.get('pool'))
> + pool_type = self._get_storage_type(pool['name'])
> + if pool_type in ['iscsi', 'scsi']:
> + disk_info = {'index': 0, 'format': 'raw', 'volume': None}
> +
> + disk_info.update(disk)
> + pool_name = disk_info.get('pool', {}).get('name')
> + if pool_name is None:
> + raise MissingParameter('KCHTMPL0028E')
> +
> + keys = sorted(disk_info.keys())
> + if ((keys != sorted(basic_disk)) and
> + (keys != sorted(ro_disk)) and
> + (keys != sorted(base_disk)) and
> + (keys != basic_path_disk)):
> + raise MissingParameter('KCHTMPL0028E')
> +
> + if pool_type in ['logical', 'iscsi', 'scsi']:
> + if disk_info['format'] != 'raw':
> + raise InvalidParameter('KCHTMPL0029E')
> +
> + disk_info['pool']['type'] = pool_type
> + disk_info['index'] = disk_info.get('index', index)
> + self.info['disks'][index] = disk_info
> + else:
> + if is_s390x(): # For now support 'path' only on s390x
any specific reason of keeping if in side else and not just elif?
> + path = disk.get('path', default_disk['path'])
> + disk_info.update(disk)
> + keys = sorted(disk_info.keys())
> + if keys != sorted(basic_path_disk):
> + raise MissingParameter('KCHTMPL0028E')
i18n message has to be changed to convey that either pool or path has to
be given in a disk, as current is "/KCHTMPL0028E/": _("When setting
template disks, following parameters are required: 'index', 'pool name',
'format', 'size' or 'volume' (for scsi/iscsi pools)")
> + disk_info['path'] = path
> + disk_info['index'] = disk_info.get('index', index)
> + self.info['disks'][index] = disk_info
> + else:
> + raise InvalidParameter('KCHTMPL0034E')
>
> def _get_os_info(self, args, scan):
> distro = version = 'unknown'
> @@ -217,8 +233,9 @@ class VMTemplate(object):
> params = dict(base_disk_params)
> params['format'] = disk['format']
> params['index'] = index
> - params.update(locals().get('%s_disk_params' %
> - disk['pool']['type'], {}))
> + if disk.get('pool'):
> + params.update(locals().get('%s_disk_params' %
> + disk['pool']['type'], {}))
>
> volume = disk.get('volume')
> if volume is not None:
> @@ -226,9 +243,13 @@ class VMTemplate(object):
> volume)
> else:
> img = "%s-%s.img" % (vm_uuid, params['index'])
> - storage_path = self._get_storage_path(disk['pool']['name'])
> + if disk.get('pool'):
> + storage_path = self._get_storage_path(disk['pool']['name'])
> + params['pool_type'] = disk['pool']['type']
> + elif disk.get('path'):
> + storage_path = disk.get('path')
> + params['pool_type'] = None
> params['path'] = os.path.join(storage_path, img)
> - params['pool_type'] = disk['pool']['type']
> disks_xml += get_disk_xml(params)[1]
>
> return unicode(disks_xml, 'utf-8')
> @@ -237,20 +258,24 @@ class VMTemplate(object):
> ret = []
> for i, d in enumerate(self.info['disks']):
> # Create only .img. If storagepool is (i)SCSI, volumes will be LUNs
> - if d['pool']['type'] in ["iscsi", "scsi"]:
> + if not is_s390x() and d['pool']['type'] in ["iscsi", "scsi"]:
> continue
in case of s390x also if pool is present and type is iscsi or scsi we
should continue.
so the check should be:- in case of arch is 390x if pool is not present
or if pool of type iscsi or scsi just continue. Or can have for all arch
also that if pool not present or if pool type is scsi or iscsi just
continue.
@Aline: What do you think, pool not present just continue can be true
for all arch. Or you think we should get key error in case of non s390x
arch?
>
> index = d.get('index', i)
> volume = "%s-%s.img" % (vm_uuid, index)
>
> - storage_path = self._get_storage_path(d['pool']['name'])
> + if 'path' in d:
> + storage_path = d['path']
> + else:
> + storage_path = self._get_storage_path(d['pool']['name'])
> +
I guess as this method is meant for libvirt volume listing, I guess we
should restructure.
May be one method to deal with pool and vol, and another to deal with
path, and use both while iterating through list of disks.
> info = {'name': volume,
> 'capacity': d['size'],
> 'format': d['format'],
> 'path': '%s/%s' % (storage_path, volume),
> - 'pool': d['pool']['name']}
> + 'pool': d['pool']['name'] if 'pool' in d else None}
>
> - if 'logical' == d['pool']['type'] or \
> + if ('pool' in d and 'logical' == d['pool']['type']) or \
> info['format'] not in ['qcow2', 'raw']:
> info['allocation'] = info['capacity']
> else:
> @@ -444,8 +469,9 @@ class VMTemplate(object):
>
> def validate(self):
> for disk in self.info.get('disks'):
> - pool_uri = disk.get('pool', {}).get('name')
> - self._get_storage_pool(pool_uri)
> + if not is_s390x(): # s390x specific handling
> + pool_uri = disk.get('pool', {}).get('name')
> + self._get_storage_pool(pool_uri)
Again in case of s390x also if pool is present, it should validate it.
> self._network_validate()
> self._iso_validate()
> self.cpuinfo_validate()
> @@ -494,10 +520,11 @@ class VMTemplate(object):
>
> # validate storagepools and image-based templates integrity
> 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_active_storagepools_name():
> - invalid['storagepools'] = [pool_name]
> + if 'pool' in disk:
> + pool_uri = disk['pool']['name']
> + pool_name = pool_name_from_uri(pool_uri)
> + if pool_name not in self._get_active_storagepools_name():
> + invalid['storagepools'] = [pool_name]
>
> if disk.get("base") is None:
> continue
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ovirt.org/pipermail/kimchi-devel/attachments/20160830/1eb58b3d/attachment.html>
More information about the Kimchi-devel
mailing list