I talked to Aline about a lot of this, and I'll summarize quickly instead of addressing each item. 1. Aline didn't catch that the common_spec elements for cores and threads existed before my patch, so after she realized what I was trying to say, she agreed that removing them there and putting this information into my next patch set (the set that will make topology decisions based on guest and host) makes sense. 2. While topology is currently the only cpu item that Kimchi supports, there are others supported by libvirt that we may or may not add later. That's why I always check for topology. It's future-proofing. She's okay with leaving that in. Everything else she suggested I've put into v3 and sent. Regards, and thanks to Royce and Aline for all the good suggestions, - Christy On 10/13/2014 02:41 PM, Aline Manera wrote:
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@linux.vnet.ibm.com> --- 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'/>