[Kimchi-devel] [PATCH] Backend support for templates with sockets, cores, and threads
Aline Manera
alinefm at linux.vnet.ibm.com
Fri Oct 3 15:31:57 UTC 2014
On 09/02/2014 07:56 PM, Christy Perez wrote:
> Aline,
>
> A question for you (inline)...
>
> On 08/25/2014 02:08 PM, Christy Perez wrote:
>>
>> On 08/21/2014 01:22 PM, Aline Manera wrote:
>>> On 08/18/2014 08:04 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
>>>>
>>>> Signed-off-by: Christy Perez <christy at linux.vnet.ibm.com>
>>>> ---
>>>> docs/API.md | 7 +++++++
>>>> src/kimchi/API.json | 28 ++++++++++++++++++++++++++--
>>>> src/kimchi/control/templates.py | 11 ++++++++---
>>>> src/kimchi/osinfo.py | 2 +-
>>>> src/kimchi/vmtemplate.py | 22 ++++++++++++++++++++++
>>>> 5 files changed, 64 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/docs/API.md b/docs/API.md
>>>> index d75c55f..cb4d541 100644
>>>> --- a/docs/API.md
>>>> +++ b/docs/API.md
>>>> @@ -200,6 +200,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/src/kimchi/API.json b/src/kimchi/API.json
>>>> index c3fc5e3..d510634 100644
>>>> --- a/src/kimchi/API.json
>>>> +++ b/src/kimchi/API.json
>>>> @@ -26,6 +26,28 @@
>>>> ]
>>>> }
>>>> }
>>>> + },
>>>> + "cpu_info": {
>>>> + "description": "Configure CPU specifics for a VM.",
>>>> + "type": "object",
>>>> + "properties": {
>>>> + "topology": {
>>>> + "description": "Configure the guest CPU topology.",
>>>> + "type": "object",
>>>> + "properties": {
>>>> + "sockets": {
>>>> + "type": "number"
>>>> + },
>>>> + "cores": {
>>>> + "type": "number"
>>>> + },
>>>> + "threads": {
>>>> + "type": "number"
>>>> + }
>>>> + },
>>>> + "required": [ "sockets", "cores", "threads" ]
>>> Usually we set the required = true for each parameter.
>>>
>>> "sockets": {
>>> "type": "number",
>>> "required": true
>>> },
>>>
>> I've changed it to that.
>>>
>>>> + }
>>>> + }
>>>> }
>>>> },
>>>> "properties": {
>>>> @@ -438,7 +460,8 @@
>>>> "type": "array",
>>>> "items": { "type": "string" }
>>>> },
>>>> - "graphics": { "$ref": "#/kimchitype/graphics" }
>>>> + "graphics": { "$ref": "#/kimchitype/graphics" },
>>>> + "cpu_info": { "$ref": "#/kimchitype/cpu_info" }
>>>> },
>>>> "additionalProperties": false,
>>>> "error": "KCHAPI0001E"
>>>> @@ -608,7 +631,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..5ef35ce 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,8 @@ 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']
>>> We should always return the same data set while accessing a template
>>> That way "cpu_info" must always have a value.
> How would you suggest we handle this then? If it's not set, we can't
> return 0's or 1's.
>
> All 0's won't work:
>
> # virsh edit domainname
> error: XML error: Invalid CPU topology
> Failed. Try again? [y,n,f,?]:
>
> All 1's as a default but a change in the vcpus would lead to an error:
>
> # virsh edit domainname
> error: Maximum CPUs greater than topology limit
> Failed. Try again? [y,n,f,?]:
Do we need to have both <vcpu> and cpu topology?
Does the cpu topology not override the <vcpu>?
<vcpu>%(cpus)s</vcpu>
+ %(cpu_info)s
>
>>>
>>>> + return return_data
>>>> diff --git a/src/kimchi/osinfo.py b/src/kimchi/osinfo.py
>>>> index 1ad353c..8492bb7 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 structure here?
>>> I'd expect to have
>>> {..., 'cpu_info': {cores: .., threads:..., sockets:...}}
>> The structure was like that already. I just put in the new value for
>> sockets. I didn't look too deeply into how the rest of the code uses the
>> osifno spec information, but I left it as-is so as not to break any
>> current functionality.
>>
>>> And even with this cpu info (with cores, threads, sockets) we will
>>> continue to have the "cpus" data?
>> Yes. With all 1's as the default, the cpus can be left as-is.
>>
>> I'm adding a check in the vmtemplates code to make sure that this value
>> is always a product of the three values.
>>
>> When we add in some intelligence, we can set the vcpu to be a multiple.
>>
>>>> '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 761bac5..c64e61d 100644
>>>> --- a/src/kimchi/vmtemplate.py
>>>> +++ b/src/kimchi/vmtemplate.py
>>>> @@ -359,6 +359,22 @@ 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 """
>>>> + <cpu>
>>>> + <topology sockets='%(sockets)s'
>>>> + cores='%(cores)s'
>>>> + threads='%(threads)s'/>
>>>> + </cpu>
>>> We are in a movement to use "lxml.builder.E" to build the xml instead of
>>> string.
>>> Could you do it in that way too?
>> Sure! I was wondering about that. I just stuck with how it was being
>> done everywhere else, but I like that method.
>>>> + """ % cpu_topo
>>>> +
>>>> + return ""
>>>> +
>>>> def to_vm_xml(self, vm_name, vm_uuid, **kwargs):
>>>> params = dict(self.info)
>>>> params['name'] = vm_name
>>>> @@ -371,6 +387,11 @@ def to_vm_xml(self, vm_name, vm_uuid, **kwargs):
>>>> graphics = kwargs.get('graphics')
>>>> params['graphics'] = self._get_graphics_xml(graphics)
>>>> + if params.get('cpu_info') is not None:
>>>> + params['cpu_info'] = self._get_cpu_xml()
>>>> + else:
>>>> + params['cpu_info'] = ''
>>>> +
>>> As this "cpu_info" is set in the "common_spec" for any ISO file, this
>>> information will always exist. Otherwise we have a bug
>>> So you can assume that info will be there for use.
>> ACK. Thanks.
>>
>>>> # Current implementation just allows to create disk in one
>>>> single
>>>> # storage pool, so we cannot mix the types (scsi volumes vs
>>>> img file)
>>>> storage_type = self._get_storage_type()
>>>> @@ -401,6 +422,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'/>
> Regards,
>
> - Christy
More information about the Kimchi-devel
mailing list