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

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:...}}

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