[Kimchi-devel] [PATCH v3] Backend support for templates with sockets, cores, and threads
Christy Perez
christy at linux.vnet.ibm.com
Mon Oct 13 22:53:20 UTC 2014
I talked to Aline about a lot of this, and I'll summarize quickly
instead of addressing each item.
1. Aline didn't catch that the common_spec elements for cores and
threads existed before my patch, so after she realized what I was trying
to say, she agreed that removing them there and putting this information
into my next patch set (the set that will make topology decisions based
on guest and host) makes sense.
2. While topology is currently the only cpu item that Kimchi supports,
there are others supported by libvirt that we may or may not add later.
That's why I always check for topology. It's future-proofing. She's okay
with leaving that in.
Everything else she suggested I've put into v3 and sent.
Regards, and thanks to Royce and Aline for all the good suggestions,
- Christy
On 10/13/2014 02:41 PM, Aline Manera wrote:
>
> On 10/09/2014 06:20 PM, 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
>>
>> 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 | 52
>> +++++++++++++++++++++++++++++++++++++++++
>> src/kimchi/osinfo.py | 2 +-
>> src/kimchi/vmtemplate.py | 15 ++++++++++++
>> 10 files changed, 131 insertions(+), 7 deletions(-)
>>
>> diff --git a/docs/API.md b/docs/API.md
>> index cc438cc..7f1a8c2 100644
>> --- a/docs/API.md
>> +++ b/docs/API.md
>> @@ -179,7 +179,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.
>> @@ -201,6 +203,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 5b752dc..fe3df72 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
>> + }
>> + }
>> + }
>> + }
>
> The cpu_info is the cpu tolopoly, right?
> We could rename cpu_info to cpu_topology and eliminate one date
> structure level.
>
>> }
>> },
>> "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):
>> - 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')):
>> + return_data['cpu_info'] = self.info['cpu_info']
>> + else:
>> + return_data['cpu_info'] = ''
>
> You could return it directly and let the model.lookup() care about the
> cpu_info value
>
> 'cpu_info': self.info['cpu_info']
>
>
>> + return return_data
>> diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py
>> index 1b543ce..9ce8e86 100644
>> --- a/src/kimchi/i18n.py
>> +++ b/src/kimchi/i18n.py
>> @@ -131,6 +131,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..04d2af9 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'])
>> + topology = cpu_info.get('topology')
>
> When there is a "cpu_info" parameter I need to have a "topology", right?
> Otherwise, we should raise an error.
>
>> + # 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 sockets * cores * threads != vcpus:
>> + raise InvalidParameter("KCHTMPL0025E")
>> +
>
>> conn = self.conn.get()
>> pool_uri = params.get(u'storagepool', '')
>> if pool_uri:
>> @@ -154,6 +170,9 @@ def delete(self, name):
>> def update(self, name, params):
>> old_t = self.lookup(name)
>
>> + if not self._verify_updated_cpu_params(params, old_t):
>> + raise InvalidParameter('KCHTMPL0025E')
>> +
>> new_t = copy.copy(old_t)
>> new_t.update(params)
>
> As you can see here, the new_t is the current Template with the updated
> values.
> From my view, you can get the new_t and pass the function above
>
>> ident = name
>> @@ -187,6 +206,39 @@ def update(self, name, params):
>> raise
>> return ident
>> + def _verify_updated_cpu_params(self, params, old_t):
>> + # Note: cpu_info is the parent of topology. cpus is vcpus
>> + # Keep two check_ variables for what we'll verify at the end to
>> + # keep from duplicating code.
>> + check_cpu = None
>> + check_topology = None
>> + # First see what parameters were passed in.
>> + new_vcpus = params.get('cpus')
>> + new_cpu_info = params.get('cpu_info')
>> + new_topology = ''
>> + if new_cpu_info:
>> + new_topology = new_cpu_info.get('topology')
>> + # Now figure out what needs to be verified.
>> + if new_vcpus and new_topology:
>> + check_cpu = new_vcpus
>> + check_topology = new_topology
>> + elif new_vcpus:
>> + old_cpu_info = old_t.get('cpu_info')
>> + if old_cpu_info:
>> + old_topology = old_cpu_info.get('topology')
>> + if old_topology:
>> + check_cpu = new_vcpus
>> + check_topology = old_topology
>> + elif new_topology:
>> + check_cpu = old_t.get('cpus')
>> + check_topology = new_topology
>> + # Now verify the cpu and topoloy parameters
>> + if check_cpu and (check_cpu != check_topology['sockets'] * \
>> + check_topology['cores'] * check_topology['threads']):
>> + return False
>> +
>> + return True
>> +
>> 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..10d0ab0 100644
>> --- a/src/kimchi/osinfo.py
>> +++ b/src/kimchi/osinfo.py
>> @@ -32,7 +32,7 @@
>> 'power': ('ppc', 'ppc64')}
>> -common_spec = {'cpus': 1, 'cpu_cores': 1, 'cpu_threads': 1,
>> 'memory': 1024,
>> +common_spec = {'cpus': 1, 'cpu_sockets': '1', 'cpu_cores': 1,
>> 'cpu_threads': 1, 'memory': 1024,
>
> Why are you using a different data structure here?
>
> It should be:
>
> cpu_topology: {cores:.., thread:..., sockets:...}
>
>> 'disks': [{'index': 0, 'size': 10}], 'cdrom_bus': 'ide',
>> 'cdrom_index': 2, 'mouse_bus': 'ps2'}
>> diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py
>> index 5f22db9..34720de 100644
>> --- a/src/kimchi/vmtemplate.py
>> +++ b/src/kimchi/vmtemplate.py
>> @@ -358,6 +358,19 @@ 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 not None:
>> + cpu_topo = cpu_info.get('topology')
>
> The same I commented before as "topology" is mandatory for cpu_info.
>
>
>> + if cpu_topo is not None:
>> + return etree.tostring(E.cpu(E.topology(
>> + sockets=str(cpu_topo['sockets']),
>> + cores=str(cpu_topo['cores']),
>> + threads=str(cpu_topo['threads']))))
>> +
>> + return ""
>
> You can do:
>
> if cpu_info is None:
> return ""
>
> cpu_topo = ...
> if cpu_topo is None:
> return ""
>
> return etree...
>
> That way we reduce the indentation levels.
>
>> +
>> def to_vm_xml(self, vm_name, vm_uuid, **kwargs):
>> params = dict(self.info)
>> params['name'] = vm_name
>> @@ -369,6 +382,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 +414,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