Francesco Romani has posted comments on this change. Change subject: scale: limit cpu usage using cpu-affinity ......................................................................
Patch Set 15: (10 comments) https://gerrit.ovirt.org/#/c/45738/15/debian/control File debian/control: Line 16: psmisc (>= 22.6), Line 17: pyflakes, Line 18: python (>=2.7.3), Line 19: python-dev, Line 20: python-cpopen (>= 1.4), > it doesn't exist yet reverted Line 21: python-dmidecode, Line 22: python-libvirt (>= 1.2.9), Line 23: python-m2crypto, Line 24: python-netaddr, https://gerrit.ovirt.org/#/c/45738/15/lib/vdsm/constants.py.in File lib/vdsm/constants.py.in: Line 137: EXT_SU = '@SU_PATH@' Line 138: EXT_SUDO = '@SUDO_PATH@' Line 139: Line 140: EXT_TAR = '@TAR_PATH@' Line 141: EXT_TASKSET = '@TASKSET_PATH@' > why do you need it here? you use it only once in the new module. declare it we had a circular dependency issue between taskset.py, cmdutils.py and utils.py. Will check if this is still the case and update accordingly. Line 142: EXT_TUNE2FS = '@TUNE2FS_PATH@' Line 143: Line 144: EXT_UMOUNT = '@UMOUNT_PATH@' Line 145: 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): > Using RuntimeError makes error handling harder or impossible. You cannot de I strongly agree with Nir. Kept this Error. 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/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 648 Line 649 Line 650 Line 651 Line 652 > Previously we added taskset first, creating this command: Yes, funny how we missed a few revisions ago :) Done. Line 666: execCmdLogger.debug(cmdutils.command_log_line(printable, cwd=cwd)) Line 667: Line 668: p = CPopen(command, close_fds=True, cwd=cwd, env=env, Line 669: deathSignal=deathSignal, childUmask=childUmask, Line 670: resetCpuAffinity=resetCpuAffinity) > having a wrapper process makes it easier - no cpopen changes, can be used l I agree about better avoiding cpopen. I agree that we should (try to) drop cpopen when switching to py3. I'm not convincend that adding so many layers of wrappers is a good solution. Anyway, for this patch is the best approach, so: Done. Line 671: if not sync: Line 672: p = AsyncProc(p) Line 673: if data is not None: Line 674: p.stdin.write(data) 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'] > Makes sense, but this tests will work only on machine with more then one cp let me see what I can do that. 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 > until we don't have build for that better to use TBD Not needed anymore (hopefully :) ), just removed. 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(): > you can do it in separate patch, but for now keep the file standards ... tw I also never heard of this convention, anyway no big deal. Done. 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) > Good idea - this is important change that needs a info log message. Done Line 264: Line 265: Line 266: def main(): Line 267: __assertVdsmUser() Line 279: Line 280: _setCpuAffinity() Line 281: Line 282: argDict = parse_args() Line 283: run(**argDict) > I would call it inside the run - here are just few validation checks Done Line 284: Line 285: Line 286: if __name__ == '__main__': Line 287: try: -- 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
