Nir Soffer has posted comments on this change. Change subject: scale: limit cpu usage using cpu-affinity ......................................................................
Patch Set 18: (8 comments) Nice, few things can be improved. https://gerrit.ovirt.org/#/c/45738/18/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 648 Line 649 Line 650 Line 651 Line 652 Yaniv: for another patch - we should add sudo first - will make the sudoers rules simpler and safer. These commands shuld be equivalent: sudo nice nice-args ionice ionice-args real-command nice nice-args ionice ionice-args sudo real-command Writing safe sudoers rule for the first command is impossible. Need to check about setsid. Line 649 Line 650 Line 651 Line 652 Line 653 Add comment about importance of order; taskset must be added after sudo so it come before it in the final command. Line 625: def __del__(self): Line 626: self._poller.close() Line 627: Line 628: Line 629: _ANY_CPU = ["0-%d" % (os.sysconf('SC_NPROCESSORS_CONF') - 1)] We were using only online cpus in cpopen, and we can use it here with "SC_NPROCESSORS_ONLN" Tested on rhel 7.1 and 6.6. Line 630: Line 631: Line 632: _USING_CPU_AFFINITY = config.get('vars', 'cpu_affinity') != "" Line 633: https://gerrit.ovirt.org/#/c/45738/18/tests/testlib.py File tests/testlib.py: Line 466: Line 467: return wrapper Line 468: Line 469: Line 470: # we expect that on devel/test boxes there are no offline cpu Use sysconf to get the online cpus instead of depending on multiprocessing for this. Also better call it online_cpus? The manual says: "The number of processors currently online (available)." Line 471: def all_active_cpus(): https://gerrit.ovirt.org/#/c/45738/18/tests/utilsTests.py File tests/utilsTests.py: Line 526: resetCpuAffinity=True) Line 527: Line 528: self.assertEquals(taskset.get(proc.pid), Line 529: all_active_cpus()) Line 530: proc.kill() Use try finally so the process is kill when the test fail. Line 531: Line 532: Line 533: class ExecCmdStressTest(TestCaseBase): Line 534: Line 528: self.assertEquals(taskset.get(proc.pid), Line 529: all_active_cpus()) Line 530: proc.kill() Line 531: Line 532: Missing test that we do not change cpu affinity when _USING_CPU_AFFINITY is False. Line 533: class ExecCmdStressTest(TestCaseBase): Line 534: Line 535: CONCURRENCY = 50 Line 536: FUNC_DELAY = 0.01 https://gerrit.ovirt.org/#/c/45738/18/vdsm/vdsm File vdsm/vdsm: Line 164: sysname, nodename, release, version, machine = os.uname() Line 165: log.info('(PID: %s) I am the actual vdsm %s %s (%s)', Line 166: pid, dsaversion.raw_version_revision, nodename, release) Line 167: Line 168: __set_cpu_affinity(log) Passing the log is ugly, we should use getLogger in functions, or set the log as global. But we can clean this later as serve_clients is already using this anti pattern. Line 169: Line 170: serve_clients(log) Line 171: except: Line 172: log.error("Exception raised", exc_info=True) Line 263: cpu_affinity = config.get('vars', 'cpu_affinity') Line 264: if cpu_affinity: Line 265: cpu_list = [cpu.strip() for cpu in cpu_affinity.split(',')] Line 266: taskset.set(os.getpid(), cpu_list, all_tasks=True) Line 267: log.info('VDSM configured with cpu_affinity: %s', cpu_list) Better log before you make the change. This makes errors more clear - you see the log when you attempt to do something, and then you see the error. This is very important when an operation takes lot of time. Not critical here, but I would like to have consistent logging anywhere is vdsm. Line 268: Line 269: Line 270: def main(): Line 271: __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: 18 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
