Francesco Romani has posted comments on this change.
Change subject: sampling: Collect numa related statistics
......................................................................
Patch Set 5: Code-Review-1
(9 comments)
A few comments, -1 only for visibility.
http://gerrit.ovirt.org/#/c/26876/5/vdsm/virt/sampling.py
File vdsm/virt/sampling.py:
Line 127: A sample of the CPU consumption of each core
Line 128:
Line 129: The sample is taken at initialization time and can't be updated.
Line 130: """
Line 131: CPU_CORE_STATS_PATTERN = re.compile(r'cpu(\d+)\s+(.*)')
Any chance to get rid of this? Just asking, probably not but it is worth to try... :\
Line 132:
Line 133: def __init__(self):
Line 134: self._coresSample = {}
Line 135: with open('/proc/stat') as src:
Line 142: coreSample['user'] = user
Line 143: coreSample['userNice'] = userNice
Line 144: coreSample['sys'] = sys
Line 145: coreSample['idle'] = idle
Line 146: self._coresSample[match.group(1)] = coreSample
minor note: I'd prefer the literal syntax here:
user, userNice, sys, idle = \
map(int, match.group(2).split()[0:4])
self._coresSample[match.group(1)] = {
'user': user,
'userNice': userNice,
'sys': sys,
'idle': idle
}
also, it is preferred to leverage the open-bracket than the continuation mark:
user, userNice, sys, idle = map(
int, match.group(2).split()[0:4])
(and yes, in retrospect getNumaTopology should have been changed as well... but it is not
a big deal however).
Line 147:
Line 148: def getCoreSample(self, coreId):
Line 149: strCoreId = str(coreId)
Line 150: if strCoreId in self._coresSample:
Line 158: The sample is taken at initialization time and can't be updated.
Line 159: """
Line 160: def __init__(self):
Line 161: self._nodesMemSample = {}
Line 162: numaTopology = caps._getNumaTopology()
See the comment below, this is probably a candidate to be made public.
Line 163: for nodeIndex in numaTopology:
Line 164: nodeMemSample = {}
Line 165: if len(numaTopology) < 2:
Line 166: memInfo = caps._getUMAHostMemoryStats()
Line 164: nodeMemSample = {}
Line 165: if len(numaTopology) < 2:
Line 166: memInfo = caps._getUMAHostMemoryStats()
Line 167: else:
Line 168: memInfo = caps._getMemoryStatsByNumaCell(int(nodeIndex))
All those caps functions (caps._get*) have a leading underscore, yet we are using them
from a different module. I'd like to either have them "public" or, better,
to add a public helper to provide the right 'meminfo' result given the memory
topology (UMA vs NUMA).
Line 169: nodeMemSample['memFree'] = memInfo['free']
Line 170: nodeMemSample['memPercent'] = 100 - \
Line 171: int(100.0 * int(memInfo['free']) /
int(memInfo['total']))
Line 172: self._nodesMemSample[nodeIndex] = nodeMemSample
Line 168: memInfo = caps._getMemoryStatsByNumaCell(int(nodeIndex))
Line 169: nodeMemSample['memFree'] = memInfo['free']
Line 170: nodeMemSample['memPercent'] = 100 - \
Line 171: int(100.0 * int(memInfo['free']) /
int(memInfo['total']))
Line 172: self._nodesMemSample[nodeIndex] = nodeMemSample
minor nit: same as above:
if len(numaTopology) < 2:
memInfo = caps._getUMAHostMemoryStats()
else:
memInfo = caps._getMemoryStatsByNumaCell(int(nodeIndex))
self._nodesMemSample[nodeIndex] = {
'memFree': memInfo['free'],
'memPercent': 100 - int(
100.0 * int(memInfo['free']) / int(memInfo['total']))
}
Line 173:
Line 174:
Line 175: class PidCpuSample:
Line 176: """
Line 538:
Line 539: if self._boot_time():
Line 540: stats['bootTime'] = self._boot_time()
Line 541:
Line 542: stats['numaNodeMemFree'] = hs1.numaNodeMem._nodesMemSample
same here for this 'private' field; most often accessing a leading underscore
field is a code smell, if possible I'd like to respect the proper interface.
Line 543: stats['cpuStatistics'] = self._getCpuCoresStats()
Line 544: return stats
Line 545:
Line 546: def _getCpuCoresStats(self):
Line 552: """
Line 553: cpuCoreStats = {}
Line 554: numaTopology = caps._getNumaTopology()
Line 555: for nodeIndex in numaTopology:
Line 556: cpuCores = numaTopology[nodeIndex]['cpus']
_getNumaTopology() returns a dict, and apparently we are just interested on its values
here. Then 'numaTopology' may be elided, while we need 'nodeIndex' below.
So please consider the following:
for nodeIndex, numaNode in caps._getNumaTopology().iteritems():
cpuCores = numaNode['cpus']
Line 557: for cpuCore in cpuCores:
Line 558: coreStat = {}
Line 559: coreStat['nodeIndex'] = int(nodeIndex)
Line 560: hs0, hs1 = self._samples[0], self._samples[-1]
Line 564: (2 ** 32)
Line 565: coreStat['cpuUser'] = ("%.2f" % (jiffies /
interval))
Line 566: jiffies = (hs1.cpuCores.getCoreSample(cpuCore)['sys'] -
Line 567: hs0.cpuCores.getCoreSample(cpuCore)['sys']) %
\
Line 568: (2 ** 32)
To see the same code twice is usually the threshold to extract an helper. Can it be done
here?
Line 569: coreStat['cpuSys'] = ("%.2f" % (jiffies /
interval))
Line 570: coreStat['cpuIdle'] = ("%.2f" %
Line 571: max(0.0, 100.0 -
Line 572: float(coreStat['cpuUser']) -
Line 570: coreStat['cpuIdle'] = ("%.2f" %
Line 571: max(0.0, 100.0 -
Line 572: float(coreStat['cpuUser']) -
Line 573: float(coreStat['cpuSys'])))
Line 574: cpuCoreStats[str(cpuCore)] = coreStat
same not as above as the usage of the dict, please see above
Line 575: return cpuCoreStats
Line 576:
Line 577: def _getInterfacesStats(self):
Line 578: """
--
To view, visit
http://gerrit.ovirt.org/26876
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8d2edd74b8b5d5d3d3e508afc3f675037a86597
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Xiaolei Shi <xiao-lei.shi(a)hp.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Gilad Chaplik <gchaplik(a)redhat.com>
Gerrit-Reviewer: Martin Sivák <msivak(a)redhat.com>
Gerrit-Reviewer: Xiaolei Shi <xiao-lei.shi(a)hp.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes