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

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.

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