Change in vdsm[master]: host stats: Collect stats from online cpu cores only

nsoffer at redhat.com nsoffer at redhat.com
Sun Dec 6 14:55:12 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: host stats: Collect stats from online cpu cores only
......................................................................


Patch Set 13: Code-Review-1

(7 comments)

https://gerrit.ovirt.org/#/c/46269/13/tests/hoststatsTests.py
File tests/hoststatsTests.py:

Line 129:         }
Line 130: 
Line 131:     def testCpuCoreStats(self):
Line 132:         cpu_sample = {'user': 1.0, 'sys': 2.0}
Line 133: 
For completeness, comment about both cpus being online. All this is very clear to you now, but in six month from now, it will be much easier to understand this with such comments.
Line 134:         first_sample = fake.HostSample(1.0, {0: cpu_sample, 1: cpu_sample})
Line 135:         last_sample = fake.HostSample(2.0, {0: cpu_sample, 1: cpu_sample})
Line 136: 
Line 137:         with MonkeyPatchScope([(caps, 'getNumaTopology',


Line 143: 
Line 144:     def testSkipStatsOnMissingFirstSample(self):
Line 145:         cpu_sample = {'user': 1.0, 'sys': 2.0}
Line 146: 
Line 147:         first_sample = fake.HostSample(1.0, {0: cpu_sample})
Sample missing here because cpu 1 was offline, right? Can you add a comment explaining this when you create the samples?

For example:

    # cpu 1 is offline when first sample was taken
    first_sample = fake.HostSample(1.0, {0: cpu_sample})
    last_sample = fake.HostSample(2.0, {0: cpu_sample, 1: cpu_sample})
Line 148:         last_sample = fake.HostSample(2.0, {0: cpu_sample, 1: cpu_sample})
Line 149: 
Line 150:         with MonkeyPatchScope([(caps, 'getNumaTopology',
Line 151:                                 self._fakeNumaTopology)]):


Line 156:     def testSkipStatsOnMissingLastSample(self):
Line 157:         cpu_sample = {'user': 1.0, 'sys': 2.0}
Line 158: 
Line 159:         first_sample = fake.HostSample(1.0, {0: cpu_sample, 1: cpu_sample})
Line 160:         last_sample = fake.HostSample(2.0, {0: cpu_sample})
Same - comment why sample is missing
Line 161: 
Line 162:         with MonkeyPatchScope([(caps, 'getNumaTopology',
Line 163:                                 self._fakeNumaTopology)]):
Line 164:             result = hoststats._get_cpu_core_stats(first_sample, last_sample)


https://gerrit.ovirt.org/#/c/46269/13/tests/vmfakelib.py
File tests/vmfakelib.py:

Line 382:     def __init__(self, samples):
Line 383:         self._samples = samples
Line 384: 
Line 385:     def getCoreSample(self, key):
Line 386:         return self._samples.get(key)
Returning None instead of raising KeyError is bad change. If you don't want to handle KeyError since it may hide a bug, raise specific error like MissingSample.
Line 387: 
Line 388: 
Line 389: class HostSample(object):
Line 390: 


https://gerrit.ovirt.org/#/c/46269/13/vdsm/virt/hoststats.py
File vdsm/virt/hoststats.py:

Line 100:     def compute_cpu_usage(cpu_core, mode):
Line 101:         first_core_sample = first_sample.cpuCores.getCoreSample(cpu_core)
Line 102:         last_core_sample = last_sample.cpuCores.getCoreSample(cpu_core)
Line 103:         if not first_core_sample or not last_core_sample:
Line 104:             return None
Again you are returning None instead of failing, now the caller has to check for None.

Better to let the error bubble up to the caller, no error handling need here.
Line 105:         jiffies = (
Line 106:             last_core_sample[mode] - first_core_sample[mode]
Line 107:         ) % JIFFIES_BOUND
Line 108:         return ("%.2f" % (jiffies / interval))


Line 114:             user_cpu_usage = compute_cpu_usage(cpu_core, 'user')
Line 115:             system_cpu_usage = compute_cpu_usage(cpu_core, 'sys')
Line 116:             # Do not try to collect cpu core data when no samples are present
Line 117:             if not user_cpu_usage or not system_cpu_usage:
Line 118:                 continue
If you raised an error in the underlying calls, we could do instead:

    try:
        user_cpu_usage = ...
        system_cpu_usage = ...
    except MissingSample:
        continue

Isn't this much nicer?
Line 119:             core_stat = {
Line 120:                 'nodeIndex': int(node_index),
Line 121:                 'cpuUser': user_cpu_usage,
Line 122:                 'cpuSys': system_cpu_usage,


https://gerrit.ovirt.org/#/c/46269/13/vdsm/virt/sampling.py
File vdsm/virt/sampling.py:

Line 156:                     self.coresSample[match.group(1)] = coreSample
Line 157: 
Line 158:     def getCoreSample(self, coreId):
Line 159:         strCoreId = str(coreId)
Line 160:         return self.coresSample.get(strCoreId)
Returning None makes it harder for the callers.
Line 161: 
Line 162: 
Line 163: class NumaNodeMemorySample(object):
Line 164:     """


-- 
To view, visit https://gerrit.ovirt.org/46269
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia9c247f9138e02a9230a0849a04cb2e1705e7fac
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Roman Mohr <rmohr at redhat.com>
Gerrit-Reviewer: Arik Hadas <ahadas at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Martin Sivák <msivak at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Omer Frenkel <d.mosquito at gmail.com>
Gerrit-Reviewer: Roman Mohr <rmohr at redhat.com>
Gerrit-Reviewer: Roy Golan <rgolan at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list