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 v2->v3: - Set vcpus based on topology, if specified. - Move the update cpu+topology validation out to a function for redability - Add a minimum value of 1 for topology values - Leave new English error msg as empty string - Update the API documentation on cpu defaults v1->v2: - Added a check to make sure that vcpus = sockets*cores*threads - Set individual topoology params to required in API.json - Change the topology object types from string to integer - Always return cpu_info from templates lookup() - Removed check for cpu_info in to_vm_xml - Build cpu_info xml using lxml.builder instead of string - CPU and topology verification on template update Signed-off-by: Christy Perez <christy(a)linux.vnet.ibm.com&gt; --- docs/API.md | 13 ++++++++++- po/en_US.po | 3 +++ po/pt_BR.po | 3 +++ po/zh_CN.po | 3 +++ src/kimchi/API.json | 33 ++++++++++++++++++++++++-- src/kimchi/control/templates.py | 13 ++++++++--- src/kimchi/i18n.py | 1 + src/kimchi/model/templates.py | 52 +++++++++++++++++++++++++++++++++++++++++ src/kimchi/osinfo.py | 2 +- src/kimchi/vmtemplate.py | 15 ++++++++++++ 10 files changed, 131 insertions(+), 7 deletions(-) diff --git a/docs/API.md b/docs/API.md index cc438cc..7f1a8c2 100644 --- a/docs/API.md +++ b/docs/API.md @@ -179,7 +179,9 @@ Represents a snapshot of the Virtual Machine's primary monitor. * name: The name of the Template. Used to identify the Template in this API * os_distro *(optional)*: The operating system distribution * os_version *(optional)*: The version of the operating system distribution - * cpus *(optional)*: The number of CPUs assigned to the VM. Default is 1. + * cpus *(optional)*: The number of CPUs assigned to the VM. + Default is 1, unlees specifying a cpu topology. In that case, cpus

+ will default to a product of the topology values (see cpu_info). * memory *(optional)*: The amount of memory assigned to the VM. Default is 1024M. * cdrom *(optional)*: A volume name or URI to an ISO image. @@ -201,6 +203,15 @@ 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. + If specifying both cpus and CPU topology, make sure cpus is + equal to the product of sockets, cores, and threads. ### Sub-Collection: Virtual Machine Network Interfaces diff --git a/po/en_US.po b/po/en_US.po index eb571ca..a88675c 100644 --- a/po/en_US.po +++ b/po/en_US.po @@ -371,6 +371,9 @@ msgstr "" msgid "Cannot identify base image %(path)s format" msgstr "" +msgid "When specifying CPU topology, VCPUs must be a product of sockets, cores, and threads." +msgstr "" + #, python-format msgid "Storage pool %(name)s already exists" msgstr "" diff --git a/po/pt_BR.po b/po/pt_BR.po index f2fba64..1bebe30 100644 --- a/po/pt_BR.po +++ b/po/pt_BR.po @@ -424,6 +424,9 @@ msgstr "Imagem base do modelo deve ser um arquivo de imagem local válido" msgid "Cannot identify base image %(path)s format" msgstr "Não foi possível identificar o formato da imagem base %(path)s" +msgid "When specifying CPU topology, VCPUs must be a product of sockets, cores, and threads." +msgstr "" + #, python-format msgid "Storage pool %(name)s already exists" msgstr "Storage pool %(name)s já existe" diff --git a/po/zh_CN.po b/po/zh_CN.po index f77e405..f944b8d 100644 --- a/po/zh_CN.po +++ b/po/zh_CN.po @@ -392,6 +392,9 @@ msgstr "模板基础镜像必须为一个有效的本地镜像文件" msgid "Cannot identify base image %(path)s format" msgstr "未能识别基础镜像%(path)s格式" +msgid "When specifying CPU topology, VCPUs must be a product of sockets, cores, and threads." +msgstr "" + #, python-format msgid "Storage pool %(name)s already exists" msgstr "存储池%(name)s已经存在" diff --git a/src/kimchi/API.json b/src/kimchi/API.json index 5b752dc..fe3df72 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -26,6 +26,33 @@ ] } } + }, + "cpu_info": { + "description": "Configure CPU specifics for a VM.", + "type": "object", + "properties": { + "topology": { + "description": "Configure the guest CPU topology.", + "type": "object", + "properties": { + "sockets": { + "type": "integer", + "required": true, + "minimum": 1 + }, + "cores": { + "type": "integer", + "required": true, + "minimum": 1 + }, + "threads": { + "type": "integer", + "required": true, + "minimum": 1 + } + } + } + } } }, "properties": { @@ -448,7 +475,8 @@ "type": "array", "items": { "type": "string" } }, - "graphics": { "$ref": "#/kimchitype/graphics" } + "graphics": { "$ref": "#/kimchitype/graphics" }, + "cpu_info": { "$ref": "#/kimchitype/cpu_info" } }, "additionalProperties": false, "error": "KCHAPI0001E" @@ -612,7 +640,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..54caa92 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,10 @@ 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'] + else: + return_data['cpu_info'] = '' + return return_data diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index 1b543ce..9ce8e86 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -131,6 +131,7 @@ "KCHTMPL0022E": _("Disk size must be an integer greater than 1GB."), "KCHTMPL0023E": _("Template base image must be a valid local image file"), "KCHTMPL0024E": _("Cannot identify base image %(path)s format"), + "KCHTMPL0025E": _("When specifying CPU topology, VCPUs must be a product of sockets, cores, and threads."), "KCHPOOL0001E": _("Storage pool %(name)s already exists"), "KCHPOOL0002E": _("Storage pool %(name)s does not exist"), diff --git a/src/kimchi/model/templates.py b/src/kimchi/model/templates.py index 9278cdc..04d2af9 100644 --- a/src/kimchi/model/templates.py +++ b/src/kimchi/model/templates.py @@ -48,6 +48,22 @@ def create(self, params): {'filename': iso, 'user': user, 'err': excp}) + cpu_info = params.get('cpu_info') + if cpu_info: + cpu_info = dict(params['cpu_info']) + topology = cpu_info.get('topology') + # Check, even though currently only topology + # is supported. + if topology: + sockets = topology['sockets'] + cores = topology['cores'] + threads = topology['threads'] + vcpus = params.get('cpus') + if vcpus is None: + params['cpus'] = sockets * cores * threads + elif sockets * cores * threads != vcpus: + raise InvalidParameter("KCHTMPL0025E") + conn = self.conn.get() pool_uri = params.get(u'storagepool', '') if pool_uri: @@ -154,6 +170,9 @@ def delete(self, name): def update(self, name, params): old_t = self.lookup(name) + if not self._verify_updated_cpu_params(params, old_t): + raise InvalidParameter('KCHTMPL0025E') + new_t = copy.copy(old_t) new_t.update(params) ident = name @@ -187,6 +206,39 @@ def update(self, name, params): raise return ident + def _verify_updated_cpu_params(self, params, old_t): + # Note: cpu_info is the parent of topology. cpus is vcpus + # Keep two check_ variables for what we'll verify at the end to + # keep from duplicating code. + check_cpu = None + check_topology = None + # First see what parameters were passed in. + new_vcpus = params.get('cpus') + new_cpu_info = params.get('cpu_info') + new_topology = '' + if new_cpu_info: + new_topology = new_cpu_info.get('topology') + # Now figure out what needs to be verified. + if new_vcpus and new_topology: + check_cpu = new_vcpus + check_topology = new_topology + elif new_vcpus: + old_cpu_info = old_t.get('cpu_info') + if old_cpu_info: + old_topology = old_cpu_info.get('topology') + if old_topology: + check_cpu = new_vcpus + check_topology = old_topology + elif new_topology: + check_cpu = old_t.get('cpus') + check_topology = new_topology + # Now verify the cpu and topoloy parameters + if check_cpu and (check_cpu != check_topology['sockets'] * \ + check_topology['cores'] * check_topology['threads']): + return False + + return True + class LibvirtVMTemplate(VMTemplate): def __init__(self, args, scan=False, conn=None): diff --git a/src/kimchi/osinfo.py b/src/kimchi/osinfo.py index 6ee5e48..10d0ab0 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,

'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 5f22db9..34720de 100644 --- a/src/kimchi/vmtemplate.py +++ b/src/kimchi/vmtemplate.py @@ -358,6 +358,19 @@ 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 etree.tostring(E.cpu(E.topology( + sockets=str(cpu_topo['sockets']), + cores=str(cpu_topo['cores']), + threads=str(cpu_topo['threads'])))) + + return "" + def to_vm_xml(self, vm_name, vm_uuid, **kwargs): params = dict(self.info) params['name'] = vm_name @@ -369,6 +382,7 @@ def to_vm_xml(self, vm_name, vm_uuid, **kwargs): params['qemu-stream-cmdline'] = '' graphics = kwargs.get('graphics') params['graphics'] = self._get_graphics_xml(graphics) + params['cpu_info'] = self._get_cpu_xml() # Current implementation just allows to create disk in one single # storage pool, so we cannot mix the types (scsi volumes vs img file) @@ -400,6 +414,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'/>

On 10/13/2014 04:57 AM, Royce Lv wrote: > On 2014年10月10日 05:20, 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 >> >> v2->v3: >> - Set vcpus based on topology, if specified. >> - Move the update cpu+topology validation out to a function >> for redability >> - Add a minimum value of 1 for topology values >> - Leave new English error msg as empty string >> - Update the API documentation on cpu defaults >> >> v1->v2: >> - Added a check to make sure that vcpus = sockets*cores*threads >> - Set individual topoology params to required in API.json >> - Change the topology object types from string to integer >> - Always return cpu_info from templates lookup() >> - Removed check for cpu_info in to_vm_xml >> - Build cpu_info xml using lxml.builder instead of string >> - CPU and topology verification on template update >> >> Signed-off-by: Christy Perez <christy(a)linux.vnet.ibm.com&gt; >> --- >> docs/API.md | 13 ++++++++++- >> po/en_US.po | 3 +++ >> po/pt_BR.po | 3 +++ >> po/zh_CN.po | 3 +++ >> src/kimchi/API.json | 33 ++++++++++++++++++++++++-- >> src/kimchi/control/templates.py | 13 ++++++++--- >> src/kimchi/i18n.py | 1 + >> src/kimchi/model/templates.py | 52 >> +++++++++++++++++++++++++++++++++++++++++ >> src/kimchi/osinfo.py | 2 +- >> src/kimchi/vmtemplate.py | 15 ++++++++++++ >> 10 files changed, 131 insertions(+), 7 deletions(-) >> >> diff --git a/docs/API.md b/docs/API.md >> index cc438cc..7f1a8c2 100644 >> --- a/docs/API.md >> +++ b/docs/API.md >> @@ -179,7 +179,9 @@ Represents a snapshot of the Virtual Machine's >> primary monitor. >> * name: The name of the Template. Used to identify the Template >> in this API >> * os_distro *(optional)*: The operating system distribution >> * os_version *(optional)*: The version of the operating system >> distribution >> - * cpus *(optional)*: The number of CPUs assigned to the VM. >> Default is 1. >> + * cpus *(optional)*: The number of CPUs assigned to the VM. >> + Default is 1, unlees specifying a cpu topology. In that >> case, cpus > unlees-->unless? :) >> + will default to a product of the topology values (see >> cpu_info). >> * memory *(optional)*: The amount of memory assigned to the VM. >> Default is 1024M. >> * cdrom *(optional)*: A volume name or URI to an ISO image. >> @@ -201,6 +203,15 @@ 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. >> + If specifying both cpus and CPU topology, make sure cpus is >> + equal to the product of sockets, cores, and threads. >> ### Sub-Collection: Virtual Machine Network Interfaces >> diff --git a/po/en_US.po b/po/en_US.po >> index eb571ca..a88675c 100644 >> --- a/po/en_US.po >> +++ b/po/en_US.po >> @@ -371,6 +371,9 @@ msgstr "" >> msgid "Cannot identify base image %(path)s format" >> msgstr "" >> +msgid "When specifying CPU topology, VCPUs must be a product of >> sockets, cores, and threads." >> +msgstr "" >> + >> #, python-format >> msgid "Storage pool %(name)s already exists" >> msgstr "" >> diff --git a/po/pt_BR.po b/po/pt_BR.po >> index f2fba64..1bebe30 100644 >> --- a/po/pt_BR.po >> +++ b/po/pt_BR.po >> @@ -424,6 +424,9 @@ msgstr "Imagem base do modelo deve ser um arquivo >> de imagem local válido" >> msgid "Cannot identify base image %(path)s format" >> msgstr "Não foi possível identificar o formato da imagem base %(path)s" >> +msgid "When specifying CPU topology, VCPUs must be a product of >> sockets, cores, and threads." >> +msgstr "" >> + >> #, python-format >> msgid "Storage pool %(name)s already exists" >> msgstr "Storage pool %(name)s já existe" >> diff --git a/po/zh_CN.po b/po/zh_CN.po >> index f77e405..f944b8d 100644 >> --- a/po/zh_CN.po >> +++ b/po/zh_CN.po >> @@ -392,6 +392,9 @@ msgstr "模板基础镜像必须为一个有效的本地镜像文件" >> msgid "Cannot identify base image %(path)s format" >> msgstr "未能识别基础镜像%(path)s格式" >> +msgid "When specifying CPU topology, VCPUs must be a product of >> sockets, cores, and threads." >> +msgstr "" >> + >> #, python-format >> msgid "Storage pool %(name)s already exists" >> msgstr "存储池%(name)s已经存在" >> diff --git a/src/kimchi/API.json b/src/kimchi/API.json >> index 5b752dc..fe3df72 100644 >> --- a/src/kimchi/API.json >> +++ b/src/kimchi/API.json >> @@ -26,6 +26,33 @@ >> ] >> } >> } >> + }, >> + "cpu_info": { >> + "description": "Configure CPU specifics for a VM.", >> + "type": "object", >> + "properties": { >> + "topology": { >> + "description": "Configure the guest CPU topology.", >> + "type": "object", >> + "properties": { >> + "sockets": { >> + "type": "integer", >> + "required": true, >> + "minimum": 1 >> + }, >> + "cores": { >> + "type": "integer", >> + "required": true, >> + "minimum": 1 >> + }, >> + "threads": { >> + "type": "integer", >> + "required": true, >> + "minimum": 1 >> + } >> + } >> + } >> + } >> } >> }, >> "properties": { >> @@ -448,7 +475,8 @@ >> "type": "array", >> "items": { "type": "string" } >> }, >> - "graphics": { "$ref": "#/kimchitype/graphics" } >> + "graphics": { "$ref": "#/kimchitype/graphics" }, >> + "cpu_info": { "$ref": "#/kimchitype/cpu_info" } >> }, >> "additionalProperties": false, >> "error": "KCHAPI0001E" >> @@ -612,7 +640,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..54caa92 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,10 @@ 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'] >> + else: >> + return_data['cpu_info'] = '' >> + return return_data >> diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py >> index 1b543ce..9ce8e86 100644 >> --- a/src/kimchi/i18n.py >> +++ b/src/kimchi/i18n.py >> @@ -131,6 +131,7 @@ >> "KCHTMPL0022E": _("Disk size must be an integer greater than >> 1GB."), >> "KCHTMPL0023E": _("Template base image must be a valid local >> image file"), >> "KCHTMPL0024E": _("Cannot identify base image %(path)s format"), >> + "KCHTMPL0025E": _("When specifying CPU topology, VCPUs must be a >> product of sockets, cores, and threads."), >> "KCHPOOL0001E": _("Storage pool %(name)s already exists"), >> "KCHPOOL0002E": _("Storage pool %(name)s does not exist"), >> diff --git a/src/kimchi/model/templates.py >> b/src/kimchi/model/templates.py >> index 9278cdc..04d2af9 100644 >> --- a/src/kimchi/model/templates.py >> +++ b/src/kimchi/model/templates.py >> @@ -48,6 +48,22 @@ def create(self, params): >> {'filename': iso, 'user': user, >> 'err': excp}) >> + cpu_info = params.get('cpu_info') >> + if cpu_info: >> + cpu_info = dict(params['cpu_info']) >> + topology = cpu_info.get('topology') >> + # Check, even though currently only topology >> + # is supported. >> + if topology: >> + sockets = topology['sockets'] >> + cores = topology['cores'] >> + threads = topology['threads'] >> + vcpus = params.get('cpus') >> + if vcpus is None: >> + params['cpus'] = sockets * cores * threads >> + elif sockets * cores * threads != vcpus: >> + raise InvalidParameter("KCHTMPL0025E") >> + >> conn = self.conn.get() >> pool_uri = params.get(u'storagepool', '') >> if pool_uri: >> @@ -154,6 +170,9 @@ def delete(self, name): >> def update(self, name, params): >> old_t = self.lookup(name) >> + if not self._verify_updated_cpu_params(params, old_t): >> + raise InvalidParameter('KCHTMPL0025E') >> + >> new_t = copy.copy(old_t) >> new_t.update(params) >> ident = name >> @@ -187,6 +206,39 @@ def update(self, name, params): >> raise >> return ident >> + def _verify_updated_cpu_params(self, params, old_t): >> + # Note: cpu_info is the parent of topology. cpus is vcpus >> + # Keep two check_ variables for what we'll verify at the end to >> + # keep from duplicating code. >> + check_cpu = None >> + check_topology = None >> + # First see what parameters were passed in. >> + new_vcpus = params.get('cpus') >> + new_cpu_info = params.get('cpu_info') >> + new_topology = '' >> + if new_cpu_info: >> + new_topology = new_cpu_info.get('topology') >> + # Now figure out what needs to be verified. >> + if new_vcpus and new_topology: >> + check_cpu = new_vcpus >> + check_topology = new_topology >> + elif new_vcpus: >> + old_cpu_info = old_t.get('cpu_info') >> + if old_cpu_info: >> + old_topology = old_cpu_info.get('topology') >> + if old_topology: >> + check_cpu = new_vcpus >> + check_topology = old_topology >> + elif new_topology: >> + check_cpu = old_t.get('cpus') >> + check_topology = new_topology >> + # Now verify the cpu and topoloy parameters >> + if check_cpu and (check_cpu != check_topology['sockets'] * \ >> + check_topology['cores'] * check_topology['threads']): >> + return False >> + >> + return True >> + >> class LibvirtVMTemplate(VMTemplate): >> def __init__(self, args, scan=False, conn=None): >> diff --git a/src/kimchi/osinfo.py b/src/kimchi/osinfo.py >> index 6ee5e48..10d0ab0 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, > As we use common_spec to create template, we need to put it in dict > "cpu_info", I suppose put it here won't get expected default value, right? > In osinfo.py: > > def lookup(distro, version): > .... > > if distro in modern_version_bases[arch]: > if LooseVersion(version) >= LooseVersion( > modern_version_bases[arch][distro]): > params.update(template_specs[arch]['modern']) > else: > params.update(template_specs[arch]['old']) > else: > params['os_distro'] = params['os_version'] = "unknown" > params.update(template_specs[arch]['old']) > .... > > return params > > In vmtemplate.py: > ... > entry = osinfo.lookup(os_distro, os_version) > self.info.update(entry) > > And in def _get_cpu_xml(self), you used > > cpu_info = self.info.get('cpu_info') > > So we need to define default value in 'cpu_info' dict instead. I think the default value for this should be to leave it out from the backend standpoint. When we implement the UI, I think the default value should be to have SMT/threads checked, and make the user uncheck it. But, that's another discussion. I'm not sure why the cpu_info information was put into the dict like it was. Maybe we should just take those elements out?

>> '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 5f22db9..34720de 100644 >> --- a/src/kimchi/vmtemplate.py >> +++ b/src/kimchi/vmtemplate.py >> @@ -358,6 +358,19 @@ 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 etree.tostring(E.cpu(E.topology( >> + sockets=str(cpu_topo['sockets']), >> + cores=str(cpu_topo['cores']), >> + threads=str(cpu_topo['threads'])))) >> + >> + return "" >> + >> def to_vm_xml(self, vm_name, vm_uuid, **kwargs): >> params = dict(self.info) >> params['name'] = vm_name >> @@ -369,6 +382,7 @@ def to_vm_xml(self, vm_name, vm_uuid, **kwargs): >> params['qemu-stream-cmdline'] = '' >> graphics = kwargs.get('graphics') >> params['graphics'] = self._get_graphics_xml(graphics) >> + params['cpu_info'] = self._get_cpu_xml() >> # Current implementation just allows to create disk in one >> single >> # storage pool, so we cannot mix the types (scsi volumes vs >> img file) >> @@ -400,6 +414,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'/> _______________________________________________ Kimchi-devel mailing list Kimchi-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel

On 10/09/2014 06:20 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 > > v2->v3: > - Set vcpus based on topology, if specified. > - Move the update cpu+topology validation out to a function > for redability > - Add a minimum value of 1 for topology values > - Leave new English error msg as empty string > - Update the API documentation on cpu defaults > > v1->v2: > - Added a check to make sure that vcpus = sockets*cores*threads > - Set individual topoology params to required in API.json > - Change the topology object types from string to integer > - Always return cpu_info from templates lookup() > - Removed check for cpu_info in to_vm_xml > - Build cpu_info xml using lxml.builder instead of string > - CPU and topology verification on template update > > Signed-off-by: Christy Perez <christy(a)linux.vnet.ibm.com&gt; > --- > docs/API.md | 13 ++++++++++- > po/en_US.po | 3 +++ > po/pt_BR.po | 3 +++ > po/zh_CN.po | 3 +++ > src/kimchi/API.json | 33 ++++++++++++++++++++++++-- > src/kimchi/control/templates.py | 13 ++++++++--- > src/kimchi/i18n.py | 1 + > src/kimchi/model/templates.py | 52 > +++++++++++++++++++++++++++++++++++++++++ > src/kimchi/osinfo.py | 2 +- > src/kimchi/vmtemplate.py | 15 ++++++++++++ > 10 files changed, 131 insertions(+), 7 deletions(-) > > diff --git a/docs/API.md b/docs/API.md > index cc438cc..7f1a8c2 100644 > --- a/docs/API.md > +++ b/docs/API.md > @@ -179,7 +179,9 @@ Represents a snapshot of the Virtual Machine's > primary monitor. > * name: The name of the Template. Used to identify the Template > in this API > * os_distro *(optional)*: The operating system distribution > * os_version *(optional)*: The version of the operating system > distribution > - * cpus *(optional)*: The number of CPUs assigned to the VM. > Default is 1. > + * cpus *(optional)*: The number of CPUs assigned to the VM. > + Default is 1, unlees specifying a cpu topology. In that > case, cpus > + will default to a product of the topology values (see > cpu_info). > * memory *(optional)*: The amount of memory assigned to the VM. > Default is 1024M. > * cdrom *(optional)*: A volume name or URI to an ISO image. > @@ -201,6 +203,15 @@ 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. > + If specifying both cpus and CPU topology, make sure cpus is > + equal to the product of sockets, cores, and threads. > ### Sub-Collection: Virtual Machine Network Interfaces > diff --git a/po/en_US.po b/po/en_US.po > index eb571ca..a88675c 100644 > --- a/po/en_US.po > +++ b/po/en_US.po > @@ -371,6 +371,9 @@ msgstr "" > msgid "Cannot identify base image %(path)s format" > msgstr "" > +msgid "When specifying CPU topology, VCPUs must be a product of > sockets, cores, and threads." > +msgstr "" > + > #, python-format > msgid "Storage pool %(name)s already exists" > msgstr "" > diff --git a/po/pt_BR.po b/po/pt_BR.po > index f2fba64..1bebe30 100644 > --- a/po/pt_BR.po > +++ b/po/pt_BR.po > @@ -424,6 +424,9 @@ msgstr "Imagem base do modelo deve ser um arquivo > de imagem local válido" > msgid "Cannot identify base image %(path)s format" > msgstr "Não foi possível identificar o formato da imagem base %(path)s" > +msgid "When specifying CPU topology, VCPUs must be a product of > sockets, cores, and threads." > +msgstr "" > + > #, python-format > msgid "Storage pool %(name)s already exists" > msgstr "Storage pool %(name)s já existe" > diff --git a/po/zh_CN.po b/po/zh_CN.po > index f77e405..f944b8d 100644 > --- a/po/zh_CN.po > +++ b/po/zh_CN.po > @@ -392,6 +392,9 @@ msgstr "模板基础镜像必须为一个有效的本地镜像文件" > msgid "Cannot identify base image %(path)s format" > msgstr "未能识别基础镜像%(path)s格式" > +msgid "When specifying CPU topology, VCPUs must be a product of > sockets, cores, and threads." > +msgstr "" > + > #, python-format > msgid "Storage pool %(name)s already exists" > msgstr "存储池%(name)s已经存在" > diff --git a/src/kimchi/API.json b/src/kimchi/API.json > index 5b752dc..fe3df72 100644 > --- a/src/kimchi/API.json > +++ b/src/kimchi/API.json > @@ -26,6 +26,33 @@ > ] > } > } > + }, > + "cpu_info": { > + "description": "Configure CPU specifics for a VM.", > + "type": "object", > + "properties": { > + "topology": { > + "description": "Configure the guest CPU topology.", > + "type": "object", > + "properties": { > + "sockets": { > + "type": "integer", > + "required": true, > + "minimum": 1 > + }, > + "cores": { > + "type": "integer", > + "required": true, > + "minimum": 1 > + }, > + "threads": { > + "type": "integer", > + "required": true, > + "minimum": 1 > + } > + } > + } > + } The cpu_info is the cpu tolopoly, right? We could rename cpu_info to cpu_topology and eliminate one date structure level. > } > }, > "properties": { > @@ -448,7 +475,8 @@ > "type": "array", > "items": { "type": "string" } > }, > - "graphics": { "$ref": "#/kimchitype/graphics" } > + "graphics": { "$ref": "#/kimchitype/graphics" }, > + "cpu_info": { "$ref": "#/kimchitype/cpu_info" } > }, > "additionalProperties": false, > "error": "KCHAPI0001E" > @@ -612,7 +640,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..54caa92 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,10 @@ 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'] > + else: > + return_data['cpu_info'] = '' You could return it directly and let the model.lookup() care about the cpu_info value 'cpu_info': self.info['cpu_info'] > + return return_data > diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py > index 1b543ce..9ce8e86 100644 > --- a/src/kimchi/i18n.py > +++ b/src/kimchi/i18n.py > @@ -131,6 +131,7 @@ > "KCHTMPL0022E": _("Disk size must be an integer greater than > 1GB."), > "KCHTMPL0023E": _("Template base image must be a valid local > image file"), > "KCHTMPL0024E": _("Cannot identify base image %(path)s format"), > + "KCHTMPL0025E": _("When specifying CPU topology, VCPUs must be a > product of sockets, cores, and threads."), > "KCHPOOL0001E": _("Storage pool %(name)s already exists"), > "KCHPOOL0002E": _("Storage pool %(name)s does not exist"), > diff --git a/src/kimchi/model/templates.py > b/src/kimchi/model/templates.py > index 9278cdc..04d2af9 100644 > --- a/src/kimchi/model/templates.py > +++ b/src/kimchi/model/templates.py > @@ -48,6 +48,22 @@ def create(self, params): > {'filename': iso, 'user': user, > 'err': excp}) > > + cpu_info = params.get('cpu_info') > + if cpu_info: > + cpu_info = dict(params['cpu_info']) > + topology = cpu_info.get('topology') When there is a "cpu_info" parameter I need to have a "topology", right? Otherwise, we should raise an error. > + # Check, even though currently only topology > + # is supported. > + if topology: > + sockets = topology['sockets'] > + cores = topology['cores'] > + threads = topology['threads'] > + vcpus = params.get('cpus') > + if vcpus is None: > + params['cpus'] = sockets * cores * threads > + elif sockets * cores * threads != vcpus: > + raise InvalidParameter("KCHTMPL0025E") > + > conn = self.conn.get() > pool_uri = params.get(u'storagepool', '') > if pool_uri: > @@ -154,6 +170,9 @@ def delete(self, name): > def update(self, name, params): > old_t = self.lookup(name) > + if not self._verify_updated_cpu_params(params, old_t): > + raise InvalidParameter('KCHTMPL0025E') > + > new_t = copy.copy(old_t) > new_t.update(params) As you can see here, the new_t is the current Template with the updated values. From my view, you can get the new_t and pass the function above > ident = name > @@ -187,6 +206,39 @@ def update(self, name, params): > raise > return ident > + def _verify_updated_cpu_params(self, params, old_t): > + # Note: cpu_info is the parent of topology. cpus is vcpus > + # Keep two check_ variables for what we'll verify at the end to > + # keep from duplicating code. > + check_cpu = None > + check_topology = None > + # First see what parameters were passed in. > + new_vcpus = params.get('cpus') > + new_cpu_info = params.get('cpu_info') > + new_topology = '' > + if new_cpu_info: > + new_topology = new_cpu_info.get('topology') > + # Now figure out what needs to be verified. > + if new_vcpus and new_topology: > + check_cpu = new_vcpus > + check_topology = new_topology > + elif new_vcpus: > + old_cpu_info = old_t.get('cpu_info') > + if old_cpu_info: > + old_topology = old_cpu_info.get('topology') > + if old_topology: > + check_cpu = new_vcpus > + check_topology = old_topology > + elif new_topology: > + check_cpu = old_t.get('cpus') > + check_topology = new_topology > + # Now verify the cpu and topoloy parameters > + if check_cpu and (check_cpu != check_topology['sockets'] * \ > + check_topology['cores'] * check_topology['threads']): > + return False > + > + return True > + > class LibvirtVMTemplate(VMTemplate): > def __init__(self, args, scan=False, conn=None): > diff --git a/src/kimchi/osinfo.py b/src/kimchi/osinfo.py > index 6ee5e48..10d0ab0 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 data structure here? It should be: cpu_topology: {cores:.., thread:..., sockets:...} > '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 5f22db9..34720de 100644 > --- a/src/kimchi/vmtemplate.py > +++ b/src/kimchi/vmtemplate.py > @@ -358,6 +358,19 @@ 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') The same I commented before as "topology" is mandatory for cpu_info. > + if cpu_topo is not None: > + return etree.tostring(E.cpu(E.topology( > + sockets=str(cpu_topo['sockets']), > + cores=str(cpu_topo['cores']), > + threads=str(cpu_topo['threads'])))) > + > + return "" You can do: if cpu_info is None: return "" cpu_topo = ... if cpu_topo is None: return "" return etree... That way we reduce the indentation levels. > + > def to_vm_xml(self, vm_name, vm_uuid, **kwargs): > params = dict(self.info) > params['name'] = vm_name > @@ -369,6 +382,7 @@ def to_vm_xml(self, vm_name, vm_uuid, **kwargs): > params['qemu-stream-cmdline'] = '' > graphics = kwargs.get('graphics') > params['graphics'] = self._get_graphics_xml(graphics) > + params['cpu_info'] = self._get_cpu_xml() > # Current implementation just allows to create disk in one > single > # storage pool, so we cannot mix the types (scsi volumes vs > img file) > @@ -400,6 +414,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'/>