Dan Kenigsberg has posted comments on this change. Change subject: scale: limit cpu usage using cpu-affinity ......................................................................
Patch Set 21: Code-Review-1 (7 comments) https://gerrit.ovirt.org/#/c/45738/21/lib/vdsm/taskset.py File lib/vdsm/taskset.py: Line 42: We assume all threads of the process have the same affinity, because Line 43: this is the only usecase VDSM cares about - and requires. Line 44: Return a list of strings, each one being a cpu indices on which the Line 45: process can run. Line 46: Example: ['0', '1', '2', '3'] It would be more noble (and in my opinion, safer) for this function to return a list of integers, so that sort and max are easier. Too bad that "taskset" calls it --cpu-list, as the pythonic type for the return value would be a frozenset(). Line 47: Raise taskset.Error on failure. Line 48: """ Line 49: command = [constants.EXT_TASKSET, '--pid', str(pid)] Line 50: Line 91: sorted why do we care about the order? strictly speaking, `taskset --pid` returns a frozenset(). and do recall that the order is a bit awkward >>> sorted(['1', '2', '10']) ['1', '10', '2'] https://gerrit.ovirt.org/#/c/45738/21/tests/tasksetTests.py File tests/tasksetTests.py: Line 54: not self.stop could you add a comment on why self.stop.wait() is not good? Line 59: self.assertTrue(self.running.wait(0.5)) If we fail to get the subprocess running within 0.5 sec, it's an environment error, not a failure to read the taskset. Thus we should raise here, not assert. Line 57: self.proc = multiprocessing.Process(target=_run_helper) Line 58: self.proc.start() Line 59: self.assertTrue(self.running.wait(0.5)) Line 60: Line 61: self.assertEqual(taskset.get(self.proc.pid), online_cpus()) this test would fail if the test runner is executed under a limiting taskset. no biggie... Line 62: Line 63: @permutations(_CPU_COMBINATIONS) Line 64: def test_set_from_parent(self, cpu_list): Line 65: Line 81: def test_set_from_child(self, cpu_list): Line 82: Line 83: validate_running_with_enough_cpus(cpu_list) Line 84: Line 85: def _run_helper(): I believe that _run_helper(cpu_list=()) can be written once, on the class level, instead of repeated 3 times. Line 86: # avoid race on startup: first do taskset, then notify Line 87: taskset.set(os.getpid(), cpu_list) Line 88: self.running.set() Line 89: while not self.stop.is_set(): Line 106: Line 107: # TODO: find a clean way to make this a decorator Line 108: def validate_running_with_enough_cpus(cpu_list): Line 109: max_available_cpu = int(online_cpus()[-1]) Line 110: max_required_cpu = int(sorted(cpu_list)[-1]) since '10' < '2', I think you want max(int(i) for i in cpu_list) Line 111: Line 112: if (max_available_cpu < max_required_cpu): Line 113: raise SkipTest( Line 114: "This test requires at least %i available CPUs" -- 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: 21 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
