Re: [Kimchi-devel] [PATCH v1] Parts to allow Kimchi to configure the cpu topology.

On 11/03/2014 08:16 PM, Christy Perez wrote: > > 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. On edit Template I plan to add a new combo box for SMT configuration with the options: off, smt2, smt4, smt8

> >> See more comments inline below: >> >> >> >> On 11/03/2014 12:36 AM, Christy Perez wrote: >>> Signed-off-by: Christy Perez <christy(a)linux.vnet.ibm.com&gt; >>> --- >>> 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.). Is it really needed? It seems a very complex information to display a Kimchi user. As I said before I was planning to allow the following options to user: off, smt2, smt4, smt8

> >> >>> 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? Yeap. Sorry, I inserted the comment in the wrong line My point is, comparing output string is an easy point of failure. Couldn't we rely only with rc code?

>> 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() >>>