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@linux.vnet.ibm.com wrote:

From: Harshal Patil <harshalp@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@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