[Kimchi-devel] [PATCH] Backend support for templates with sockets, cores, and threads
Christy Perez
christy at linux.vnet.ibm.com
Fri Oct 3 16:46:21 UTC 2014
On 10/03/2014 10:31 AM, Aline Manera wrote:
>
> 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
>
>
I just tried deleting it from the XML of a guest I had with a topology
element of 1 socket, 1 core, and 2 threads. It looks like if you don't
put vcpus, it defaults back to just '1', so, yes. We need both.
Is just returning and empty string for this value okay? I actually sent
a v2 to the ml at the same time you replied to this, but I had a typo in
the address so it didn't make it to the list. I'll hold off on v2 for
this discussion.
>
>>
>>>>
>>>>> + 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