[Kimchi-devel] [PATCH v3] Backend support for templates with sockets, cores, and threads
Aline Manera
alinefm at linux.vnet.ibm.com
Mon Oct 13 20:11:27 UTC 2014
On 10/13/2014 11:40 AM, Christy Perez wrote:
>
> On 10/13/2014 04:57 AM, Royce Lv wrote:
>> On 2014年10月10日 05:20, 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
>> unlees-->unless? :)
>>> + 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
>>> + }
>>> + }
>>> + }
>>> + }
>>> }
>>> },
>>> "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'] = ''
>>> + 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')
>>> + # 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)
>>> 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,
>> As we use common_spec to create template, we need to put it in dict
>> "cpu_info", I suppose put it here won't get expected default value, right?
>> In osinfo.py:
>>
>> def lookup(distro, version):
>> ....
>>
>> if distro in modern_version_bases[arch]:
>> if LooseVersion(version) >= LooseVersion(
>> modern_version_bases[arch][distro]):
>> params.update(template_specs[arch]['modern'])
>> else:
>> params.update(template_specs[arch]['old'])
>> else:
>> params['os_distro'] = params['os_version'] = "unknown"
>> params.update(template_specs[arch]['old'])
>> ....
>>
>> return params
>>
>> In vmtemplate.py:
>> ...
>> entry = osinfo.lookup(os_distro, os_version)
>> self.info.update(entry)
>>
>> And in def _get_cpu_xml(self), you used
>>
>> cpu_info = self.info.get('cpu_info')
>>
>> So we need to define default value in 'cpu_info' dict instead.
> I think the default value for this should be to leave it out from the
> backend standpoint.
>
> When we implement the UI, I think the default value should be to have
> SMT/threads checked, and make the user uncheck it. But, that's another
> discussion.
>
> I'm not sure why the cpu_info information was put into the dict like it
> was. Maybe we should just take those elements out?
No, those values must be there.
Let me try to clarify the relation between osinfo.py -> VMTemplate
(vmtemplate.py) -> Template (model/templates.py)
First, keep in mind that you only need a ISO or Image file to create a
Template.
When creating a Template, Kimchi will scan the ISO or Image file to get
its OS and OS version.
Based on OS information we use the osinfo.py to get the default
configuration,
*O**n vmtemplate.py* (__init__)
self.info = {}
entry = osinfo.lookup(os_distro, os_version)
self.info.update(entry)
(...)
# update the dict with user inputs
self.info.update(args)
You can think VMTemplate like a dict of data.
if you add to common_specs:
cpu_info: {topology: {sockets: 1, cores: 1, threads: 1}}
All VMTemplate instances will have this configuration.
And it will be only override by user inputs (if he/she wants to)
Template is the Kimchi resource. Its data is just a copy of
VMTemplate.info data.
Does that make sense?
In a next step, we will have a new default dict based on host architecture.
For example, for Power 8, we should use SMT8
Something like:
power_specs = {8: {cpu_info: {topology: {sockets: 1, cores: 2, threads:
4}}}},
....
}
That way, user does not care about the topology values. Kimchi will
automatically handle it according to host architecture and guest OS.
>
>>> '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'/>
> _______________________________________________
> Kimchi-devel mailing list
> Kimchi-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ovirt.org/pipermail/kimchi-devel/attachments/20141013/dfa48ee1/attachment.html>
More information about the Kimchi-devel
mailing list