Thanks Zhou. Replies below. I'll send out a v5 with all your suggestions, and a few from Aline, which includes updating all the po files. Note: I'm out tomorrow (Wed) through Friday. Back Monday. On 10/14/2014 03:59 AM, Zhou Zheng Sheng wrote:
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@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, ....
I cleaned that up and also incorporated your next suggestion. Aline liked the idea. With just that, GET returns "cpu_info":null, So I also added a line in the model to set cpu_info to "" if it's None. Now GET returns "cpu_info":""
- 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.
I don't remember why I might have had that there. You're right, though. It isn't needed. Thanks.
+ 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.
Fixed. Thanks. Copy/paste remnant.
+ 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.
Thanks. I was having copy/paste indent issues so I wiped my .vimrc file and forgot to be careful about the tabs there.
+ 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.
Fixed.
+ 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"
Fixed.
+ return vcpus == topology['sockets'] * topology['cores'] * \ + topology['threads']
Trailing white space.
Fixed.
+
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'/>