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

On 2014年10月04日 06:13, 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 > > 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 | 11 +++++++++- > po/en_US.po | 3 +++ > po/pt_BR.po | 3 +++ > po/zh_CN.po | 3 +++ > src/kimchi/API.json | 30 ++++++++++++++++++++++++-- > src/kimchi/control/templates.py | 13 +++++++++--- > src/kimchi/i18n.py | 1 + > src/kimchi/model/templates.py | 47 > +++++++++++++++++++++++++++++++++++++++++ > src/kimchi/osinfo.py | 2 +- > src/kimchi/vmtemplate.py | 15 +++++++++++++ > 10 files changed, 121 insertions(+), 7 deletions(-) > > diff --git a/docs/API.md b/docs/API.md > index cc438cc..6281f70 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. If specifying a CPU topology, make sure this > value is > + equal to the product of sockets, cores, and threads. If cpu_info is given, the cpus value seems redundant, what about: If specifying a CPU topology, this value is omitted and given the value of product of sockets, cores and threads. or we can just ensure only one of cpus or cpu_info-topology is assigned, what do you think?

> * 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,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/po/en_US.po b/po/en_US.po > index eb571ca..74f1ec6 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 "When specifying CPU topology, VCPUs must be a product of > sockets, cores, and threads." msgstr is not needed because if we can't find the translation, we just fallback to msgid;)

> + > #, 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..0fe3993 100644 > --- a/src/kimchi/API.json > +++ b/src/kimchi/API.json > @@ -26,6 +26,30 @@ > ] > } > } > + }, > + "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 > + }, As "integer" is "the set <http://en.wikipedia.org/wiki/Set_%28mathematics%29> of integers consists of zero <http://en.wikipedia.org/wiki/Zero> (0 <http://en.wikipedia.org/wiki/0_%28number%29>), the counting numbers <http://en.wikipedia.org/wiki/Counting_number> (1 <http://en.wikipedia.org/wiki/1_%28number%29>;, 2 <http://en.wikipedia.org/wiki/2_%28number%29>;, 3 <http://en.wikipedia.org/wiki/3_%28number%29>;, ...)", I think we need to add: "minimum": 1;

> + "cores": { > + "type": "integer", > + "required": true > + }, > + "threads": { > + "type": "integer", > + "required": true > + } > + } > + } > + } > } > }, > "properties": { > @@ -448,7 +472,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 +637,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'] = '' Can we just omit this value(omit the "else" part) if no cpu_info found? that way it'll be easier for UI to make judgement.

> + 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."), As I commented before

> "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..203c324 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 = int(topology.get('sockets')) > + cores = int(topology.get('cores')) > + threads = int(topology.get('threads')) Since we did validation of this param type , why do we do this conversion?

Also they are "required",so we do not need "get"

> + vcpus = params.get('cpus') > + if vcpus is None: > + vcpus = 1 > + if sockets * cores * threads != vcpus: > + raise InvalidParameter("KCHTMPL0025E") > + > conn = self.conn.get() > pool_uri = params.get(u'storagepool', '') > if pool_uri: > @@ -154,6 +170,37 @@ def delete(self, name): > def update(self, name, params): > old_t = self.lookup(name) > + # If changing vcpus, check for topology values, and vice versa > + # 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']): > + raise InvalidParameter('KCHTMPL0025E') I suggest we wrap this to a function so that update function won't be too long and easier to understand.

> + > new_t = copy.copy(old_t) > new_t.update(params) > ident = name > 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'/>