Hi,
Just some style suggestions.
on 2014/10/14 06:54, Christy Perez wrote:
In order to allow a guest to use SMT/hyperthreading, we should
enable passing in of the sockets, cores, and threads values when
creating a template.
All three values must be specified, as per the topology descr at
http://libvirt.org/formatdomain.html#elementsCPU
v3->v4:
- Remove the unused cpu_ elements from common_spec
- Pass new_t into validate function to reduce complexity
- Rearrange code to decrese indents in _get_cpu_xml
v2->v3:
- Set vcpus based on topology, if specified.
- Move the update cpu+topology validation out to a function
for redability
- Add a minimum value of 1 for topology values
- Leave new English error msg as empty string
- Update the API documentation on cpu defaults
v1->v2:
- Added a check to make sure that vcpus = sockets*cores*threads
- Set individual topoology params to required in API.json
- Change the topology object types from string to integer
- Always return cpu_info from templates lookup()
- Removed check for cpu_info in to_vm_xml
- Build cpu_info xml using lxml.builder instead of string
- CPU and topology verification on template update
Signed-off-by: Christy Perez <christy(a)linux.vnet.ibm.com>
---
docs/API.md | 13 ++++++++++++-
po/en_US.po | 3 +++
po/pt_BR.po | 3 +++
po/zh_CN.po | 3 +++
src/kimchi/API.json | 33 +++++++++++++++++++++++++++++++--
src/kimchi/control/templates.py | 13 ++++++++++---
src/kimchi/i18n.py | 1 +
src/kimchi/model/templates.py | 32 ++++++++++++++++++++++++++++++++
src/kimchi/osinfo.py | 5 ++---
src/kimchi/vmtemplate.py | 16 ++++++++++++++++
10 files changed, 113 insertions(+), 9 deletions(-)
diff --git a/docs/API.md b/docs/API.md
index 92fbbd5..6984649 100644
--- a/docs/API.md
+++ b/docs/API.md
@@ -194,7 +194,9 @@ Represents a snapshot of the Virtual Machine's primary monitor.
* name: The name of the Template. Used to identify the Template in this API
* os_distro *(optional)*: The operating system distribution
* os_version *(optional)*: The version of the operating system distribution
- * cpus *(optional)*: The number of CPUs assigned to the VM. Default is 1.
+ * cpus *(optional)*: The number of CPUs assigned to the VM.
+ Default is 1, unlees specifying a cpu topology. In that case, cpus
+ will default to a product of the topology values (see cpu_info).
* memory *(optional)*: The amount of memory assigned to the VM.
Default is 1024M.
* cdrom *(optional)*: A volume name or URI to an ISO image.
@@ -216,6 +218,15 @@ Represents a snapshot of the Virtual Machine's primary monitor.
Independent Computing Environments
* null: Graphics is disabled or type not supported
* listen: The network which the vnc/spice server listens on.
+ * cpu_info *(optional)*: CPU-specific information.
+ * topology: Specify sockets, threads, and cores to run the virtual CPU
+ threads on.
+ All three are required in order to specify cpu topology.
+ * sockets - The number of sockets to use.
+ * cores - The number of cores per socket.
+ * threads - The number of threads per core.
+ If specifying both cpus and CPU topology, make sure cpus is
+ equal to the product of sockets, cores, and threads.
### Sub-Collection: Virtual Machine Network Interfaces
diff --git a/po/en_US.po b/po/en_US.po
index eb571ca..a88675c 100644
--- a/po/en_US.po
+++ b/po/en_US.po
@@ -371,6 +371,9 @@ msgstr ""
msgid "Cannot identify base image %(path)s format"
msgstr ""
+msgid "When specifying CPU topology, VCPUs must be a product of sockets, cores, and
threads."
+msgstr ""
+
#, python-format
msgid "Storage pool %(name)s already exists"
msgstr ""
diff --git a/po/pt_BR.po b/po/pt_BR.po
index f2fba64..1bebe30 100644
--- a/po/pt_BR.po
+++ b/po/pt_BR.po
@@ -424,6 +424,9 @@ msgstr "Imagem base do modelo deve ser um arquivo de imagem
local válido"
msgid "Cannot identify base image %(path)s format"
msgstr "Não foi possível identificar o formato da imagem base %(path)s"
+msgid "When specifying CPU topology, VCPUs must be a product of sockets, cores, and
threads."
+msgstr ""
+
#, python-format
msgid "Storage pool %(name)s already exists"
msgstr "Storage pool %(name)s já existe"
diff --git a/po/zh_CN.po b/po/zh_CN.po
index f77e405..f944b8d 100644
--- a/po/zh_CN.po
+++ b/po/zh_CN.po
@@ -392,6 +392,9 @@ msgstr "模板基础镜像必须为一个有效的本地镜像文件"
msgid "Cannot identify base image %(path)s format"
msgstr "未能识别基础镜像%(path)s格式"
+msgid "When specifying CPU topology, VCPUs must be a product of sockets, cores, and
threads."
+msgstr ""
+
#, python-format
msgid "Storage pool %(name)s already exists"
msgstr "存储池%(name)s已经存在"
diff --git a/src/kimchi/API.json b/src/kimchi/API.json
index d9e13f0..d3edf27 100644
--- a/src/kimchi/API.json
+++ b/src/kimchi/API.json
@@ -26,6 +26,33 @@
]
}
}
+ },
+ "cpu_info": {
+ "description": "Configure CPU specifics for a VM.",
+ "type": "object",
+ "properties": {
+ "topology": {
+ "description": "Configure the guest CPU
topology.",
+ "type": "object",
+ "properties": {
+ "sockets": {
+ "type": "integer",
+ "required": true,
+ "minimum": 1
+ },
+ "cores": {
+ "type": "integer",
+ "required": true,
+ "minimum": 1
+ },
+ "threads": {
+ "type": "integer",
+ "required": true,
+ "minimum": 1
+ }
+ }
+ }
+ }
}
},
"properties": {
@@ -448,7 +475,8 @@
"type": "array",
"items": { "type": "string" }
},
- "graphics": { "$ref":
"#/kimchitype/graphics" }
+ "graphics": { "$ref":
"#/kimchitype/graphics" },
+ "cpu_info": { "$ref":
"#/kimchitype/cpu_info" }
},
"additionalProperties": false,
"error": "KCHAPI0001E"
@@ -612,7 +640,8 @@
"type": "array",
"items": { "type": "string" }
},
- "graphics": { "$ref":
"#/kimchitype/graphics" }
+ "graphics": { "$ref":
"#/kimchitype/graphics" },
+ "cpu_info": { "$ref":
"#/kimchitype/cpu_info" }
},
"additionalProperties": false,
"error": "KCHAPI0001E"
diff --git a/src/kimchi/control/templates.py b/src/kimchi/control/templates.py
index e17fa54..54caa92 100644
--- a/src/kimchi/control/templates.py
+++ b/src/kimchi/control/templates.py
@@ -38,13 +38,14 @@ def __init__(self, model, ident):
self.update_params = ["name", "folder", "icon",
"os_distro",
"storagepool", "os_version",
"cpus",
"memory", "cdrom", "disks",
"networks",
- "graphics"]
+ "graphics", "cpu_info"]
self.uri_fmt = "/templates/%s"
self.clone = self.generate_action_handler('clone')
@property
def data(self):
There is a indent problem.
The correct style (in terms of passing PEP8 check) is
return_data = {
'name': self.ident,
...
Maybe the PEP8 tool is a bit strange. My understanding of its rule is
that when there is an element follows (, { or [, the wrapped elements
indent with this first element.
return [aaaa,
bbbb,
....
When we wrap directly after (, {, [, we just need one more level of
indentation to the previous line.
return [
aaaa,
bbbb,
....
- return {'name': self.ident,
+ return_data = {
+ 'name': self.ident,
'icon': self.info['icon'],
'invalid': self.info['invalid'],
'os_distro': self.info['os_distro'],
@@ -56,4 +57,10 @@ def data(self):
'storagepool': self.info['storagepool'],
'networks': self.info['networks'],
'folder': self.info.get('folder', []),
- 'graphics': self.info['graphics']}
+ 'graphics': self.info['graphics']
+ }
+ if (self.info.get('cpu_info')):
Maybe you want to express "if 'cpu_info' in self.info"?
+ return_data['cpu_info'] =
self.info['cpu_info']
+ else:
+ return_data['cpu_info'] = ''
Is "cpu_info" a dictionary? In case of missing cpu info, maybe None is
more Pythonic than ''. If you agree to use None instead of '', maybe the
whole "if..esle" block can be changed to just one line
return_data = {
....
'cpu_info' = self.info.get('cpu_info')
}
+ return return_data
diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py
index 75fb076..611316c 100644
--- a/src/kimchi/i18n.py
+++ b/src/kimchi/i18n.py
@@ -143,6 +143,7 @@
"KCHTMPL0022E": _("Disk size must be an integer greater than
1GB."),
"KCHTMPL0023E": _("Template base image must be a valid local image
file"),
"KCHTMPL0024E": _("Cannot identify base image %(path)s
format"),
+ "KCHTMPL0025E": _("When specifying CPU topology, VCPUs must be a
product of sockets, cores, and threads."),
"KCHPOOL0001E": _("Storage pool %(name)s already exists"),
"KCHPOOL0002E": _("Storage pool %(name)s does not exist"),
diff --git a/src/kimchi/model/templates.py b/src/kimchi/model/templates.py
index 9278cdc..2954148 100644
--- a/src/kimchi/model/templates.py
+++ b/src/kimchi/model/templates.py
@@ -48,6 +48,22 @@ def create(self, params):
{'filename': iso, 'user': user,
'err': excp})
+ cpu_info = params.get('cpu_info')
+ if cpu_info:
+ cpu_info = dict(params['cpu_info'])
It seems according to "API.json", params['cpu_info'] is already a
dictionary, so the above line is not needed.
+ topology = cpu_info.get('topology')
+ # Check, even though currently only topology
+ # is supported.
+ if topology:
+ sockets = topology['sockets']
+ cores = topology['cores']
+ threads = topology['threads']
+ vcpus = params.get('cpus')
+ if vcpus is None:
+ params['cpus'] = sockets * cores * threads
+ elif vcpus != sockets * cores * threads:
There two spaces after "!=". According to PEP8, just one space is needed.
+ raise
InvalidParameter("KCHTMPL0025E")
+
conn = self.conn.get()
pool_uri = params.get(u'storagepool', '')
if pool_uri:
@@ -156,6 +172,10 @@ def update(self, name, params):
old_t = self.lookup(name)
new_t = copy.copy(old_t)
new_t.update(params)
+
+ if not self._validate_updated_cpu_params(new_t):
A tab is used before the "if". I think it should be 8 spaces.
+ raise InvalidParameter('KCHTMPL0025E')
+
ident = name
conn = self.conn.get()
@@ -187,6 +207,18 @@ def update(self, name, params):
raise
return ident
I found tabs and spaces are mixed for indent in the following method
definition.
+ def _validate_updated_cpu_params(self, info):
+ # Note: cpu_info is the parent of topology. cpus is vcpus
+ vcpus = info['cpus']
+ cpu_info = info.get('cpu_info')
+ if cpu_info is None:
+ return True
+ topology = cpu_info.get('topology')
+ if topology is None:
+ return True
Trailing white space after "True"
+ return vcpus == topology['sockets'] *
topology['cores'] * \
+ topology['threads']
Trailing white space.
+
class LibvirtVMTemplate(VMTemplate):
def __init__(self, args, scan=False, conn=None):
diff --git a/src/kimchi/osinfo.py b/src/kimchi/osinfo.py
index 6ee5e48..0e16b50 100644
--- a/src/kimchi/osinfo.py
+++ b/src/kimchi/osinfo.py
@@ -32,9 +32,8 @@
'power': ('ppc', 'ppc64')}
-common_spec = {'cpus': 1, 'cpu_cores': 1, 'cpu_threads': 1,
'memory': 1024,
- 'disks': [{'index': 0, 'size': 10}],
'cdrom_bus': 'ide',
- 'cdrom_index': 2, 'mouse_bus': 'ps2'}
+common_spec = {'cpus': 1, 'memory': 1024, 'disks':
[{'index': 0, 'size': 10}],
+ 'cdrom_bus': 'ide', 'cdrom_index': 2,
'mouse_bus': 'ps2'}
modern_spec = dict(common_spec, disk_bus='virtio', nic_model='virtio')
diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py
index 5f22db9..cfb7704 100644
--- a/src/kimchi/vmtemplate.py
+++ b/src/kimchi/vmtemplate.py
@@ -358,6 +358,20 @@ def _get_input_output_xml(self):
input_output += sound % self.info
return input_output
+ def _get_cpu_xml(self):
+
+ cpu_info = self.info.get('cpu_info')
+ if cpu_info is None:
+ return ""
+ cpu_topo = cpu_info.get('topology')
+ if cpu_topo is None:
+ return ""
+ return etree.tostring(E.cpu(E.topology(
+ sockets=str(cpu_topo['sockets']),
+ cores=str(cpu_topo['cores']),
+ threads=str(cpu_topo['threads']))))
+
+
def to_vm_xml(self, vm_name, vm_uuid, **kwargs):
params = dict(self.info)
params['name'] = vm_name
@@ -369,6 +383,7 @@ def to_vm_xml(self, vm_name, vm_uuid, **kwargs):
params['qemu-stream-cmdline'] = ''
graphics = kwargs.get('graphics')
params['graphics'] = self._get_graphics_xml(graphics)
+ params['cpu_info'] = self._get_cpu_xml()
# Current implementation just allows to create disk in one single
# storage pool, so we cannot mix the types (scsi volumes vs img file)
@@ -400,6 +415,7 @@ def to_vm_xml(self, vm_name, vm_uuid, **kwargs):
<uuid>%(uuid)s</uuid>
<memory unit='MiB'>%(memory)s</memory>
<vcpu>%(cpus)s</vcpu>
+ %(cpu_info)s
<os>
<type arch='%(arch)s'>hvm</type>
<boot dev='hd'/>