On 01/27/2016 07:14 PM, Lucio Correia wrote:

- Update defalut values for maxvcpus and cpus - Rename check_topology() to check_cpu_info() to also verify cpus and maxvcpus and use it for simplification of all CPU parameter validations - Unduplicate and move _get_host_maxcpu() to CPUInfoModel as get_host_max_vcpus() - Update docs and APIs with maxvcpus attribute

Signed-off-by: Lucio Correia <luciojhc@linux.vnet.ibm.com> --- API.json | 12 ++++++++++ control/templates.py | 3 ++- docs/API.md | 24 +++++++++++++++---- i18n.py | 10 ++++---- model/cpuinfo.py | 59 ++++++++++++++++++++++++++++++++------------- model/templates.py | 67 ++++++++++++++++++++++++++++------------------------ model/vms.py | 25 +------------------- vmtemplate.py | 6 ++++- 8 files changed, 125 insertions(+), 81 deletions(-)

diff --git a/API.json b/API.json index 2b64d07..4876cc0 100644 --- a/API.json +++ b/API.json @@ -454,6 +454,12 @@ "minimum": 1, "error": "KCHTMPL0012E" }, + "maxvcpus": { + "description": "The maximum number of virtual CPUs that can be assigned to the VMs created from the template", + "type": "integer", + "minimum": 1, + "error": "KCHTMPL0012E" + }, "memory": { "description": "Memory (MB) for the template", "type": "integer", @@ -637,6 +643,12 @@ "minimum": 1, "error": "KCHTMPL0012E" }, + "maxvcpus": { + "description": "The maximum number of virtual CPUs that can be assigned to the VMs created from the template", + "type": "integer", + "minimum": 1, + "error": "KCHTMPL0012E" + }, "memory": { "description": "Memory (MB) for the template", "type": "integer", diff --git a/control/templates.py b/control/templates.py index 4cd70c2..fcc298e 100644 --- a/control/templates.py +++ b/control/templates.py @@ -1,7 +1,7 @@ # # Project Kimchi # -# Copyright IBM, Corp. 2013-2015 +# Copyright IBM, Corp. 2013-2016 # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -47,6 +47,7 @@ class Template(Resource): 'os_distro': self.info['os_distro'], 'os_version': self.info['os_version'], 'cpus': self.info['cpus'], + 'maxvcpus': self.info.get('maxvcpus'), 'memory': self.info['memory'], 'cdrom': self.info.get('cdrom', None), 'disks': self.info['disks'], diff --git a/docs/API.md b/docs/API.md index 5122a0c..4790d08 100644 --- a/docs/API.md +++ b/docs/API.md @@ -276,8 +276,11 @@ Represents a snapshot of the Virtual Machine's primary monitor. * 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, unlees specifying a cpu topology. In that case, cpus - will default to a product of the topology values (see cpu_info). + Default is 1, unless a CPU topology is specified. In that case, cpus + will default to maxvcpus value. + * maxvcpus *(optional)*: The maximum number of CPUs that can be assigned to

In API.json you said that maxvcpu is "The maximum number of virtual CPUs that can be assigned to the VMs..." and here looks like you missed the *virtual* word. Is it correct?

+ the VM. If a CPU topology is specified (see cpu_info), maxvcpus must + be a product of sockets, cores and threads. * memory *(optional)*: The amount of memory assigned to the VM. Default is 1024M. * cdrom *(optional)*: A volume name or URI to an ISO image. @@ -304,10 +307,10 @@ Represents a snapshot of the Virtual Machine's primary monitor. * 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. + * sockets - The maximum 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 + If specifying both maxvcpus and CPU topology, make sure maxvcpus is equal to the product of sockets, cores, and threads.

### Sub-Collection: Virtual Machine Network Interfaces @@ -379,6 +382,7 @@ A interface represents available network interface on VM. * os_distro: The operating system distribution * os_version: The version of the operating system distribution * cpus: The number of CPUs assigned to the VM + * maxvcpus: The maximum number of CPUs that can be assigned to the VM

Same as my first comment.

* memory: The amount of memory assigned to the VM in the unit of MB * cdrom: A volume name or URI to an ISO image * storagepool: URI of the storagepool where template allocates vm storage. @@ -416,6 +420,7 @@ A interface represents available network interface on VM. * os_distro: The operating system distribution * os_version: The version of the operating system distribution * cpus: The number of CPUs assigned to the VM + * maxvcpus: The maximum number of CPUs that can be assigned to the VM

Same here.

* memory: The amount of memory assigned to the VM * cdrom: A volume name or URI to an ISO image * networks *(optional)*: list of networks will be assigned to the new VM. @@ -435,6 +440,17 @@ A interface represents available network interface on VM. Independent Computing Environments * null: Graphics is disabled or type not supported * listen: The network which the vnc/spice server listens on. + * cpu_info: 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 maximum number of sockets to use. + * cores - The number of cores per socket. + * threads - The number of threads per core. + If CPU topology is specified, *cpus* and *maxvcpus* also must be + specified. Make sure *maxvcpus* is equal to the product of sockets, + cores and threads, and *cpus* is a multiple of a product of cores + and threads.

**Actions (POST):**

diff --git a/i18n.py b/i18n.py index a575922..bb6f3d1 100644 --- a/i18n.py +++ b/i18n.py @@ -1,7 +1,7 @@ # # Project Kimchi # -# Copyright IBM, Corp. 2014-2015 +# Copyright IBM, Corp. 2014-2016 # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -175,7 +175,6 @@ messages = { "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."), "KCHTMPL0026E": _("When specifying CPU topology, each element must be an integer greater than zero."), "KCHTMPL0027E": _("Invalid disk image format. Valid formats: bochs, cloop, cow, dmg, qcow, qcow2, qed, raw, vmdk, vpc."), "KCHTMPL0028E": _("When setting template disks, following parameters are required: 'index', 'pool name', 'format', 'size' or 'volume' (for scsi/iscsi pools)"), @@ -311,9 +310,12 @@ messages = { "KCHSNAP0009E": _("Unable to revert virtual machine '%(vm)s' to snapshot '%(name)s'. Details: %(err)s"), "KCHSNAP0010E": _("Unable to create snapshot of virtual machine '%(vm)s' because it contains a disk with format '%(format)s'; only 'qcow2' is supported."),

- "KCHCPUINF0001E": _("The number of vCPUs is too large for this system."), - "KCHCPUINF0002E": _("Invalid vCPU/topology combination."), + "KCHCPUINF0001E": _("The number of vCPUs must be less than or equal the maximum number of vCPUs specified."), + "KCHCPUINF0002E": _("When CPU topology is defined, maximum number of vCPUs must be a product of sockets, cores, and threads."), "KCHCPUINF0003E": _("This host (or current configuration) does not allow CPU topology."), + "KCHCPUINF0004E": _("The maximum number of vCPUs is too large for this system."), + "KCHCPUINF0005E": _("When CPU topology is defined, vCPUs must be a multiple of a product of cores and threads."), + "KCHCPUINF0006E": _("The number of threads is too large for this system."),

"KCHLVMS0001E": _("Invalid volume group name parameter: %(name)s."),

diff --git a/model/cpuinfo.py b/model/cpuinfo.py index 299e445..83e4ec2 100644 --- a/model/cpuinfo.py +++ b/model/cpuinfo.py @@ -1,7 +1,7 @@ # # Project Kimchi # -# Copyright IBM, Corp. 2014-2015 +# Copyright IBM, Corp. 2014-2016 # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -25,6 +25,7 @@ from wok.utils import run_command, wok_log

ARCH = 'power' if platform.machine().startswith('ppc') else 'x86' +MAX_PPC_VCPUS = 255

def get_topo_capabilities(connect): @@ -106,21 +107,47 @@ class CPUInfoModel(object): 'threads_per_core': self.threads_per_core, }

- def check_topology(self, vcpus, topology): + def check_cpu_info(self, params): """ - param vcpus: should be an integer - param iso_path: the path of the guest ISO - param topology: {'sockets': x, 'cores': x, 'threads': x} + param params: dict containing: + maxvcpus: integer + vcpus: integer + cpu_info: None if topology undefined or topology definition + dict: {'topology': { + 'sockets': x, + 'cores': x, + 'threads': x + } + } """ - sockets = topology['sockets'] - cores = topology['cores'] - threads = topology['threads'] - - if not self.guest_threads_enabled: - raise InvalidOperation("KCHCPUINF0003E") - if vcpus != sockets * cores * threads: - raise InvalidParameter("KCHCPUINF0002E") - if vcpus > self.cores_available * self.threads_per_core: + maxvcpus = params.get('maxvcpus') + vcpus = params.get('cpus') + topology = params.get('cpu_info', {}).get('topology', None) + if topology: + sockets = topology['sockets'] + cores = topology['cores'] + threads = topology['threads'] + + if not self.guest_threads_enabled: + raise InvalidOperation("KCHCPUINF0003E") + if threads > self.threads_per_core: + raise InvalidParameter("KCHCPUINF0006E") + if maxvcpus != sockets * cores * threads: + raise InvalidParameter("KCHCPUINF0002E") + if vcpus % (cores * threads) != 0: + raise InvalidParameter("KCHCPUINF0005E") + + if maxvcpus > self.get_host_max_vcpus(): + raise InvalidParameter("KCHCPUINF0004E") + if vcpus > maxvcpus: raise InvalidParameter("KCHCPUINF0001E") - if threads > self.threads_per_core: - raise InvalidParameter("KCHCPUINF0002E") + + def get_host_max_vcpus(self): + if ARCH == 'power': + max_vcpus = self.cores_available * self.threads_per_core + if max_vcpus > MAX_PPC_VCPUS: + max_vcpus = MAX_PPC_VCPUS + else: + max_vcpus = self.conn.get().getMaxVcpus('kvm') + + return max_vcpus diff --git a/model/templates.py b/model/templates.py index c9b11c3..1f29bd9 100644 --- a/model/templates.py +++ b/model/templates.py @@ -1,7 +1,7 @@ # # Project Kimchi # -# Copyright IBM, Corp. 2014-2015 +# Copyright IBM, Corp. 2014-2016 # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -54,22 +54,6 @@ class TemplatesModel(object): {'filename': iso, 'user': user, 'err': excp})

- cpu_info = params.get('cpu_info') - if 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'] - if params.get('cpus') is None: - params['cpus'] = sockets * cores * threads - # check_topoology will raise the appropriate - # exception if a topology is invalid. - CPUInfoModel(conn=self.conn).\ - check_topology(params['cpus'], topology) - conn = self.conn.get() for net_name in params.get(u'networks', []): try: @@ -82,6 +66,9 @@ class TemplatesModel(object): # will be raised here t = LibvirtVMTemplate(params, scan=True, conn=self.conn)

+ # Validate cpu info + t.cpuinfo_validate() + # Validate max memory maxMem = (t._get_max_memory(t.info.get('memory')) >> 10) if t.info.get('memory') > maxMem: @@ -180,9 +167,6 @@ class TemplateModel(object): new_t = copy.copy(old_t) new_t.update(params)

- if not self._validate_updated_cpu_params(new_t): - raise InvalidParameter('KCHTMPL0025E') - for net_name in params.get(u'networks', []): try: conn = self.conn.get() @@ -199,22 +183,18 @@ class TemplateModel(object): raise return ident

- def _validate_updated_cpu_params(self, info): - # Note: cpu_info is the parent of topology. cpus is vcpus - vcpus = info['cpus'] - cpu_info = info.get('cpu_info') - # cpu_info will always be at least an empty dict - topology = cpu_info.get('topology') - if topology is None: - return True - return vcpus == topology['sockets'] * topology['cores'] * \ - topology['threads'] -

class LibvirtVMTemplate(VMTemplate): def __init__(self, args, scan=False, conn=None): self.conn = conn VMTemplate.__init__(self, args, scan) + self.set_cpu_info(args) + + def cpuinfo_validate(self): + cpu_model = CPUInfoModel(conn=self.conn) + + # validate CPU info values - will raise appropriate exceptions + cpu_model.check_cpu_info(self.info)

def _storage_validate(self, pool_uri): pool_name = pool_name_from_uri(pool_uri) @@ -283,3 +263,28 @@ class LibvirtVMTemplate(VMTemplate): except libvirt.libvirtError as e: raise OperationFailed("KCHVMSTOR0008E", {'error': e.message}) return vol_list + + def set_cpu_info(self, params): + # undefined topology: consider these values to calculate maxvcpus + sockets = 1 + cores = 1 + threads = 1 + + # get topology values + topology = self.info.get('cpu_info', {}).get('topology', None) + if topology: + sockets = topology['sockets'] + cores = topology['cores'] + threads = topology['threads'] + + # maxvcpus not specified: use defaults + if 'maxvcpus' not in params: + cpus = self.info.get('cpus', None) + if cpus and not topology: + self.info['maxvcpus'] = cpus + else: + self.info['maxvcpus'] = sockets * cores * threads + + # current vcpus not specified: defaults is maxvcpus + if 'cpus' not in params: + self.info['cpus'] = self.info['maxvcpus'] diff --git a/model/vms.py b/model/vms.py index 1203c71..9aaf530 100644 --- a/model/vms.py +++ b/model/vms.py @@ -107,17 +107,6 @@ class VMsModel(object): self.caps = CapabilitiesModel(**kargs) self.task = TaskModel(**kargs)

- def _get_host_maxcpu(self): - if os.uname()[4] in ['ppc', 'ppc64', 'ppc64le']: - cpu_model = CPUInfoModel(conn=self.conn) - max_vcpu_val = (cpu_model.cores_available * - cpu_model.threads_per_core) - if max_vcpu_val > 255: - max_vcpu_val = 255 - else: - max_vcpu_val = self.conn.get().getMaxVcpus('kvm') - return max_vcpu_val - def create(self, params): t_name = template_name_from_uri(params['template']) vm_list = self.get_list() @@ -178,8 +167,7 @@ class VMsModel(object): stream_protocols = self.caps.libvirt_stream_protocols xml = t.to_vm_xml(name, vm_uuid, libvirt_stream_protocols=stream_protocols, - graphics=graphics, - max_vcpus=self._get_host_maxcpu()) + graphics=graphics)

cb('Defining new VM') try: @@ -940,17 +928,6 @@ class VMModel(object):

return ET.tostring(root, encoding="utf-8")

- def _get_host_maxcpu(self): - if os.uname()[4] in ['ppc', 'ppc64', 'ppc64le']: - cpu_model = CPUInfoModel(conn=self.conn) - max_vcpu_val = (cpu_model.cores_available * - cpu_model.threads_per_core) - if max_vcpu_val > 255: - max_vcpu_val = 255 - else: - max_vcpu_val = self.conn.get().getMaxVcpus('kvm') - return max_vcpu_val - def _live_vm_update(self, dom, params): self._vm_update_access_metadata(dom, params) if 'memory' in params and dom.isActive(): diff --git a/vmtemplate.py b/vmtemplate.py index d629226..2f524a7 100644 --- a/vmtemplate.py +++ b/vmtemplate.py @@ -1,7 +1,7 @@ # # Project Kimchi # -# Copyright IBM, Corp. 2013-2015 +# Copyright IBM, Corp. 2013-2016 # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -466,6 +466,10 @@ class VMTemplate(object): self._storage_validate(pool_uri) self._network_validate() self._iso_validate() + self.cpuinfo_validate() + + def cpuinfo_validate(self): + pass

def _iso_validate(self): pass

On 01/29/2016 03:02 PM, Lucio Correia wrote:

On 29-01-2016 14:54, Paulo Ricardo Paz Vital wrote:

...

On 01/27/2016 07:14 PM, Lucio Correia wrote:

...

- Update defalut values for maxvcpus and cpus - Rename check_topology() to check_cpu_info() to also verify cpus and maxvcpus and use it for simplification of all CPU parameter validations - Unduplicate and move _get_host_maxcpu() to CPUInfoModel as get_host_max_vcpus() - Update docs and APIs with maxvcpus attribute

Signed-off-by: Lucio Correia <luciojhc@linux.vnet.ibm.com> --- API.json | 12 ++++++++++ control/templates.py | 3 ++- docs/API.md | 24 +++++++++++++++---- i18n.py | 10 ++++---- model/cpuinfo.py | 59 ++++++++++++++++++++++++++++++++------------- model/templates.py | 67 ++++++++++++++++++++++++++++------------------------ model/vms.py | 25 +------------------- vmtemplate.py | 6 ++++- 8 files changed, 125 insertions(+), 81 deletions(-)

diff --git a/API.json b/API.json index 2b64d07..4876cc0 100644 --- a/API.json +++ b/API.json @@ -454,6 +454,12 @@ "minimum": 1, "error": "KCHTMPL0012E" }, + "maxvcpus": { + "description": "The maximum number of virtual CPUs that can be assigned to the VMs created from the template", + "type": "integer", + "minimum": 1, + "error": "KCHTMPL0012E" + }, "memory": { "description": "Memory (MB) for the template", "type": "integer", @@ -637,6 +643,12 @@ "minimum": 1, "error": "KCHTMPL0012E" }, + "maxvcpus": { + "description": "The maximum number of virtual CPUs that can be assigned to the VMs created from the template", + "type": "integer", + "minimum": 1, + "error": "KCHTMPL0012E" + }, "memory": { "description": "Memory (MB) for the template", "type": "integer", diff --git a/control/templates.py b/control/templates.py index 4cd70c2..fcc298e 100644 --- a/control/templates.py +++ b/control/templates.py @@ -1,7 +1,7 @@ # # Project Kimchi # -# Copyright IBM, Corp. 2013-2015 +# Copyright IBM, Corp. 2013-2016 # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -47,6 +47,7 @@ class Template(Resource): 'os_distro': self.info['os_distro'], 'os_version': self.info['os_version'], 'cpus': self.info['cpus'], + 'maxvcpus': self.info.get('maxvcpus'), 'memory': self.info['memory'], 'cdrom': self.info.get('cdrom', None), 'disks': self.info['disks'], diff --git a/docs/API.md b/docs/API.md index 5122a0c..4790d08 100644 --- a/docs/API.md +++ b/docs/API.md @@ -276,8 +276,11 @@ Represents a snapshot of the Virtual Machine's primary monitor. * 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, unlees specifying a cpu topology. In that case, cpus - will default to a product of the topology values (see cpu_info). + Default is 1, unless a CPU topology is specified. In that case, cpus + will default to maxvcpus value. + * maxvcpus *(optional)*: The maximum number of CPUs that can be assigned to

In API.json you said that maxvcpu is "The maximum number of virtual CPUs that can be assigned to the VMs..." and here looks like you missed the *virtual* word. Is it correct?

I tried to follow what was already in docs and UI. UI does not mention "virtual". I believe that's because in the context of guest, "virtual" is implicit.

See that I have not changed first line (* cpus...) and it does not mention "virtual".

We may want to do a patch to review and fix all occurrences and avoid confusion.

OK, so all patches are OK.

On 01/27/2016 07:14 PM, Lucio Correia wrote:

- Update defalut values for maxvcpus and cpus - Rename check_topology() to check_cpu_info() to also verify cpus and maxvcpus and use it for simplification of all CPU parameter validations - Unduplicate and move _get_host_maxcpu() to CPUInfoModel as get_host_max_vcpus() - Update docs and APIs with maxvcpus attribute

Signed-off-by: Lucio Correia <luciojhc@linux.vnet.ibm.com> --- API.json | 12 ++++++++++ control/templates.py | 3 ++- docs/API.md | 24 +++++++++++++++---- i18n.py | 10 ++++---- model/cpuinfo.py | 59 ++++++++++++++++++++++++++++++++------------- model/templates.py | 67 ++++++++++++++++++++++++++++------------------------ model/vms.py | 25 +------------------- vmtemplate.py | 6 ++++- 8 files changed, 125 insertions(+), 81 deletions(-)

diff --git a/API.json b/API.json index 2b64d07..4876cc0 100644 --- a/API.json +++ b/API.json @@ -454,6 +454,12 @@ "minimum": 1, "error": "KCHTMPL0012E" },

+ "maxvcpus": { + "description": "The maximum number of virtual CPUs that can be assigned to the VMs created from the template", + "type": "integer", + "minimum": 1, + "error": "KCHTMPL0012E" + },

It is better to concentrate all the CPU information in the same object That way the 'cpus' would be changed to 'cpu' with 'current' and 'max' values {.. cpu: {current: ..., max:...}} It would be good to also include 'cpu_info[topology]' in the same data structure. {... cpu: {current:..., max:..., topology: ...} To include topology in the same data structure will require a lot of changes. So it is better to do in a separated patch after getting this one (related to max cpus) merged.

"memory": { "description": "Memory (MB) for the template", "type": "integer", @@ -637,6 +643,12 @@ "minimum": 1, "error": "KCHTMPL0012E" }, + "maxvcpus": { + "description": "The maximum number of virtual CPUs that can be assigned to the VMs created from the template", + "type": "integer", + "minimum": 1, + "error": "KCHTMPL0012E" + }, "memory": { "description": "Memory (MB) for the template", "type": "integer", diff --git a/control/templates.py b/control/templates.py index 4cd70c2..fcc298e 100644 --- a/control/templates.py +++ b/control/templates.py @@ -1,7 +1,7 @@ # # Project Kimchi # -# Copyright IBM, Corp. 2013-2015 +# Copyright IBM, Corp. 2013-2016 # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -47,6 +47,7 @@ class Template(Resource): 'os_distro': self.info['os_distro'], 'os_version': self.info['os_version'], 'cpus': self.info['cpus'], + 'maxvcpus': self.info.get('maxvcpus'), 'memory': self.info['memory'], 'cdrom': self.info.get('cdrom', None), 'disks': self.info['disks'], diff --git a/docs/API.md b/docs/API.md index 5122a0c..4790d08 100644 --- a/docs/API.md +++ b/docs/API.md @@ -276,8 +276,11 @@ Represents a snapshot of the Virtual Machine's primary monitor. * 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, unlees specifying a cpu topology. In that case, cpus - will default to a product of the topology values (see cpu_info). + Default is 1, unless a CPU topology is specified. In that case, cpus + will default to maxvcpus value. + * maxvcpus *(optional)*: The maximum number of CPUs that can be assigned to + the VM. If a CPU topology is specified (see cpu_info), maxvcpus must + be a product of sockets, cores and threads. * memory *(optional)*: The amount of memory assigned to the VM. Default is 1024M. * cdrom *(optional)*: A volume name or URI to an ISO image. @@ -304,10 +307,10 @@ Represents a snapshot of the Virtual Machine's primary monitor. * 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. + * sockets - The maximum 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 + If specifying both maxvcpus and CPU topology, make sure maxvcpus is equal to the product of sockets, cores, and threads.

### Sub-Collection: Virtual Machine Network Interfaces @@ -379,6 +382,7 @@ A interface represents available network interface on VM. * os_distro: The operating system distribution * os_version: The version of the operating system distribution * cpus: The number of CPUs assigned to the VM + * maxvcpus: The maximum number of CPUs that can be assigned to the VM * memory: The amount of memory assigned to the VM in the unit of MB * cdrom: A volume name or URI to an ISO image * storagepool: URI of the storagepool where template allocates vm storage. @@ -416,6 +420,7 @@ A interface represents available network interface on VM. * os_distro: The operating system distribution * os_version: The version of the operating system distribution * cpus: The number of CPUs assigned to the VM + * maxvcpus: The maximum number of CPUs that can be assigned to the VM * memory: The amount of memory assigned to the VM * cdrom: A volume name or URI to an ISO image * networks *(optional)*: list of networks will be assigned to the new VM. @@ -435,6 +440,17 @@ A interface represents available network interface on VM. Independent Computing Environments * null: Graphics is disabled or type not supported * listen: The network which the vnc/spice server listens on. + * cpu_info: 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 maximum number of sockets to use. + * cores - The number of cores per socket. + * threads - The number of threads per core. + If CPU topology is specified, *cpus* and *maxvcpus* also must be + specified. Make sure *maxvcpus* is equal to the product of sockets, + cores and threads, and *cpus* is a multiple of a product of cores + and threads.

**Actions (POST):**

diff --git a/i18n.py b/i18n.py index a575922..bb6f3d1 100644 --- a/i18n.py +++ b/i18n.py @@ -1,7 +1,7 @@ # # Project Kimchi # -# Copyright IBM, Corp. 2014-2015 +# Copyright IBM, Corp. 2014-2016 # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -175,7 +175,6 @@ messages = { "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."), "KCHTMPL0026E": _("When specifying CPU topology, each element must be an integer greater than zero."), "KCHTMPL0027E": _("Invalid disk image format. Valid formats: bochs, cloop, cow, dmg, qcow, qcow2, qed, raw, vmdk, vpc."), "KCHTMPL0028E": _("When setting template disks, following parameters are required: 'index', 'pool name', 'format', 'size' or 'volume' (for scsi/iscsi pools)"), @@ -311,9 +310,12 @@ messages = { "KCHSNAP0009E": _("Unable to revert virtual machine '%(vm)s' to snapshot '%(name)s'. Details: %(err)s"), "KCHSNAP0010E": _("Unable to create snapshot of virtual machine '%(vm)s' because it contains a disk with format '%(format)s'; only 'qcow2' is supported."),

- "KCHCPUINF0001E": _("The number of vCPUs is too large for this system."), - "KCHCPUINF0002E": _("Invalid vCPU/topology combination."), + "KCHCPUINF0001E": _("The number of vCPUs must be less than or equal the maximum number of vCPUs specified."), + "KCHCPUINF0002E": _("When CPU topology is defined, maximum number of vCPUs must be a product of sockets, cores, and threads."), "KCHCPUINF0003E": _("This host (or current configuration) does not allow CPU topology."), + "KCHCPUINF0004E": _("The maximum number of vCPUs is too large for this system."), + "KCHCPUINF0005E": _("When CPU topology is defined, vCPUs must be a multiple of a product of cores and threads."), + "KCHCPUINF0006E": _("The number of threads is too large for this system."),

"KCHLVMS0001E": _("Invalid volume group name parameter: %(name)s."),

diff --git a/model/cpuinfo.py b/model/cpuinfo.py index 299e445..83e4ec2 100644 --- a/model/cpuinfo.py +++ b/model/cpuinfo.py @@ -1,7 +1,7 @@ # # Project Kimchi # -# Copyright IBM, Corp. 2014-2015 +# Copyright IBM, Corp. 2014-2016 # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -25,6 +25,7 @@ from wok.utils import run_command, wok_log

ARCH = 'power' if platform.machine().startswith('ppc') else 'x86' +MAX_PPC_VCPUS = 255

def get_topo_capabilities(connect): @@ -106,21 +107,47 @@ class CPUInfoModel(object): 'threads_per_core': self.threads_per_core, }

- def check_topology(self, vcpus, topology): + def check_cpu_info(self, params): """ - param vcpus: should be an integer - param iso_path: the path of the guest ISO - param topology: {'sockets': x, 'cores': x, 'threads': x} + param params: dict containing: + maxvcpus: integer + vcpus: integer + cpu_info: None if topology undefined or topology definition + dict: {'topology': { + 'sockets': x, + 'cores': x, + 'threads': x + } + } """ - sockets = topology['sockets'] - cores = topology['cores'] - threads = topology['threads'] - - if not self.guest_threads_enabled: - raise InvalidOperation("KCHCPUINF0003E") - if vcpus != sockets * cores * threads: - raise InvalidParameter("KCHCPUINF0002E") - if vcpus > self.cores_available * self.threads_per_core: + maxvcpus = params.get('maxvcpus') + vcpus = params.get('cpus') + topology = params.get('cpu_info', {}).get('topology', None) + if topology: + sockets = topology['sockets'] + cores = topology['cores'] + threads = topology['threads'] + + if not self.guest_threads_enabled: + raise InvalidOperation("KCHCPUINF0003E") + if threads > self.threads_per_core: + raise InvalidParameter("KCHCPUINF0006E") + if maxvcpus != sockets * cores * threads: + raise InvalidParameter("KCHCPUINF0002E") + if vcpus % (cores * threads) != 0: + raise InvalidParameter("KCHCPUINF0005E") + + if maxvcpus > self.get_host_max_vcpus(): + raise InvalidParameter("KCHCPUINF0004E") + if vcpus > maxvcpus: raise InvalidParameter("KCHCPUINF0001E") - if threads > self.threads_per_core: - raise InvalidParameter("KCHCPUINF0002E") + + def get_host_max_vcpus(self): + if ARCH == 'power': + max_vcpus = self.cores_available * self.threads_per_core + if max_vcpus > MAX_PPC_VCPUS: + max_vcpus = MAX_PPC_VCPUS + else: + max_vcpus = self.conn.get().getMaxVcpus('kvm') + + return max_vcpus diff --git a/model/templates.py b/model/templates.py index c9b11c3..1f29bd9 100644 --- a/model/templates.py +++ b/model/templates.py @@ -1,7 +1,7 @@ # # Project Kimchi # -# Copyright IBM, Corp. 2014-2015 +# Copyright IBM, Corp. 2014-2016 # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -54,22 +54,6 @@ class TemplatesModel(object): {'filename': iso, 'user': user, 'err': excp})

- cpu_info = params.get('cpu_info') - if 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'] - if params.get('cpus') is None: - params['cpus'] = sockets * cores * threads - # check_topoology will raise the appropriate - # exception if a topology is invalid. - CPUInfoModel(conn=self.conn).\ - check_topology(params['cpus'], topology) - conn = self.conn.get() for net_name in params.get(u'networks', []): try: @@ -82,6 +66,9 @@ class TemplatesModel(object): # will be raised here t = LibvirtVMTemplate(params, scan=True, conn=self.conn)

+ # Validate cpu info + t.cpuinfo_validate() + # Validate max memory maxMem = (t._get_max_memory(t.info.get('memory')) >> 10) if t.info.get('memory') > maxMem: @@ -180,9 +167,6 @@ class TemplateModel(object): new_t = copy.copy(old_t) new_t.update(params)

- if not self._validate_updated_cpu_params(new_t): - raise InvalidParameter('KCHTMPL0025E') - for net_name in params.get(u'networks', []): try: conn = self.conn.get() @@ -199,22 +183,18 @@ class TemplateModel(object): raise return ident

- def _validate_updated_cpu_params(self, info): - # Note: cpu_info is the parent of topology. cpus is vcpus - vcpus = info['cpus'] - cpu_info = info.get('cpu_info') - # cpu_info will always be at least an empty dict - topology = cpu_info.get('topology') - if topology is None: - return True - return vcpus == topology['sockets'] * topology['cores'] * \ - topology['threads'] -

class LibvirtVMTemplate(VMTemplate): def __init__(self, args, scan=False, conn=None): self.conn = conn VMTemplate.__init__(self, args, scan) + self.set_cpu_info(args) + + def cpuinfo_validate(self): + cpu_model = CPUInfoModel(conn=self.conn) + + # validate CPU info values - will raise appropriate exceptions + cpu_model.check_cpu_info(self.info)

def _storage_validate(self, pool_uri): pool_name = pool_name_from_uri(pool_uri) @@ -283,3 +263,28 @@ class LibvirtVMTemplate(VMTemplate): except libvirt.libvirtError as e: raise OperationFailed("KCHVMSTOR0008E", {'error': e.message}) return vol_list + + def set_cpu_info(self, params): + # undefined topology: consider these values to calculate maxvcpus + sockets = 1 + cores = 1 + threads = 1 + + # get topology values + topology = self.info.get('cpu_info', {}).get('topology', None) + if topology: + sockets = topology['sockets'] + cores = topology['cores'] + threads = topology['threads'] + + # maxvcpus not specified: use defaults + if 'maxvcpus' not in params: + cpus = self.info.get('cpus', None) + if cpus and not topology: + self.info['maxvcpus'] = cpus + else: + self.info['maxvcpus'] = sockets * cores * threads + + # current vcpus not specified: defaults is maxvcpus + if 'cpus' not in params: + self.info['cpus'] = self.info['maxvcpus'] diff --git a/model/vms.py b/model/vms.py index 1203c71..9aaf530 100644 --- a/model/vms.py +++ b/model/vms.py @@ -107,17 +107,6 @@ class VMsModel(object): self.caps = CapabilitiesModel(**kargs) self.task = TaskModel(**kargs)

- def _get_host_maxcpu(self): - if os.uname()[4] in ['ppc', 'ppc64', 'ppc64le']: - cpu_model = CPUInfoModel(conn=self.conn) - max_vcpu_val = (cpu_model.cores_available * - cpu_model.threads_per_core) - if max_vcpu_val > 255: - max_vcpu_val = 255 - else: - max_vcpu_val = self.conn.get().getMaxVcpus('kvm') - return max_vcpu_val - def create(self, params): t_name = template_name_from_uri(params['template']) vm_list = self.get_list() @@ -178,8 +167,7 @@ class VMsModel(object): stream_protocols = self.caps.libvirt_stream_protocols xml = t.to_vm_xml(name, vm_uuid, libvirt_stream_protocols=stream_protocols, - graphics=graphics, - max_vcpus=self._get_host_maxcpu()) + graphics=graphics)

cb('Defining new VM') try: @@ -940,17 +928,6 @@ class VMModel(object):

return ET.tostring(root, encoding="utf-8")

- def _get_host_maxcpu(self): - if os.uname()[4] in ['ppc', 'ppc64', 'ppc64le']: - cpu_model = CPUInfoModel(conn=self.conn) - max_vcpu_val = (cpu_model.cores_available * - cpu_model.threads_per_core) - if max_vcpu_val > 255: - max_vcpu_val = 255 - else: - max_vcpu_val = self.conn.get().getMaxVcpus('kvm') - return max_vcpu_val - def _live_vm_update(self, dom, params): self._vm_update_access_metadata(dom, params) if 'memory' in params and dom.isActive(): diff --git a/vmtemplate.py b/vmtemplate.py index d629226..2f524a7 100644 --- a/vmtemplate.py +++ b/vmtemplate.py @@ -1,7 +1,7 @@ # # Project Kimchi # -# Copyright IBM, Corp. 2013-2015 +# Copyright IBM, Corp. 2013-2016 # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -466,6 +466,10 @@ class VMTemplate(object): self._storage_validate(pool_uri) self._network_validate() self._iso_validate() + self.cpuinfo_validate() + + def cpuinfo_validate(self): + pass

def _iso_validate(self): pass

On 01/29/2016 03:15 PM, Aline Manera wrote:

On 01/29/2016 03:12 PM, Aline Manera wrote:

...

On 01/27/2016 07:14 PM, Lucio Correia wrote:

...

- Update defalut values for maxvcpus and cpus - Rename check_topology() to check_cpu_info() to also verify cpus and maxvcpus and use it for simplification of all CPU parameter validations - Unduplicate and move _get_host_maxcpu() to CPUInfoModel as get_host_max_vcpus() - Update docs and APIs with maxvcpus attribute

Signed-off-by: Lucio Correia <luciojhc@linux.vnet.ibm.com> --- API.json | 12 ++++++++++ control/templates.py | 3 ++- docs/API.md | 24 +++++++++++++++---- i18n.py | 10 ++++---- model/cpuinfo.py | 59 ++++++++++++++++++++++++++++++++------------- model/templates.py | 67 ++++++++++++++++++++++++++++------------------------ model/vms.py | 25 +------------------- vmtemplate.py | 6 ++++- 8 files changed, 125 insertions(+), 81 deletions(-)

diff --git a/API.json b/API.json index 2b64d07..4876cc0 100644 --- a/API.json +++ b/API.json @@ -454,6 +454,12 @@ "minimum": 1, "error": "KCHTMPL0012E" },

...

+ "maxvcpus": { + "description": "The maximum number of virtual CPUs that can be assigned to the VMs created from the template", + "type": "integer", + "minimum": 1, + "error": "KCHTMPL0012E" + },

It is better to concentrate all the CPU information in the same object

That way the 'cpus' would be changed to 'cpu' with 'current' and 'max' values

{.. cpu: {current: ..., max:...}}

Or extend the current cpu_info to include current and max values for cpu.

cpu_info: {topology:..., current:..., max:...}

s/current/cpus s/max/maxvcpus =)

Maybe it turns on a few changes needed to have all the CPU details in one data structure.

...

It would be good to also include 'cpu_info[topology]' in the same data structure.

{... cpu: {current:..., max:..., topology: ...}

To include topology in the same data structure will require a lot of changes. So it is better to do in a separated patch after getting this one (related to max cpus) merged.

...

"memory": { "description": "Memory (MB) for the template", "type": "integer", @@ -637,6 +643,12 @@ "minimum": 1, "error": "KCHTMPL0012E" }, + "maxvcpus": { + "description": "The maximum number of virtual CPUs that can be assigned to the VMs created from the template", + "type": "integer", + "minimum": 1, + "error": "KCHTMPL0012E" + }, "memory": { "description": "Memory (MB) for the template", "type": "integer", diff --git a/control/templates.py b/control/templates.py index 4cd70c2..fcc298e 100644 --- a/control/templates.py +++ b/control/templates.py @@ -1,7 +1,7 @@ # # Project Kimchi # -# Copyright IBM, Corp. 2013-2015 +# Copyright IBM, Corp. 2013-2016 # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -47,6 +47,7 @@ class Template(Resource): 'os_distro': self.info['os_distro'], 'os_version': self.info['os_version'], 'cpus': self.info['cpus'], + 'maxvcpus': self.info.get('maxvcpus'), 'memory': self.info['memory'], 'cdrom': self.info.get('cdrom', None), 'disks': self.info['disks'], diff --git a/docs/API.md b/docs/API.md index 5122a0c..4790d08 100644 --- a/docs/API.md +++ b/docs/API.md @@ -276,8 +276,11 @@ Represents a snapshot of the Virtual Machine's primary monitor. * 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, unlees specifying a cpu topology. In that case, cpus - will default to a product of the topology values (see cpu_info). + Default is 1, unless a CPU topology is specified. In that case, cpus + will default to maxvcpus value. + * maxvcpus *(optional)*: The maximum number of CPUs that can be assigned to + the VM. If a CPU topology is specified (see cpu_info), maxvcpus must + be a product of sockets, cores and threads. * memory *(optional)*: The amount of memory assigned to the VM. Default is 1024M. * cdrom *(optional)*: A volume name or URI to an ISO image. @@ -304,10 +307,10 @@ Represents a snapshot of the Virtual Machine's primary monitor. * 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. + * sockets - The maximum 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 + If specifying both maxvcpus and CPU topology, make sure maxvcpus is equal to the product of sockets, cores, and threads.

### Sub-Collection: Virtual Machine Network Interfaces @@ -379,6 +382,7 @@ A interface represents available network interface on VM. * os_distro: The operating system distribution * os_version: The version of the operating system distribution * cpus: The number of CPUs assigned to the VM + * maxvcpus: The maximum number of CPUs that can be assigned to the VM * memory: The amount of memory assigned to the VM in the unit of MB * cdrom: A volume name or URI to an ISO image * storagepool: URI of the storagepool where template allocates vm storage. @@ -416,6 +420,7 @@ A interface represents available network interface on VM. * os_distro: The operating system distribution * os_version: The version of the operating system distribution * cpus: The number of CPUs assigned to the VM + * maxvcpus: The maximum number of CPUs that can be assigned to the VM * memory: The amount of memory assigned to the VM * cdrom: A volume name or URI to an ISO image * networks *(optional)*: list of networks will be assigned to the new VM. @@ -435,6 +440,17 @@ A interface represents available network interface on VM. Independent Computing Environments * null: Graphics is disabled or type not supported * listen: The network which the vnc/spice server listens on. + * cpu_info: 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 maximum number of sockets to use. + * cores - The number of cores per socket. + * threads - The number of threads per core. + If CPU topology is specified, *cpus* and *maxvcpus* also must be + specified. Make sure *maxvcpus* is equal to the product of sockets, + cores and threads, and *cpus* is a multiple of a product of cores + and threads.

**Actions (POST):**

diff --git a/i18n.py b/i18n.py index a575922..bb6f3d1 100644 --- a/i18n.py +++ b/i18n.py @@ -1,7 +1,7 @@ # # Project Kimchi # -# Copyright IBM, Corp. 2014-2015 +# Copyright IBM, Corp. 2014-2016 # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -175,7 +175,6 @@ messages = { "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."), "KCHTMPL0026E": _("When specifying CPU topology, each element must be an integer greater than zero."), "KCHTMPL0027E": _("Invalid disk image format. Valid formats: bochs, cloop, cow, dmg, qcow, qcow2, qed, raw, vmdk, vpc."), "KCHTMPL0028E": _("When setting template disks, following parameters are required: 'index', 'pool name', 'format', 'size' or 'volume' (for scsi/iscsi pools)"), @@ -311,9 +310,12 @@ messages = { "KCHSNAP0009E": _("Unable to revert virtual machine '%(vm)s' to snapshot '%(name)s'. Details: %(err)s"), "KCHSNAP0010E": _("Unable to create snapshot of virtual machine '%(vm)s' because it contains a disk with format '%(format)s'; only 'qcow2' is supported."),

- "KCHCPUINF0001E": _("The number of vCPUs is too large for this system."), - "KCHCPUINF0002E": _("Invalid vCPU/topology combination."), + "KCHCPUINF0001E": _("The number of vCPUs must be less than or equal the maximum number of vCPUs specified."), + "KCHCPUINF0002E": _("When CPU topology is defined, maximum number of vCPUs must be a product of sockets, cores, and threads."), "KCHCPUINF0003E": _("This host (or current configuration) does not allow CPU topology."), + "KCHCPUINF0004E": _("The maximum number of vCPUs is too large for this system."), + "KCHCPUINF0005E": _("When CPU topology is defined, vCPUs must be a multiple of a product of cores and threads."), + "KCHCPUINF0006E": _("The number of threads is too large for this system."),

"KCHLVMS0001E": _("Invalid volume group name parameter: %(name)s."),

diff --git a/model/cpuinfo.py b/model/cpuinfo.py index 299e445..83e4ec2 100644 --- a/model/cpuinfo.py +++ b/model/cpuinfo.py @@ -1,7 +1,7 @@ # # Project Kimchi # -# Copyright IBM, Corp. 2014-2015 +# Copyright IBM, Corp. 2014-2016 # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -25,6 +25,7 @@ from wok.utils import run_command, wok_log

ARCH = 'power' if platform.machine().startswith('ppc') else 'x86' +MAX_PPC_VCPUS = 255

def get_topo_capabilities(connect): @@ -106,21 +107,47 @@ class CPUInfoModel(object): 'threads_per_core': self.threads_per_core, }

- def check_topology(self, vcpus, topology): + def check_cpu_info(self, params): """ - param vcpus: should be an integer - param iso_path: the path of the guest ISO - param topology: {'sockets': x, 'cores': x, 'threads': x} + param params: dict containing: + maxvcpus: integer + vcpus: integer + cpu_info: None if topology undefined or topology definition + dict: {'topology': { + 'sockets': x, + 'cores': x, + 'threads': x + } + } """ - sockets = topology['sockets'] - cores = topology['cores'] - threads = topology['threads'] - - if not self.guest_threads_enabled: - raise InvalidOperation("KCHCPUINF0003E") - if vcpus != sockets * cores * threads: - raise InvalidParameter("KCHCPUINF0002E") - if vcpus > self.cores_available * self.threads_per_core: + maxvcpus = params.get('maxvcpus') + vcpus = params.get('cpus') + topology = params.get('cpu_info', {}).get('topology', None) + if topology: + sockets = topology['sockets'] + cores = topology['cores'] + threads = topology['threads'] + + if not self.guest_threads_enabled: + raise InvalidOperation("KCHCPUINF0003E") + if threads > self.threads_per_core: + raise InvalidParameter("KCHCPUINF0006E") + if maxvcpus != sockets * cores * threads: + raise InvalidParameter("KCHCPUINF0002E") + if vcpus % (cores * threads) != 0: + raise InvalidParameter("KCHCPUINF0005E") + + if maxvcpus > self.get_host_max_vcpus(): + raise InvalidParameter("KCHCPUINF0004E") + if vcpus > maxvcpus: raise InvalidParameter("KCHCPUINF0001E") - if threads > self.threads_per_core: - raise InvalidParameter("KCHCPUINF0002E") + + def get_host_max_vcpus(self): + if ARCH == 'power': + max_vcpus = self.cores_available * self.threads_per_core + if max_vcpus > MAX_PPC_VCPUS: + max_vcpus = MAX_PPC_VCPUS + else: + max_vcpus = self.conn.get().getMaxVcpus('kvm') + + return max_vcpus diff --git a/model/templates.py b/model/templates.py index c9b11c3..1f29bd9 100644 --- a/model/templates.py +++ b/model/templates.py @@ -1,7 +1,7 @@ # # Project Kimchi # -# Copyright IBM, Corp. 2014-2015 +# Copyright IBM, Corp. 2014-2016 # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -54,22 +54,6 @@ class TemplatesModel(object): {'filename': iso, 'user': user, 'err': excp})

- cpu_info = params.get('cpu_info') - if 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'] - if params.get('cpus') is None: - params['cpus'] = sockets * cores * threads - # check_topoology will raise the appropriate - # exception if a topology is invalid. - CPUInfoModel(conn=self.conn).\ - check_topology(params['cpus'], topology) - conn = self.conn.get() for net_name in params.get(u'networks', []): try: @@ -82,6 +66,9 @@ class TemplatesModel(object): # will be raised here t = LibvirtVMTemplate(params, scan=True, conn=self.conn)

+ # Validate cpu info + t.cpuinfo_validate() + # Validate max memory maxMem = (t._get_max_memory(t.info.get('memory')) >> 10) if t.info.get('memory') > maxMem: @@ -180,9 +167,6 @@ class TemplateModel(object): new_t = copy.copy(old_t) new_t.update(params)

- if not self._validate_updated_cpu_params(new_t): - raise InvalidParameter('KCHTMPL0025E') - for net_name in params.get(u'networks', []): try: conn = self.conn.get() @@ -199,22 +183,18 @@ class TemplateModel(object): raise return ident

- def _validate_updated_cpu_params(self, info): - # Note: cpu_info is the parent of topology. cpus is vcpus - vcpus = info['cpus'] - cpu_info = info.get('cpu_info') - # cpu_info will always be at least an empty dict - topology = cpu_info.get('topology') - if topology is None: - return True - return vcpus == topology['sockets'] * topology['cores'] * \ - topology['threads'] -

class LibvirtVMTemplate(VMTemplate): def __init__(self, args, scan=False, conn=None): self.conn = conn VMTemplate.__init__(self, args, scan) + self.set_cpu_info(args) + + def cpuinfo_validate(self): + cpu_model = CPUInfoModel(conn=self.conn) + + # validate CPU info values - will raise appropriate exceptions + cpu_model.check_cpu_info(self.info)

def _storage_validate(self, pool_uri): pool_name = pool_name_from_uri(pool_uri) @@ -283,3 +263,28 @@ class LibvirtVMTemplate(VMTemplate): except libvirt.libvirtError as e: raise OperationFailed("KCHVMSTOR0008E", {'error': e.message}) return vol_list + + def set_cpu_info(self, params): + # undefined topology: consider these values to calculate maxvcpus + sockets = 1 + cores = 1 + threads = 1 + + # get topology values + topology = self.info.get('cpu_info', {}).get('topology', None) + if topology: + sockets = topology['sockets'] + cores = topology['cores'] + threads = topology['threads'] + + # maxvcpus not specified: use defaults + if 'maxvcpus' not in params: + cpus = self.info.get('cpus', None) + if cpus and not topology: + self.info['maxvcpus'] = cpus + else: + self.info['maxvcpus'] = sockets * cores * threads + + # current vcpus not specified: defaults is maxvcpus + if 'cpus' not in params: + self.info['cpus'] = self.info['maxvcpus'] diff --git a/model/vms.py b/model/vms.py index 1203c71..9aaf530 100644 --- a/model/vms.py +++ b/model/vms.py @@ -107,17 +107,6 @@ class VMsModel(object): self.caps = CapabilitiesModel(**kargs) self.task = TaskModel(**kargs)

- def _get_host_maxcpu(self): - if os.uname()[4] in ['ppc', 'ppc64', 'ppc64le']: - cpu_model = CPUInfoModel(conn=self.conn) - max_vcpu_val = (cpu_model.cores_available * - cpu_model.threads_per_core) - if max_vcpu_val > 255: - max_vcpu_val = 255 - else: - max_vcpu_val = self.conn.get().getMaxVcpus('kvm') - return max_vcpu_val - def create(self, params): t_name = template_name_from_uri(params['template']) vm_list = self.get_list() @@ -178,8 +167,7 @@ class VMsModel(object): stream_protocols = self.caps.libvirt_stream_protocols xml = t.to_vm_xml(name, vm_uuid, libvirt_stream_protocols=stream_protocols, - graphics=graphics, - max_vcpus=self._get_host_maxcpu()) + graphics=graphics)

cb('Defining new VM') try: @@ -940,17 +928,6 @@ class VMModel(object):

return ET.tostring(root, encoding="utf-8")

- def _get_host_maxcpu(self): - if os.uname()[4] in ['ppc', 'ppc64', 'ppc64le']: - cpu_model = CPUInfoModel(conn=self.conn) - max_vcpu_val = (cpu_model.cores_available * - cpu_model.threads_per_core) - if max_vcpu_val > 255: - max_vcpu_val = 255 - else: - max_vcpu_val = self.conn.get().getMaxVcpus('kvm') - return max_vcpu_val - def _live_vm_update(self, dom, params): self._vm_update_access_metadata(dom, params) if 'memory' in params and dom.isActive(): diff --git a/vmtemplate.py b/vmtemplate.py index d629226..2f524a7 100644 --- a/vmtemplate.py +++ b/vmtemplate.py @@ -1,7 +1,7 @@ # # Project Kimchi # -# Copyright IBM, Corp. 2013-2015 +# Copyright IBM, Corp. 2013-2016 # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -466,6 +466,10 @@ class VMTemplate(object): self._storage_validate(pool_uri) self._network_validate() self._iso_validate() + self.cpuinfo_validate() + + def cpuinfo_validate(self): + pass

def _iso_validate(self): pass

_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel

_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel

On 01/29/2016 03:25 PM, Lucio Correia wrote:

On 29-01-2016 15:12, Aline Manera wrote:

...

On 01/27/2016 07:14 PM, Lucio Correia wrote:

...

- Update defalut values for maxvcpus and cpus - Rename check_topology() to check_cpu_info() to also verify cpus and maxvcpus and use it for simplification of all CPU parameter validations - Unduplicate and move _get_host_maxcpu() to CPUInfoModel as get_host_max_vcpus() - Update docs and APIs with maxvcpus attribute

Signed-off-by: Lucio Correia <luciojhc@linux.vnet.ibm.com> --- API.json | 12 ++++++++++ control/templates.py | 3 ++- docs/API.md | 24 +++++++++++++++---- i18n.py | 10 ++++---- model/cpuinfo.py | 59 ++++++++++++++++++++++++++++++++------------- model/templates.py | 67 ++++++++++++++++++++++++++++------------------------ model/vms.py | 25 +------------------- vmtemplate.py | 6 ++++- 8 files changed, 125 insertions(+), 81 deletions(-)

diff --git a/API.json b/API.json index 2b64d07..4876cc0 100644 --- a/API.json +++ b/API.json @@ -454,6 +454,12 @@ "minimum": 1, "error": "KCHTMPL0012E" },

...

+ "maxvcpus": { + "description": "The maximum number of virtual CPUs that can be assigned to the VMs created from the template", + "type": "integer", + "minimum": 1, + "error": "KCHTMPL0012E" + },

It is better to concentrate all the CPU information in the same object

That way the 'cpus' would be changed to 'cpu' with 'current' and 'max' values

{.. cpu: {current: ..., max:...}}

It would be good to also include 'cpu_info[topology]' in the same data structure.

{... cpu: {current:..., max:..., topology: ...}

To include topology in the same data structure will require a lot of changes. So it is better to do in a separated patch after getting this one (related to max cpus) merged.

Yes, I talked to Rodrigo about this. I preferred not to do it along with the new feature due to it being a big change.

There are some implications on this. Each of cpus, maxvcpus and topology values depend on each other: maxvcpus = sockets * cores * threads cpus = x * cores * threads (where 1 <= x <= sockets)

Which means that, if the client (UI or API) wants to do cpu hotplug, it will have to inform the entire struct again (not only cpus new value). My patches try to be flexible and accept cpus-only or maxvcpus-only updates. All values are required only if topology is being changed.

You can continue doing that way even changing the data structure.

On 01/27/2016 07:14 PM, Lucio Correia wrote:

- Enable update of maxvcpus, vcpus and CPU topology - Enable vCPU cold plug when topology is defined - Add cpu_info validation to guest update - Simplify _get_cpu_xml() code - Remove change_numa verification - Update docs and APIs with maxvcpus attribute

Signed-off-by: Lucio Correia <luciojhc@linux.vnet.ibm.com> --- API.json | 7 ++++ docs/API.md | 17 +++++++- i18n.py | 1 - model/vms.py | 128 +++++++++++++++++++++++++++++++++++++--------------------- vmtemplate.py | 30 ++++---------- 5 files changed, 110 insertions(+), 73 deletions(-)

diff --git a/API.json b/API.json index 4876cc0..a3d99f8 100644 --- a/API.json +++ b/API.json @@ -299,12 +299,19 @@ } } }, + "cpu_info": { "$ref": "#/kimchitype/cpu_info" }, "cpus": { "description": "The new number of virtual CPUs for the VM", "type": "integer", "minimum": 1, "error": "KCHTMPL0012E" }, + "maxvcpus": { + "description": "The maximum number of virtual CPUs that can be assigned to the VM", + "type": "integer", + "minimum": 1, + "error": "KCHTMPL0012E" + }, "memory": { "description": "The new amount (MB) of memory for the VM", "type": "integer", diff --git a/docs/API.md b/docs/API.md index 4790d08..8dbc27c 100644 --- a/docs/API.md +++ b/docs/API.md @@ -118,6 +118,7 @@ server. * uuid: UUID of the VM. * memory: The amount of memory assigned to the VM (in MB) * cpus: The number of CPUs assigned to the VM + * maxvcpus: The maximum number of CPUs that can be assigned to the VM

Same as I asked on the first patch. In API.json you said that maxvcpu is "The maximum number of virtual CPUs that can be assigned to the VMs..." and here looks like you missed the *virtual* word. Is it correct?

* screenshot: A link to a recent capture of the screen in PNG format * icon: A link to an icon that represents the VM * graphics: A dict to show detail of VM graphics. @@ -138,12 +139,13 @@ server. * groups: A list of system groups whose users have permission to access the VM. Default is: empty (i.e. no groups given access). * **DELETE**: Remove the Virtual Machine -* **PUT**: update the parameters of existed VM +* **PUT**: update the parameters of existing VM * name: New name for this VM (only applied for shutoff VM) * users: New list of system users. * groups: New list of system groups. - * cpus: New number of virtual cpus for this VM (if VM is running, new value + * cpus: New number of CPUs for this VM (if VM is running, new value will take effect in next reboot) + * maxvcpus: The maximum number of CPUs that can be assigned to the VM

Same here.

* memory: New amount of memory (MB) for this VM (if VM is running, new value will take effect in next reboot) * graphics: A dict to show detail of VM graphics. @@ -152,6 +154,17 @@ server. * passwdValidTo *(optional)*: lifetime for the console password. When omitted the password will be valid just for 30 seconds. + * cpu_info: 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 maximum number of sockets to use. + * cores - The number of cores per socket. + * threads - The number of threads per core. + If CPU topology is specified, *cpus* and *maxvcpus* also must be + specified. Make sure *maxvcpus* is equal to the product of sockets, + cores and threads, and *cpus* is a multiple of a product of cores + and threads.

* **POST**: *See Virtual Machine Actions*

diff --git a/i18n.py b/i18n.py index bb6f3d1..59870e8 100644 --- a/i18n.py +++ b/i18n.py @@ -130,7 +130,6 @@ messages = { "KCHVM0071E": _("Memory value %(mem)s must be aligned to %(alignment)sMiB."), "KCHVM0073E": _("Unable to update the following parameters while the VM is offline: %(params)s"), "KCHVM0074E": _("Unable to update the following parameters while the VM is online: %(params)s"), - "KCHVM0075E": _("Cannot change VCPU value because '%(vm)s' has a topology defined - sockets: %(sockets)s, cores: %(cores)s, threads: %(threads)s."),

"KCHVMHDEV0001E": _("VM %(vmid)s does not contain directly assigned host device %(dev_name)s."), "KCHVMHDEV0002E": _("The host device %(dev_name)s is not allowed to directly assign to VM."), diff --git a/model/vms.py b/model/vms.py index 9aaf530..fa3bcbb 100644 --- a/model/vms.py +++ b/model/vms.py @@ -41,7 +41,7 @@ from wok.model.tasks import TaskModel from wok.rollbackcontext import RollbackContext from wok.utils import add_task, convert_data_size, get_next_clone_name from wok.utils import import_class, run_setfacl_set_attr, run_command, wok_log -from wok.xmlutils.utils import xpath_get_text, xml_item_update +from wok.xmlutils.utils import xpath_get_text, xml_item_remove, xml_item_update from wok.xmlutils.utils import dictize

from wok.plugins.kimchi import model @@ -72,15 +72,12 @@ DOM_STATE_MAP = {0: 'nostate', 6: 'crashed', 7: 'pmsuspended'}

-VM_STATIC_UPDATE_PARAMS = {'name': './name', 'cpus': './vcpu'} - -VM_LIVE_UPDATE_PARAMS = {} - # update parameters which are updatable when the VM is online VM_ONLINE_UPDATE_PARAMS = ['graphics', 'groups', 'memory', 'users'] + # update parameters which are updatable when the VM is offline -VM_OFFLINE_UPDATE_PARAMS = ['cpus', 'graphics', 'groups', 'memory', 'name', - 'users'] +VM_OFFLINE_UPDATE_PARAMS = ['cpus', 'cpu_info', 'graphics', 'groups', + 'maxvcpus', 'memory', 'name', 'users']

XPATH_DOMAIN_DISK = "/domain/devices/disk[@device='disk']/source/@file" XPATH_DOMAIN_DISK_BY_FILE = "./devices/disk[@device='disk']/source[@file='%s']" @@ -93,8 +90,10 @@ XPATH_DOMAIN_MEMORY_UNIT = '/domain/memory/@unit' XPATH_DOMAIN_UUID = '/domain/uuid' XPATH_DOMAIN_DEV_CPU_ID = '/domain/devices/spapr-cpu-socket/@id'

+XPATH_NAME = './name' XPATH_NUMA_CELL = './cpu/numa/cell' XPATH_TOPOLOGY = './cpu/topology' +XPATH_VCPU = './vcpu'

# key: VM name; value: lock object vm_locks = {} @@ -285,17 +284,6 @@ class VMModel(object): raise InvalidParameter('KCHVM0074E', {'params': ', '.join(ext_params)})

- if 'cpus' in params and DOM_STATE_MAP[dom.info()[0]] == 'shutoff': - # user cannot change vcpu if topology is defined. - curr_vcpu = dom.vcpusFlags(libvirt.VIR_DOMAIN_AFFECT_CURRENT) - if self.has_topology(dom) and curr_vcpu != params['cpus']: - raise InvalidOperation( - 'KCHVM0075E', - {'vm': dom.name(), - 'sockets': self.get_vm_sockets(dom), - 'cores': self.get_vm_cores(dom), - 'threads': self.get_vm_threads(dom)}) - self._live_vm_update(dom, params) vm_name, dom = self._static_vm_update(name, dom, params) return vm_name @@ -751,6 +739,8 @@ class VMModel(object): old_xml = new_xml = dom.XMLDesc(0)

params = copy.deepcopy(params) + + # Update name name = params.get('name') nonascii_name = None if name is not None: @@ -760,37 +750,42 @@ class VMModel(object): raise InvalidParameter("KCHVM0003E", msg_args)

params['name'], nonascii_name = get_ascii_nonascii_name(name) + new_xml = xml_item_update(new_xml, XPATH_NAME, nonascii_name, None) + + # Update vcpus + cpus = params.get('cpus', None) + if cpus: + attrib = 'current' + new_xml = xml_item_update(new_xml, XPATH_VCPU, str(cpus), attrib) + + # Update maxvcpus + maxvcpus = params.get('maxvcpus', None) + if maxvcpus: + new_xml = xml_item_update(new_xml, XPATH_VCPU, str(maxvcpus), None) + + # Update topology + if 'cpu_info' in params: + topology = params.get('cpu_info', {}).get('topology', None) + + if topology: + sockets = str(topology['sockets']) + cores = str(topology['cores']) + threads = str(topology['threads']) + + xpath = XPATH_TOPOLOGY + new_xml = xml_item_update(new_xml, xpath, sockets, 'sockets') + new_xml = xml_item_update(new_xml, xpath, cores, 'cores') + new_xml = xml_item_update(new_xml, xpath, threads, 'threads') + else: + # topology may have been removed + new_xml = xml_item_remove(new_xml, xpath)

- for key, val in params.items(): - change_numa = True - if key in VM_STATIC_UPDATE_PARAMS: - if type(val) == int: - val = str(val) - xpath = VM_STATIC_UPDATE_PARAMS[key] - attrib = None - if key == 'cpus': - if self.has_topology(dom) or dom.isActive(): - change_numa = False - continue - # Update maxvcpu firstly - new_xml = xml_item_update(new_xml, xpath, - str(self._get_host_maxcpu()), - attrib) - # Update current vcpu - attrib = 'current' - new_xml = xml_item_update(new_xml, xpath, val, attrib) + # Revalidate cpu info - may raise CPUInfo exceptions + self._validate_cpu_info(new_xml, dom)

# Updating memory and NUMA if necessary, if vm is offline - if not dom.isActive(): - if 'memory' in params: - new_xml = self._update_memory_config(new_xml, params) - elif 'cpus' in params and change_numa and \ - (xpath_get_text(new_xml, XPATH_NUMA_CELL + '/@memory') != []): - new_xml = xml_item_update( - new_xml, - XPATH_NUMA_CELL, - value='0', - attr='cpus') + if 'memory' in params and not dom.isActive(): + new_xml = self._update_memory_config(new_xml, params)

if 'graphics' in params: new_xml = self._update_graphics(dom, new_xml, params) @@ -928,6 +923,27 @@ class VMModel(object):

return ET.tostring(root, encoding="utf-8")

+ def _validate_cpu_info(self, new_xml, dom): + topology = None + if self.has_topology(dom): + sockets = xpath_get_text(new_xml, XPATH_TOPOLOGY + '/@sockets')[0] + cores = xpath_get_text(new_xml, XPATH_TOPOLOGY + '/@cores')[0] + threads = xpath_get_text(new_xml, XPATH_TOPOLOGY + '/@threads')[0] + topology = { + 'sockets': int(sockets), + 'cores': int(cores), + 'threads': int(threads), + } + + params = { + 'maxvcpus': int(xpath_get_text(new_xml, 'vcpu')[0]), + 'cpus': int(xpath_get_text(new_xml, './vcpu/@current')[0]), + 'cpu_info': {'topology': topology} + } + + cpu_model = CPUInfoModel(conn=self.conn) + cpu_model.check_cpu_info(params) + def _live_vm_update(self, dom, params): self._vm_update_access_metadata(dom, params) if 'memory' in params and dom.isActive(): @@ -1140,8 +1156,24 @@ class VMModel(object): res['io_throughput_peak'] = vm_stats.get('max_disk_io', 100) users, groups = self._get_access_info(dom)

+ xml = dom.XMLDesc(0) + maxvcpus = int(xpath_get_text(xml, XPATH_VCPU)[0]) + + cpu_info = None + if self.has_topology(dom): + sockets = int(xpath_get_text(xml, XPATH_TOPOLOGY + '/@sockets')[0]) + cores = int(xpath_get_text(xml, XPATH_TOPOLOGY + '/@cores')[0]) + threads = int(xpath_get_text(xml, XPATH_TOPOLOGY + '/@threads')[0]) + + cpu_info = { + 'topology': { + 'sockets': sockets, + 'cores': cores, + 'threads': threads, + } + } + if state == 'shutoff': - xml = dom.XMLDesc(0) val = xpath_get_text(xml, XPATH_DOMAIN_MEMORY)[0] unit_list = xpath_get_text(xml, XPATH_DOMAIN_MEMORY_UNIT) if len(unit_list) > 0: @@ -1158,6 +1190,8 @@ class VMModel(object): 'uuid': dom.UUIDString(), 'memory': memory, 'cpus': info[3], + 'cpu_info': cpu_info, + 'maxvcpus': maxvcpus, 'screenshot': screenshot, 'icon': icon, # (type, listen, port, passwd, passwdValidTo) diff --git a/vmtemplate.py b/vmtemplate.py index 2f524a7..e1571b0 100644 --- a/vmtemplate.py +++ b/vmtemplate.py @@ -324,12 +324,9 @@ class VMTemplate(object):

def _get_cpu_xml(self): # Include CPU topology, if provided - cpu_info = self.info.get('cpu_info') - if cpu_info is not None: - cpu_topo = cpu_info.get('topology') - return get_cpu_xml(0, - self.info.get('memory') << 10, - cpu_topo) + cpu_topo = self.info.get('cpu_info', {}).get('topology', None) + + return get_cpu_xml(0, self.info.get('memory') << 10, cpu_topo)

def _get_max_memory(self, guest_memory): # Setting maxMemory of the VM, which will be lesser value between: @@ -371,8 +368,6 @@ class VMTemplate(object): libvirt_stream_protocols = kwargs.get('libvirt_stream_protocols', []) cdrom_xml = self._get_cdrom_xml(libvirt_stream_protocols)

- max_vcpus = kwargs.get('max_vcpus', 1) - if not urlparse.urlparse(self.info.get('cdrom', "")).scheme in \ libvirt_stream_protocols and \ params.get('iso_stream', False): @@ -402,22 +397,11 @@ class VMTemplate(object): # set a hard limit using max_memory + 1GiB params['hard_limit'] = params['max_memory'] + (1024 << 10)

- cpu_topo = self.info.get('cpu_info').get('topology') - if (cpu_topo is not None): - sockets = int(max_vcpus / (cpu_topo['cores'] * - cpu_topo['threads'])) - self.info['cpu_info']['topology']['sockets'] = sockets + # vcpu element + params['vcpus'] = "<vcpu current='%d'>%d</vcpu>" % \ + (params['cpus'], params['maxvcpus'])

- # Reduce maxvcpu to fit number of sockets if necessary - total_max_vcpu = sockets * cpu_topo['cores'] * cpu_topo['threads'] - if total_max_vcpu != max_vcpus: - max_vcpus = total_max_vcpu - - params['vcpus'] = "<vcpu current='%s'>%d</vcpu>" % \ - (params['cpus'], max_vcpus) - else: - params['vcpus'] = "<vcpu current='%s'>%d</vcpu>" % \ - (params['cpus'], max_vcpus) + # cpu_info element params['cpu_info'] = self._get_cpu_xml()

xml = """