Change in vdsm[master]: scale: limit cpu usage using cpu-affinity

nsoffer at redhat.com nsoffer at redhat.com
Wed Sep 16 16:10:20 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: scale: limit cpu usage using cpu-affinity
......................................................................


Patch Set 15:

(5 comments)

https://gerrit.ovirt.org/#/c/45738/15/lib/vdsm/taskset.py
File lib/vdsm/taskset.py:

Line 23: from . import constants
Line 24: from . import utils
Line 25: 
Line 26: 
Line 27: class Error(Exception):
> just use RuntimeError please
Using RuntimeError makes error handling harder or impossible. You cannot detect stdlib errors (probably bugs in our code) and real application errors.

We should use our own errors, which are more appropriate for the particular context.

For example, this error allows the handling code to check the return code, output and error of the process, and also provides much better error message, without duplicating the error message in the calling code.
Line 28: 
Line 29:     def __init__(self, rc, out, err):
Line 30:         self.rc = rc
Line 31:         self.out = out


https://gerrit.ovirt.org/#/c/45738/15/tests/tasksetTests.py
File tests/tasksetTests.py:

Line 68:         self.assertEqual(taskset.get(self.proc.pid), cpu_list)
Line 69: 
Line 70:     def test_set_from_child(self):
Line 71: 
Line 72:         cpu_list = ['0']
> please test also a set of more than one cpu, also wrong cpu number and othe
See bellow the 2 tests for bad input, I think they are enough. There is no point in testing other fail cases as all of them fail in the same way (raising taskset.Error after taskset binary failed.)
Line 73: 
Line 74:         def _run_helper():
Line 75:             # avoid race on startup: first do taskset, then notify
Line 76:             taskset.set(os.getpid(), cpu_list)


https://gerrit.ovirt.org/#/c/45738/15/vdsm.spec.in
File vdsm.spec.in:

Line 120: Requires: iproute >= 3.10.0
Line 121: Requires: python-netaddr
Line 122: Requires: python-inotify
Line 123: Requires: python-argparse
Line 124: Requires: python-cpopen >= 1.4
> did you invent a version ? :) next cpopen version will be 1.3.3 probably
Since this add new feature, 1.3.3 (bug fix release) does not seems appropriate.
Line 125: Requires: python-ioprocess >= 0.14
Line 126: Requires: python-pthreading >= 0.1.3-3
Line 127: Requires: python-six
Line 128: Requires: python-requests


https://gerrit.ovirt.org/#/c/45738/15/vdsm/vdsm
File vdsm/vdsm:

Line 255:                          "(stderr: %s). Verify sudoer rules configuration" %
Line 256:                          (stderr))
Line 257: 
Line 258: 
Line 259: def _setCpuAffinity():
> one underscore signs private method, two underscores (__) sign global funct
two underscores (__) sign global function? Never heard of this convention. 

Since this is not a module, everything here is private and we can remove the underscore prefix.

Most of the names in this module are using pep8 style, lets rename it to set_cpu_affinity.
Line 260:     cpu_affinity = config.get('vars', 'cpu_affinity')
Line 261:     if cpu_affinity:
Line 262:         cpu_list = [cpu.strip() for cpu in cpu_affinity.split(',')]
Line 263:         taskset.set(os.getpid(), cpu_list, all_tasks=True)


Line 259: def _setCpuAffinity():
Line 260:     cpu_affinity = config.get('vars', 'cpu_affinity')
Line 261:     if cpu_affinity:
Line 262:         cpu_list = [cpu.strip() for cpu in cpu_affinity.split(',')]
Line 263:         taskset.set(os.getpid(), cpu_list, all_tasks=True)
> and if it runs after log configuration you can add short log about this set
Good idea - this is important change that needs a info log message.
Line 264: 
Line 265: 
Line 266: def main():
Line 267:     __assertVdsmUser()


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f7f68d65eddb5a21afbc3809ea79cd1dee67984
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Ido Barkan <ibarkan 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: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list