[Kimchi-devel] [PATCH v2] Backend support for templates with sockets, cores, and threads
Christy Perez
christy at linux.vnet.ibm.com
Thu Oct 9 18:50:25 UTC 2014
Thanks Royce. Great suggestions. Replies below...
On 10/09/2014 02:58 AM, Royce Lv wrote:
> On 2014年10月04日 06:13, 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
>>
>> 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 | 11 +++++++++-
>> po/en_US.po | 3 +++
>> po/pt_BR.po | 3 +++
>> po/zh_CN.po | 3 +++
>> src/kimchi/API.json | 30 ++++++++++++++++++++++++--
>> src/kimchi/control/templates.py | 13 +++++++++---
>> src/kimchi/i18n.py | 1 +
>> src/kimchi/model/templates.py | 47
>> +++++++++++++++++++++++++++++++++++++++++
>> src/kimchi/osinfo.py | 2 +-
>> src/kimchi/vmtemplate.py | 15 +++++++++++++
>> 10 files changed, 121 insertions(+), 7 deletions(-)
>>
>> diff --git a/docs/API.md b/docs/API.md
>> index cc438cc..6281f70 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. If specifying a CPU topology, make sure this
>> value is
>> + equal to the product of sockets, cores, and threads.
> If cpu_info is given, the cpus value seems redundant, what about: If
> specifying a CPU topology, this value is omitted and given the value of
> product of sockets, cores and threads.
> or we can just ensure only one of cpus or cpu_info-topology is assigned,
> what do you think?
I'm fine with gleaning the vcpu count from the topology if it's passed
in without vcpus.
The second option of one but not both would cause us to deviate from
libvirt for users of the REST APIs.
I'll update the patch with your first suggestion.
>> * 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,13 @@ 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.
>> ### Sub-Collection: Virtual Machine Network Interfaces
>> diff --git a/po/en_US.po b/po/en_US.po
>> index eb571ca..74f1ec6 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 "When specifying CPU topology, VCPUs must be a product of
>> sockets, cores, and threads."
> msgstr is not needed because if we can't find the translation, we just
> fallback to msgid;)
ACK.
>> +
>> #, 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..0fe3993 100644
>> --- a/src/kimchi/API.json
>> +++ b/src/kimchi/API.json
>> @@ -26,6 +26,30 @@
>> ]
>> }
>> }
>> + },
>> + "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
>> + },
> As "integer" is "the set
> <http://en.wikipedia.org/wiki/Set_%28mathematics%29> of integers
> consists of zero <http://en.wikipedia.org/wiki/Zero> (0
> <http://en.wikipedia.org/wiki/0_%28number%29>), the counting numbers
> <http://en.wikipedia.org/wiki/Counting_number> (1
> <http://en.wikipedia.org/wiki/1_%28number%29>, 2
> <http://en.wikipedia.org/wiki/2_%28number%29>, 3
> <http://en.wikipedia.org/wiki/3_%28number%29>, ...)", I think we need to
> add:
> "minimum": 1;
ACK.
>> + "cores": {
>> + "type": "integer",
>> + "required": true
>> + },
>> + "threads": {
>> + "type": "integer",
>> + "required": true
>> + }
>> + }
>> + }
>> + }
>> }
>> },
>> "properties": {
>> @@ -448,7 +472,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 +637,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'] = ''
> Can we just omit this value(omit the "else" part) if no cpu_info found?
> that way it'll be easier for UI to make judgement.
Aline's comment on my v1 patch is why I added this else. She said we
should always return the same info. This was the only way that I could
think of that will work. You can read my questions in the patch v1
thread regarding this if you want more 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."),
> As I commented before
ACK. I'll only check this condition *if* both the items are passed in
(vcpus, and topology).
>> "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..203c324 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')
>> + # Check, even though currently only topology
>> + # is supported.
>> + if topology:
>> + sockets = int(topology.get('sockets'))
>> + cores = int(topology.get('cores'))
>> + threads = int(topology.get('threads'))
> Since we did validation of this param type , why do we do this conversion?
Oops. I had them as strings before and changed it. I can take this out.
Thanks.
> Also they are "required",so we do not need "get"
ACK.
>> + vcpus = params.get('cpus')
>> + if vcpus is None:
>> + vcpus = 1
>> + if sockets * cores * threads != vcpus:
>> + raise InvalidParameter("KCHTMPL0025E")
>
>> +
>> conn = self.conn.get()
>> pool_uri = params.get(u'storagepool', '')
>> if pool_uri:
>> @@ -154,6 +170,37 @@ def delete(self, name):
>> def update(self, name, params):
>> old_t = self.lookup(name)
>> + # If changing vcpus, check for topology values, and vice versa
>> + # 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']):
>> + raise InvalidParameter('KCHTMPL0025E')
> I suggest we wrap this to a function so that update function won't be
> too long and easier to understand.
Okay.
>> +
>> new_t = copy.copy(old_t)
>> new_t.update(params)
>> ident = name
>> 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,
>> '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')
>> + 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 ""
>> +
>> 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'/>
>
>
Thanks! Will send a v3 soon.
- Christy
More information about the Kimchi-devel
mailing list