Re: [Kimchi-devel] [PATCH] [Kimchi 1/3] Add maxvcpus attribute to templates

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(a)linux.vnet.ibm.com&gt; >> --- >> 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:...}

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(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/kimchi-devel > _______________________________________________ Kimchi-devel mailing list Kimchi-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel