Change in vdsm[master]: vm: conf: proper locking in setNumberOfCpus()

mpolednik at redhat.com mpolednik at redhat.com
Mon Apr 11 07:45:42 UTC 2016


Martin Polednik has posted comments on this change.

Change subject: vm: conf: proper locking in setNumberOfCpus()
......................................................................


Patch Set 4: Code-Review-1

(1 comment)

I'm OK with the change itself, I dislike the test for it's failure to deliver consistent results (have you seen it fail at some point?).

https://gerrit.ovirt.org/#/c/54910/4/tests/vmOperationsTests.py
File tests/vmOperationsTests.py:

PS4, Line 321:     def testSetNumberOfVcpusConcurrently(self):
             : 
             :         with MonkeyPatchScope(
             :             [(hooks, 'before_set_num_of_cpus', lambda: None),
             :              (hooks, 'after_set_num_of_cpus', lambda: None)]
             :         ):
             :             with fake.VM() as testvm:
             :                 smp = 8  # random value
             :                 dom = fake.Domain()
             :                 dom.setVcpusFlags = lambda *args: None
             :                 testvm._dom = dom
             : 
             :                 done = threading.Event()
             :                 res = [None]
             : 
             :                 def _helper():
             :                     time.sleep(0.)  # yield the CPU
             :                     res[0] = testvm.setNumberOfCpus(smp)
             :                     done.set()
             : 
             :                 with _run_thread(concurrent.thread(_helper)):
             :                     while not done.is_set():
             :                         testvm.status()
             : 
             :                 self.assertFalse(response.is_error(res[0]))
             :                 self.assertEquals(testvm.conf['smp'], str(smp))
I don't feel that this tests adds any value as it's not consistent - I'm unable to fail it even with unpatched conf (~100k iterations). It feels more dangerous to have this test (where someone could eventually depend on it) X not having it at all.


-- 
To view, visit https://gerrit.ovirt.org/54910
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I20822e31af1e7350833d9c3080efc5d0f79a5cbe
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani 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: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list