<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<p>Hi Harshal,</p>
<p>Some inline comments below. Please feel free to reply back if it
does not make sense to you.<br>
</p>
<p>Thanks,</p>
<p>Archana Singh<br>
</p>
<div class="moz-cite-prefix">On 08/30/2016 06:39 PM,
<a class="moz-txt-link-abbreviated" href="mailto:harshalp@linux.vnet.ibm.com">harshalp@linux.vnet.ibm.com</a> wrote:<br>
</div>
<blockquote
cite="mid:1472562570-25225-1-git-send-email-harshalp@linux.vnet.ibm.com"
type="cite">
<pre wrap="">From: Harshal Patil <a class="moz-txt-link-rfc2396E" href="mailto:harshalp@linux.vnet.ibm.com"><harshalp@linux.vnet.ibm.com></a>
This patch adds support for creating templates on s390x arch
without using libvirt related storage calls
Signed-off-by: Harshal Patil <a class="moz-txt-link-rfc2396E" href="mailto:harshalp@linux.vnet.ibm.com"><harshalp@linux.vnet.ibm.com></a>
---
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</pre>
</blockquote>
Can we have some more details in comments on s390x checks?<br>
<blockquote
cite="mid:1472562570-25225-1-git-send-email-harshalp@linux.vnet.ibm.com"
type="cite">
<pre wrap="">
+ 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</pre>
</blockquote>
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?<br>
<blockquote
cite="mid:1472562570-25225-1-git-send-email-harshalp@linux.vnet.ibm.com"
type="cite">
<pre wrap="">
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):</pre>
</blockquote>
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?<br>
<blockquote
cite="mid:1472562570-25225-1-git-send-email-harshalp@linux.vnet.ibm.com"
type="cite">
<pre wrap="">
+ """
+ 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"])</pre>
</blockquote>
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.<br>
Does wok_log is taken care inside run_command, if not we need to
take care of that also.<br>
<br>
<blockquote
cite="mid:1472562570-25225-1-git-send-email-harshalp@linux.vnet.ibm.com"
type="cite">
<pre wrap="">
+
+ 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:</pre>
</blockquote>
If 'pool' key will not present, it will throw key error. Is this
expected to be the behaviour?<br>
Are we saying that key should be there but value can be None?<br>
<blockquote
cite="mid:1472562570-25225-1-git-send-email-harshalp@linux.vnet.ibm.com"
type="cite">
<pre wrap="">
+ 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)</pre>
</blockquote>
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?<br>
<blockquote
cite="mid:1472562570-25225-1-git-send-email-harshalp@linux.vnet.ibm.com"
type="cite">
<pre wrap="">
+
except libvirt.libvirtError as e:
raise OperationFailed("KCHVMSTOR0008E<a class="moz-txt-link-rfc2396E" href="mailto:,{'error':e.message})+returnvol_listdefset_cpu_info(self):diff--gita/model/vms.pyb/model/vms.pyindex3380278..5d1eebb100644---a/model/vms.py+++b/model/vms.py@@-60,13+60,13@@fromwok.plugins.kimchi.model.utilsimportremove_metadata_nodefromwok.plugins.kimchi.model.utilsimportset_metadata_nodefromwok.plugins.kimchi.osinfoimportdefaults,MEM_DEV_SLOTSfromwok.plugins.kimchi.screenshotimportVMScreenshot-fromwok.plugins.kimchi.utilsimportget_next_clone_name+fromwok.plugins.kimchi.utilsimportget_next_clone_name,is_s390xfromwok.plugins.kimchi.utilsimporttemplate_name_from_urifromwok.plugins.kimchi.xmlutils.bootorderimportget_bootorder_nodefromwok.plugins.kimchi.xmlutils.bootorderimportget_bootmenu_nodefromwok.plugins.kimchi.xmlutils.cpuimportget_topology_xmlfromwok.plugins.kimchi.xmlutils.diskimportget_vm_disk_info,get_vm_disks-fromutilsimporthas_cpu_numa,set_numa_memory+from.utilsimporthas_cpu_numa,set_numa_memoryDOM_STATE_MAP={0:'nostate',@@-114,6+114,7@@vm_locks={}classVMsModel(object):+def__init__(self,**kargs):self.conn=kargs['conn']self.objstore=kargs['objstore']@@-231,6+232,7@@classVMsModel(object):classVMModel(object):+def__init__(self,**kargs):self.conn=kargs['conn']self.objstore=kargs['objstore']@@-373,7+375,7@@classVMModel(object):vir_dom=self.get_vm(name,self.conn)flags=libvirt.VIR_DOMAIN_XML_SECURExml=vir_dom.XMLDesc(flags).decode('utf-8')-exceptlibvirt.libvirtError,e:+exceptlibvirt.libvirtErrorase:raiseOperationFailed('KCHVM0035E',{'name':name,'err':e.message})@@-407,7+409,7@@classVMModel(object):vir_conn=self.conn.get()dom=vir_conn.defineXML(xml)self._update_metadata_name(dom,nonascii_name)-exceptlibvirt.libvirtError,e:+exceptlibvirt.libvirtErrorase:raiseOperationFailed('KCHVM0035E',{'name':name,'err':e.message})@@-480,7+482,7@@classVMModel(object):orig_pool_name=vir_pool.name().decode('utf-8')orig_vol_name=vir_orig_vol.name().decode('utf-8')-exceptlibvirt.libvirtError,e:+exceptlibvirt.libvirtErrorase:raiseOperationFailed('KCHVM0035E',{'name':domain_name,'err':e.message})@@-1366,7+1368,7@@classVMModel(object):#">", {'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):
# "</a>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)</pre>
</blockquote>
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.<br>
<blockquote
cite="mid:1472562570-25225-1-git-send-email-harshalp@linux.vnet.ibm.com"
type="cite">
<pre wrap="">
+ 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')</pre>
</blockquote>
Not sure, but should we handle scenario where in case s390x specific
arch conf file is not present just go with template.conf?<br>
<blockquote
cite="mid:1472562570-25225-1-git-send-email-harshalp@linux.vnet.ibm.com"
type="cite">
<pre wrap="">
+ 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')}</pre>
</blockquote>
For one disk either it can be pool or path, both should not be
added. Do you agree?<br>
I guess getting pool should be in else of first if.<br>
<blockquote
cite="mid:1472562570-25225-1-git-send-email-harshalp@linux.vnet.ibm.com"
type="cite">
<pre wrap="">
+
+ # 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:
+
+ """</pre>
</blockquote>
do we need new line in comment?<br>
<blockquote
cite="mid:1472562570-25225-1-git-send-email-harshalp@linux.vnet.ibm.com"
type="cite">
<pre wrap="">
+ 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</pre>
</blockquote>
any specific reason of keeping if in side else and not just elif?<br>
<blockquote
cite="mid:1472562570-25225-1-git-send-email-harshalp@linux.vnet.ibm.com"
type="cite">
<pre wrap="">
+ 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')</pre>
</blockquote>
i18n message has to be changed to convey that either pool or path
has to be given in a disk, as current is <span class="pl-s"><span
class="pl-pds">"</span><em>KCHTMPL0028E</em><span class="pl-pds">"</span></span>:
_(<span class="pl-s"><span class="pl-pds">"</span>When setting
template disks, following parameters are required: 'index', 'pool
name', 'format', 'size' or 'volume' (for scsi/iscsi pools)<span
class="pl-pds">"</span></span>)<br>
<br>
<blockquote
cite="mid:1472562570-25225-1-git-send-email-harshalp@linux.vnet.ibm.com"
type="cite">
<pre wrap="">
+ 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</pre>
</blockquote>
in case of s390x also if pool is present and type is iscsi or scsi
we should continue.<br>
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.<br>
@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?<br>
<blockquote
cite="mid:1472562570-25225-1-git-send-email-harshalp@linux.vnet.ibm.com"
type="cite">
<pre wrap="">
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'])
+</pre>
</blockquote>
I guess as this method is meant for libvirt volume listing, I guess
we should restructure.<br>
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.<br>
<blockquote
cite="mid:1472562570-25225-1-git-send-email-harshalp@linux.vnet.ibm.com"
type="cite">
<pre wrap="">
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)</pre>
</blockquote>
Again in case of s390x also if pool is present, it should validate
it. <br>
<blockquote
cite="mid:1472562570-25225-1-git-send-email-harshalp@linux.vnet.ibm.com"
type="cite">
<pre wrap="">
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
</pre>
</blockquote>
<br>
</body>
</html>