[Kimchi-devel] [PATCH v4] Backend support for templates with sockets, cores, and threads
Zhou Zheng Sheng
zhshzhou at linux.vnet.ibm.com
Tue Oct 14 08:59:04 UTC 2014
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 at 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'/>
>
More information about the Kimchi-devel
mailing list