On 11/03/2014 01:08 PM, Aline Manera wrote:
By default, Kimchi always sets the best Template configuration for user based on a single input: ISO file or Image file. So you don't need to create an API (auto-topology) for it. It will be the default behavior.
The flow is:
- Create a Template which will get the default CPU topology according to host OS and ISO OS.
POST /templates {name: my-tmpl, cdrom: <my-iso>}
- Update the CPU topology according to your needs:
PUT /templates/my-tmpl {cpu_info: {...}}
Ok?
I was assuming we only did that for things that are required to use a VM and kimchi (cpus, memory, a console), etc., but if you think we should set this without being asked, I am okay with that. As long as users know, and can disable it if it doesn't fit their workloads. Currently there's no way to remove the topology from a template once it's added, so that will need to be done ASAP for this flow.
See more comments inline below:
On 11/03/2014 12:36 AM, Christy Perez wrote:
Signed-off-by: Christy Perez <christy@linux.vnet.ibm.com> --- docs/API.md | 2 + src/kimchi/API.json | 6 ++ src/kimchi/control/cpuinfo.py | 37 +++++++++ src/kimchi/i18n.py | 2 + src/kimchi/model/cpuinfo.py | 176 ++++++++++++++++++++++++++++++++++++++++++ src/kimchi/model/host.py | 1 + src/kimchi/model/templates.py | 13 +++- 7 files changed, 236 insertions(+), 1 deletion(-) create mode 100644 src/kimchi/control/cpuinfo.py create mode 100644 src/kimchi/model/cpuinfo.py
-----
diff --git a/docs/API.md b/docs/API.md index 9c06f85..4e449b3 100644 --- a/docs/API.md +++ b/docs/API.md @@ -226,6 +226,8 @@ Represents a snapshot of the Virtual Machine's primary monitor. * threads - The number of threads per core. If specifying both cpus and CPU topology, make sure cpus is equal to the product of sockets, cores, and threads. + * auto-topology: Allow Kimchi to determine the best topology + based on desired vCPUs and host capabilities.
### Sub-Collection: Virtual Machine Network Interfaces
diff --git a/src/kimchi/API.json b/src/kimchi/API.json index 0ad36ab..b405889 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -54,6 +54,12 @@ "error": "KCHTMPL0026E" } } + }, + "auto_topology": { + "description": "Auto-configure guest CPU topology.", + "type": "boolean", + "required": false, + "error": "KCHTMPL0028E" } } } diff --git a/src/kimchi/control/cpuinfo.py b/src/kimchi/control/cpuinfo.py new file mode 100644 index 0000000..3a295ad --- /dev/null +++ b/src/kimchi/control/cpuinfo.py @@ -0,0 +1,37 @@ +# +# Project Kimchi +# +# Copyright IBM, Corp. 2014 +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + +from kimchi.control.base import Resource + + +class CPUInfo(Resource): + def __init__(self, model): + super(CPUInfo, self).__init__(model) + self.admin_methods = ['GET'] + self.uri_fmt = "/host/cpu_info" + + @property + def data(self): + return {'arch': self.info['arch'], + 'threading_enabled': self.info['guest_threads_enabled'], + 'sockets': self.info['sockets'], + 'cores_present': self.info['cores_present'], + 'cores_online': self.info['cores_available'], + 'threads_per_core': self.info['threads_per_core'] + }
All that will not needed.
I think it's nice to let users know how many cores, etc., are on their system so they can plan accordingly. For example, if a Power system has been changed to split-core mode, a user might want to know that they only have 4 threads per core now instead of 8. I also wanted to have this so that the UI would have something to call so they'd know what to display (HT, SMTX, etc.).
diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index f789259..7669dbc 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -146,6 +146,8 @@ "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": _("The auto_topology value must be either True or False."), + "KCHTMPL0029E": _("This host (or current configuration) does not allow CPU topology."),
"KCHPOOL0001E": _("Storage pool %(name)s already exists"), "KCHPOOL0002E": _("Storage pool %(name)s does not exist"), diff --git a/src/kimchi/model/cpuinfo.py b/src/kimchi/model/cpuinfo.py new file mode 100644 index 0000000..12f5899 --- /dev/null +++ b/src/kimchi/model/cpuinfo.py
As this file will not needed, you can move the functions needed to identify the topology to a new module model/cputopology.py or something like it.
@@ -0,0 +1,176 @@ +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + +import platform +import libvirt +import psutil +from xml.etree import ElementTree as ET + +from kimchi.exception import InvalidParameter +from kimchi.utils import kimchi_log, run_command + + +class CPUInfoModel(object): + """ + Get information about a CPU for hyperthreading (on x86) + or SMT (on POWER) for logic when creating templates and VMs. + """ + + def __init__(self, **kargs): + """ Since there are so many similar-seeming variables: + - arch = either ppc or x86 (Intel and AMD) + - guest_threads_enabled = Can a user specify topology? + - sockets = total number of sockets in the system. While nothing + uses this value, it's part of topology so we'll track it. + - cores_present = total number of cores in the system + Note: Cores is often synonymous with CPUs. + - cores_available = cores online (in case some were offlined) + - cores_per_socket = max cores value for topology + - threads_per_core = max threads value for topology + """ + libvirt_topology = None
+ try: + connect = libvirt.open(None)
Use LibvirtConnection to establish a connection to libvirt. One of the args on "kargs" is "conn" which is already an LibvirtConnection instance.
ACK
+ except Exception as e: + raise Exception("Unable to get qemu connection: %s" % e.message) + try: + xml = connect.getCapabilities() + capabilities = ET.fromstring(xml) + libvirt_topology = capabilities.find('host').find('cpu').\ + find('topology') + if libvirt_topology is None: + kimchi_log.info("cpu_info topology not supported.") + self.guest_threads_enabled = False + self.sockets = 0 + self.cores_present = 0 + self.cores_available = 0 + self.cores_per_socket = 0 + self.threads_per_core = 0 + self.max_threads = 0 + return + except Exception as e: + raise("Unable to get CPU topology capabilities: %s" % e.message) + + if platform.machine().startswith('ppc'): + # IBM PowerPC + self.arch = 'ppc' + self.guest_threads_enabled = True + out, error, rc = run_command('ppc64_cpu', '--smt')
Only rc!=0 is not enough to check it?
I'm not sure what you mean here. Are you referring to the check for the 'out' message below?
And isn't there a libvirt API to ppc64_cpu?
It doesn't work correctly right now. This is a well-known issue and ppc64_cpu is a well-known workaround until its fixed.
+ if rc or out is not 'SMT is off': + # SMT has to be disabled for guest to use threads as CPUs. + self.guest_threads_enabled = False + out, error, rc = run_command('ppc64_cpu', '--cores_present') + if not rc: + self.cores_present = out.split()[-1] + out, error, rc = run_command('ppc64_cpu', '--cores_on') + if not rc: + self.cores_available = out.split()[-1] + out, error, rc = run_command('ppc64_cpu', '--threads_per_core') + if not rc: + self.threads_per_core = out.split()[-1] + # This next line doesn't make sense as written, but takes advantage + # of the fact that libvirt for ppc reports all the cores on the + # system as the cores value in topology (e.g. sockets: 1, cores: + # 128, threads: 1) + # @TODO: When libvirt on Power starts returning the correct info, + # we should be able to remove the seperate logic (or at least put + # in a check for the libvirt version, b/c that will break this). + self.cores_per_socket = \ + libvirt.topology.get('cores')/self.threads_per_core + # Nothing actually uses 'sockets,' but set it anyway + self.sockets = 1 + else: + # Intel or AMD + self.arch = 'x86' + self.guest_threads_enabled = True + self.sockets = int(libvirt_topology.get('sockets')) + self.cores_per_socket = int(libvirt_topology.get('cores')) + self.cores_present = self.cores_per_socket * self.sockets + self.cores_available = psutil.NUM_CPUS + self.threads_per_core = int(libvirt_topology.get('threads')) + + def lookup(self): + """ + The UI can use this data to decide what to display: + - SMT (arch = ppc) or HT (arch = x86) + - If HT: + 1. the UI can look at cores_present and + warn the user if he/she creates a VM with more + vCPUs than cores (bad performance). + 2. The UI could include a suggestion of 'cores_present' + or fewer vCPUs next to the CPU box. + 3. There could be an actual limit on the number of vCPUs. + A user can use this data to decide how to best manually + create a VM or template. + """ + return { + 'arch': self.arch, + 'guest_threads_enabled': self.guest_threads_enabled, + 'sockets': self.sockets, + 'cores_per_socket': self.cores_per_socket, + 'cores_present': self.cores_present, + 'cores_availalble': self.cores_available, + 'threads_per_core': self.threads_per_core, + } + + def get_rec_topology(self, vcpus, guest=None, smtx=0): + """ + Kimchi will provide a recommended topoology based on the + number of virtual CPUs desired and guest OS. + + param vcpus: should be an integer + param guest: should be in the form 'distro:version' + param thread_pref: a power of 2, or 0, if no preference (x86). + return: topology numbers. If all zero, SMT/hyperthreading not + supported. + """ + + """ + Note to remove later ... This will be called from the UI, + If Power, smtx should be set. + + On x86, spreading threads over cores is better than grouping. + IOW, for vcpus = 2, sockets = 1, cores = 2, threads = 1 is + better than vcpus=2, sockets = 1, cores = 1, threads = 2. + """ + + if not self.guest_threads_enabled: + return None + if vcpus > self.cores_available * self.threads_per_core: + # This check should go into template create too? + # Or should we allow this? + raise InvalidParameter("The number of vcpus is too large \ + for this system") # @TODO: msgid +
+ sockets = 1 + cores = 1 + threads = 1 + + if vcpus == 1: + return {"sockets": sockets, "cores": cores, "threads": threads} +
+ # creating a VM using REST APIs + if self.arch is 'ppc' and smtx == 0: + smtx = self.threads_per_core + + if smtx: # Power + cores = vcpus/smtx + threads = smtx + + else: # x86 + # The scheduler will move these around, but we'll split + # it up anyway. + cores = self.cores_available + threads = vcpus/cores + + return {"sockets": sockets, "cores": cores, "threads": threads} diff --git a/src/kimchi/model/host.py b/src/kimchi/model/host.py index 8cddcdc..7912311 100644 --- a/src/kimchi/model/host.py +++ b/src/kimchi/model/host.py @@ -53,6 +53,7 @@ def __init__(self, **kargs): self.task = TaskModel(**kargs) self.host_info = self._get_host_info()
+ # @TODO: Think about moving this to the new cpuinfo class def _get_ppc_cpu_info(self): res = {} with open('/proc/cpuinfo') as f: diff --git a/src/kimchi/model/templates.py b/src/kimchi/model/templates.py index ff1070d..e402260 100644 --- a/src/kimchi/model/templates.py +++ b/src/kimchi/model/templates.py @@ -25,6 +25,7 @@ from kimchi.exception import InvalidOperation, InvalidParameter from kimchi.exception import NotFoundError, OperationFailed from kimchi.kvmusertests import UserTests +from kimchi.model.cpuinfo import CPUInfoModel from kimchi.utils import pool_name_from_uri from kimchi.utils import probe_file_permission_as_user from kimchi.vmtemplate import VMTemplate @@ -50,18 +51,28 @@ def create(self, params):
cpu_info = params.get('cpu_info') if cpu_info: + vcpus = params.get('cpus') topology = cpu_info.get('topology') + auto_topology = cpu_info.get('auto_topology') # Check, even though currently only topology # is supported. if topology: sockets = topology['sockets'] cores = topology['cores'] threads = topology['threads'] - vcpus = params.get('cpus') if vcpus is None: params['cpus'] = sockets * cores * threads elif vcpus != sockets * cores * threads: raise InvalidParameter("KCHTMPL0025E") + elif auto_topology and auto_topology is True:
+ if vcpus is None: + vcpus = 1
Why are you assuming it? Isn't this value same of params['cpus'] ? The default values come from osinfo.py and we should respect them.
Good point. I'll look at the value set as the default instead.
+ topology = CPUInfoModel().get_rec_topology(vcpus) + if topology: + params['cpu_info']['topology'] = topology + else: + raise InvalidOperation("KCHTMPL0029E") + else: params['cpu_info'] = dict()