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 <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Ido Barkan <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: Martin Sivák <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: Yaniv Kaul <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
