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

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

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