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

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

> >>> >>>> + 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