[Kimchi-devel] [PATCH v4] Backend support for templates with sockets, cores, and threads
Christy Perez
christy at linux.vnet.ibm.com
Tue Oct 14 16:35:27 UTC 2014
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 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,
> ....
>
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'/>
>>
More information about the Kimchi-devel
mailing list