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

nsoffer at redhat.com nsoffer at redhat.com
Thu Sep 10 13:56:30 UTC 2015


Nir Soffer has posted comments on this change.

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


Patch Set 11:

(6 comments)

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

Line 73:                   [None],
Line 74:                   [''],
Line 75:                   ['xxx'],
Line 76:                   [-1],
Line 77:                   ])
I think we need only one test for taskset error, testing that we raise the error. We don't want to test here how taskset treat bad input, it does not test our code, and it also does not test a behavior we depend on.
Line 78:     def test_get_with_bad_pid(self, pid):
Line 79:         # looks like taskset is using atoi()
Line 80:         self.assertRaises(taskset.Error,
Line 81:                           taskset.get,


Line 84:     @permutations([
Line 85:                   [[]],
Line 86:                   [['']],
Line 87:                   [['xxx']],
Line 88:                   ])
Same, test is not needed
Line 89:     def test_set_with_bad_cpu_list(self, cpu_list):
Line 90:         pid = self._spawn_helper_child()
Line 91:         self.assertRaises(taskset.Error,
Line 92:                           taskset.set,


Line 93:                           pid,
Line 94:                           cpu_list)
Line 95: 
Line 96:     @ValidateNotRunningAsRoot
Line 97:     def test_set_pid_from_different_user(self):
Test is not needed, we are not testing our code here.
Line 98:         # minimize damage if this unexpectedly succeds
Line 99:         self.assertRaises(taskset.Error,
Line 100:                           taskset.set,
Line 101:                           1,


Line 106: 
Line 107:         self.assertRaises(taskset.Error,
Line 108:                           taskset.set,
Line 109:                           pid,
Line 110:                           [])
Not needed, and repeat test_set_with_bad_cpu_list
Line 111: 
Line 112:     def test_set_malformed_cpu_list(self):
Line 113:         pid = self._spawn_helper_child()
Line 114: 


Line 114: 
Line 115:         self.assertRaises(taskset.Error,
Line 116:                           taskset.set,
Line 117:                           pid,
Line 118:                           ['xxx'])
Not needed, and repeat test_set_with_bad_cpu_list

Looks like you forgot the delete the tests after you added the one tests with permutations.
Line 119: 
Line 120:     def _spawn_helper_child(self, target=None):
Line 121:         if target is None:
Line 122:             target = self._run_helper


Line 130: 
Line 131:     def _run_helper(self):
Line 132:         self.running.set()
Line 133:         while not self.stop.is_set():
Line 134:             time.sleep(0.1)
I think the original tests that did not use these abstractions were more clear.
Line 135: 
Line 136: 
Line 137: # we expect that on devel/test boxes there are no offline cpu
Line 138: def all_active_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: 11
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: 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: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list