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

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,?]:

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