[Kimchi-devel] [PATCH v1] Parts to allow Kimchi to configure the cpu topology.
Christy Perez
christy at linux.vnet.ibm.com
Wed Nov 5 14:54:30 UTC 2014
On 11/05/2014 05:44 AM, Aline Manera wrote:
>
> 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
Well, that's great, but, how will the remove happen since the backend
doesn't support that right now? ;)
Also just a reminder that it's only called SMT on Power...
>
>>
>>> See more comments inline below:
>>>
>>>
>>>
>>> On 11/03/2014 12:36 AM, Christy Perez wrote:
>>>> Signed-off-by: Christy Perez <christy at 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.).
>
> 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
I think it is needed for the UI at the very least. Ti display those
options (for Power), the UI will need to know how many cores are
available. Again, a system might not be SMT8 capable, so you'd want a
way to find out before putting up the options.
And also, this call will contain the arch, so the UI will know whether
to display SMT or HT. And for HT, we'll have to have different things...
>
>>
>>>
>>>> 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?
I'm assuimg that the string will return whether SMT is enabled or
not,and the rc won't be different between the cases. I'll make sure,
though, because, I agree that checking for strings is never the best way
to go.
>
>>> 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()
>>>>
>
More information about the Kimchi-devel
mailing list