Re: [Kimchi-devel] [PATCH] Backend support for templates with sockets, cores, and threads

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(a)linux.vnet.ibm.com&gt; >>>>> --- >>>>> 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