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

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 },

> + } > + } > } > }, > "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:...}}

And even with this cpu info (with cores, threads, sockets) we will continue to have the "cpus" data?

> '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?

> + """ % 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.

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