[Kimchi-devel] [PATCH] Backend support for templates with sockets, cores, and threads
Aline Manera
alinefm at linux.vnet.ibm.com
Fri Oct 3 18:45:57 UTC 2014
On 10/03/2014 01:46 PM, Christy Perez wrote:
>
> 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.
Ok. Thanks for confirm and test it.
> 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