Change in vdsm[master]: virt: Limit the number of workers in executor
gerrit-hooks has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 22: * Update tracker: IGNORE, no Bug-Url found * Set MODIFIED::IGNORE, no Bug-Url found. -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 22 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: virt: Limit the number of workers in executor
Nir Soffer has submitted this change and it was merged. Change subject: virt: Limit the number of workers in executor .. virt: Limit the number of workers in executor In some situations, periodic operations may hang inside executor worker threads. We have observed that in real situations when libvirt calls started being blocked due to some bug. Such situations shouldn't happen when everything works correctly, but actually not everything works correctly and there are unexpected bugs. We should try to minimize the possible damages. The result of the hanging periodic calls is that the number of worker threads in the executor grows above all limits. This is not good. So we limit the maximum number of the worker threads in the executor with this patch. We set the maximum number of the threads to a value sufficiently high for harmless temporary problems. If the number of the workers is higher than that then there is probably a real problem and we should start emergency actions, i.e. not spawning further threads in this case. Note that a single isolated problem of this kind may block all the periodic operations. We introduce a bit more fine grained approach to handle that in a followup patch. Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Signed-off-by: Milan Zamazal Backport-To: 4.0 Backport-To: 3.6 Reviewed-on: https://gerrit.ovirt.org/57754 Reviewed-by: Francesco Romani Reviewed-by: Nir Soffer Continuous-Integration: Jenkins CI --- M lib/vdsm/config.py.in M lib/vdsm/executor.py M lib/vdsm/virt/periodic.py M tests/executorTests.py 4 files changed, 154 insertions(+), 8 deletions(-) Approvals: Nir Soffer: Looks good to me, approved Jenkins CI: Passed CI tests Francesco Romani: Looks good to me, approved Milan Zamazal: Verified -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: merged Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 22 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: virt: Limit the number of workers in executor
gerrit-hooks has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 21: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0']) -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 21 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: virt: Limit the number of workers in executor
Nir Soffer has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 20: Code-Review+2 (1 comment) https://gerrit.ovirt.org/#/c/57754/20/tests/executorTests.py File tests/executorTests.py: Line 269: self.wait = wait Line 270: self.error = error Line 271: self.event = event Line 272: self.start_barrier = start_barrier Line 273: self.started = threading.Event() > We use the event in __repr__ for debugging purposes. Not necessary, but occ OK Line 274: self.executed = threading.Event() Line 275: Line 276: def __call__(self): Line 277: self.started.set() -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 20 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: virt: Limit the number of workers in executor
Milan Zamazal has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 20: (3 comments) https://gerrit.ovirt.org/#/c/57754/20/tests/executorTests.py File tests/executorTests.py: Line 88 Line 89 Line 90 Line 91 Line 92 > Old tests are very fragile, it would be nice to replace the sleeps with wai Indeed, I've already met a situation when an executor test failed on some insufficient timeout (or on a bug -- who knows?) when running `make check'. So this patch can serve as a basis for future improvements in this area. We've got more fragile tests and they waste our work occasionally, so it's worth to fix what we can reasonably fix. Line 269: self.wait = wait Line 270: self.error = error Line 271: self.event = event Line 272: self.start_barrier = start_barrier Line 273: self.started = threading.Event() > We are not using this event. We use the event in __repr__ for debugging purposes. Not necessary, but occasionally useful. Line 274: self.executed = threading.Event() Line 275: Line 276: def __call__(self): Line 277: self.started.set() Line 280: if self.wait: Line 281: time.sleep(self.wait) Line 282: if self.event is not None: Line 283: if not self.event.wait(10): Line 284: return > Both wait and event have similar usage, making the task block for some time It looks like a good idea. We don't use `wait' in this patch, so let's try to remove it in a future timeout-based-tests cleanup patch. I'll add it to my todo list. Line 285: self.executed.set() Line 286: if self.error: Line 287: raise self.error Line 288: -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 20 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: virt: Limit the number of workers in executor
Nir Soffer has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 20: Code-Review-1 (3 comments) https://gerrit.ovirt.org/#/c/57754/20/tests/executorTests.py File tests/executorTests.py: Line 88 Line 89 Line 90 Line 91 Line 92 Old tests are very fragile, it would be nice to replace the sleeps with waiting for events. This is not related to this patch, but it adds nice infrastructure that these tests can use. Line 269: self.wait = wait Line 270: self.error = error Line 271: self.event = event Line 272: self.start_barrier = start_barrier Line 273: self.started = threading.Event() We are not using this event. Lets remove it, and rename start_barrier to started? Line 274: self.executed = threading.Event() Line 275: Line 276: def __call__(self): Line 277: self.started.set() Line 280: if self.wait: Line 281: time.sleep(self.wait) Line 282: if self.event is not None: Line 283: if not self.event.wait(10): Line 284: return Both wait and event have similar usage, making the task block for some time. Can we unify them and always pass an event instead of sleep time? Line 285: self.executed.set() Line 286: if self.error: Line 287: raise self.error Line 288: -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 20 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: virt: Limit the number of workers in executor
gerrit-hooks has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 20: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0']) -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 20 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: virt: Limit the number of workers in executor
Milan Zamazal has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 19: Verified+1 Verified by injecting time.sleep(10) into _RunnableOnVm.__call__, printing the number of executor workers to the log and checking that the number reaches 30 and stops there. I additionally checked that basic things like starting, stopping and migrating a VM work normally with this patch. -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 19 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: virt: Limit the number of workers in executor
gerrit-hooks has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 19: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0']) -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 19 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: virt: Limit the number of workers in executor
Francesco Romani has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 18: Code-Review+2 (1 comment) https://gerrit.ovirt.org/#/c/57754/18/tests/executorTests.py File tests/executorTests.py: Line 177: Line 178: # The other tasks shouldn't be unblocked and executed, let's check Line 179: # things go as expected before proceeding (however we don't want to Line 180: # stop and wait on each of the tasks, the first one is enough) Line 181: self.assertFalse(tasks[0].executed.wait(1)) > I think it's better to give more chance than a fraction of second to potent fair point. Line 182: self.assertEqual([t for t in tasks if t.executed.is_set()], Line 183: [tasks[-1]]) Line 184: Line 185: # Extra tasks are not blocking, they were blocked just by the -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 18 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: virt: Limit the number of workers in executor
Milan Zamazal has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 18: (1 comment) https://gerrit.ovirt.org/#/c/57754/18/tests/executorTests.py File tests/executorTests.py: Line 177: Line 178: # The other tasks shouldn't be unblocked and executed, let's check Line 179: # things go as expected before proceeding (however we don't want to Line 180: # stop and wait on each of the tasks, the first one is enough) Line 181: self.assertFalse(tasks[0].executed.wait(1)) > minor: perhaps just use wait(0.01) and check all of them? I think it's better to give more chance than a fraction of second to potentially execute a task rather than to save one line of code. Line 182: self.assertEqual([t for t in tasks if t.executed.is_set()], Line 183: [tasks[-1]]) Line 184: Line 185: # Extra tasks are not blocking, they were blocked just by the -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 18 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: virt: Limit the number of workers in executor
Francesco Romani has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 18: Code-Review+1 (1 comment) one minor comment in the tests, overall looks good to me. https://gerrit.ovirt.org/#/c/57754/18/tests/executorTests.py File tests/executorTests.py: Line 177: Line 178: # The other tasks shouldn't be unblocked and executed, let's check Line 179: # things go as expected before proceeding (however we don't want to Line 180: # stop and wait on each of the tasks, the first one is enough) Line 181: self.assertFalse(tasks[0].executed.wait(1)) minor: perhaps just use wait(0.01) and check all of them? Line 182: self.assertEqual([t for t in tasks if t.executed.is_set()], Line 183: [tasks[-1]]) Line 184: Line 185: # Extra tasks are not blocking, they were blocked just by the -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 18 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: virt: Limit the number of workers in executor
Milan Zamazal has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 18: (1 comment) https://gerrit.ovirt.org/#/c/57754/16/tests/executorTests.py File tests/executorTests.py: Line 46: self.executor.start() Line 47: time.sleep(0.1) # Give time to start all threads Line 48: Line 49: def tearDown(self): Line 50: self.executor.stop() > We should use a timeout in the barrier. OK, let's use timeouts on barriers and try-finally cleanup for events. Line 51: self.scheduler.stop() Line 52: Line 53: def test_dispatch_not_running(self): Line 54: self.executor.stop() -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 18 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: virt: Limit the number of workers in executor
Nir Soffer has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 16: (1 comment) https://gerrit.ovirt.org/#/c/57754/16/tests/executorTests.py File tests/executorTests.py: Line 46: self.executor.start() Line 47: time.sleep(0.1) # Give time to start all threads Line 48: Line 49: def tearDown(self): Line 50: self.executor.stop(wait=False) > I agree, but in case there is a bug in the executor code that leaves a stuc We should use a timeout in the barrier. If there is a bug in the executor during shutdown, the tests will hang. The only way to fix this is to kill the tests process with a timer, or to run such tests in a subprocess and kill the subprocess. The test cannot create an executor with block threads and leave it running in blocked state for the next 100 seconds. When the test is done (succeeded or failed), all tasks must be unblocked so the executor can stop cleanly. Since we control the Task class, I don't see any reason not to implement this. Line 51: self.scheduler.stop() Line 52: Line 53: def test_dispatch_not_running(self): Line 54: self.executor.stop() -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 16 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: virt: Limit the number of workers in executor
Milan Zamazal has posted comments on this change.
Change subject: virt: Limit the number of workers in executor
..
Patch Set 17:
(11 comments)
https://gerrit.ovirt.org/#/c/57754/16/tests/executorTests.py
File tests/executorTests.py:
Line 183
Line 184
Line 185
Line 186
Line 187
> Better change to wait on an event, so we can have block tasks, and release
That would require touching the tests unrelated to this patch, so I'd leave it
for a followup patch or patches.
Line 35:
Line 36: def setUp(self):
Line 37: self.scheduler = schedule.Scheduler()
Line 38: self.scheduler.start()
Line 39: self.max_tasks = 20
> Why did you change max_task to be lower then max_workers?
I didn't change the original max_tasks value, I just extracted it to an
attribute, since it is used in the tests.
As for max_workers, I selected higher value than max_tasks not to mess with
other tests. But it seems all the tests should work with max_workers == 15, so
let's use that value (getting rid of waiting on the queue is a good idea). An
alternative would be to make a separate test (sub)class, but it's not needed
now.
Line 40: self.max_workers = 15
Line 41: self.executor = executor.Executor('test',
Line 42: workers_count=10,
Line 43: max_tasks=self.max_tasks,
Line 46: self.executor.start()
Line 47: time.sleep(0.1) # Give time to start all threads
Line 48:
Line 49: def tearDown(self):
Line 50: self.executor.stop()
> This is not a good idea. We want to clean up properly after each test.
I agree, but in case there is a bug in the executor code that leaves a stuck
task somewhere then we hang here. This is not what we want. Do you have a
suggestion how to cleanup properly here even when a stuck task is present?
I removed the wait parameter here and added a regular cleanup to the tests but
we should still fix the problem in a followup patch. If we don't then we should
improve the cleanup in the tests to make sure we don't hang on failed
assertions, which may be a bit complex (especially with barriers).
Line 51: self.scheduler.stop()
Line 52:
Line 53: def test_dispatch_not_running(self):
Line 54: self.executor.stop()
Line 137: time.sleep(0.3)
Line 138: for task in tasks:
Line 139: self.assertTrue(task.executed.is_set())
Line 140:
Line 141: @slowtest
> We can remove this if we fix max_tasks.
Done
Line 142: def test_max_workers(self):
Line 143: limit = self.max_workers
Line 144: blocked_forever = threading.Event()
Line 145: blocked = threading.Event()
Line 158:
Line 159: # Try to run new tasks on the executor now, when the maximum
number of
Line 160: # workers is reached
Line 161: n_extra_tasks = 2
Line 162: extra_tasks = [Task() for i in range(n_extra_tasks)]
> Using bigger max_tasks will make this unneeded.
Done
Line 163: for t in extra_tasks:
Line 164: self.executor.dispatch(t, 0)
Line 165:
Line 166: # Check that none of the new tasks got executed (the number
of the
Line 169:
Line 170: # Unblock one of the tasks and check the new tasks run
Line 171: blocked.set()
Line 172: # The last task, the only unblocked one, should be executed
now
Line 173: self.assertTrue(tasks[-1].executed.wait(1))
> Same, not needed.
Done
Line 174:
Line 175: # The other tasks shouldn't be unblocked and executed, let's
check
Line 176: # things go as expected before proceeding (however we don't
want to
Line 177: # stop and wait on each of the tasks, the first one is enough)
Line 170: # Unblock one of the tasks and check the new tasks run
Line 171: blocked.set()
Line 172: # The last task, the only unblocked one, should be executed
now
Line 173: self.assertTrue(tasks[-1].executed.wait(1))
Line 174:
> Add blank line to separate logical blocks.
Done
Line 175: # The other tasks shouldn't be unblocked and executed, let's
check
Line 176: # things go as expected before proceeding (however we don't
want to
Line 177: # stop and wait on each of the tasks, the first one is enough)
Line 178: self.assertFalse(tasks[0].executed.wait(1))
Line 178: self.assertFalse(tasks[0].executed.wait(1))
Line 179: self.assertEqual([t for t in tasks if t.executed.is_set()],
Line 180: [tasks[-1]])
Line 181:
Line 182: # Extra tasks are not blocking, they were blocked just by the
overflown
> Add blank line to make it easier to follow.
Done
Line 183: # executor, so they should be all executed now when there is
one free
Line 184: # worker
Line 185: self.assertEqual([t for t in extra_tasks if not
t.executed.wai
Change in vdsm[master]: virt: Limit the number of workers in executor
Nir Soffer has posted comments on this change.
Change subject: virt: Limit the number of workers in executor
..
Patch Set 16:
(13 comments)
https://gerrit.ovirt.org/#/c/57754/16/tests/executorTests.py
File tests/executorTests.py:
Line 36
Line 37
Line 38
Line 39
Line 40
Please keep this for the new tests unless there is a reason to change.
Line 183
Line 184
Line 185
Line 186
Line 187
Better change to wait on an event, so we can have block tasks, and release them
when the test ends.
t = Task(wait=100)
executor.dispatch(t)
try:
assert not t.started.wait(1)
finally:
t.waiter.set()
Line 35:
Line 36: def setUp(self):
Line 37: self.scheduler = schedule.Scheduler()
Line 38: self.scheduler.start()
Line 39: self.max_tasks = 20
Why did you change max_task to be lower then max_workers?
This seems to cause trouble for your tests, having to wait to prevent too many
tasks error.
I don't like changing this for the old tests - please explain why this needed
for the new tests.
Line 40: self.max_workers = 30
Line 41: self.executor = executor.Executor('test',
Line 42: workers_count=10,
Line 43: max_tasks=self.max_tasks,
Line 46: self.executor.start()
Line 47: time.sleep(0.1) # Give time to start all threads
Line 48:
Line 49: def tearDown(self):
Line 50: self.executor.stop(wait=False)
This is not a good idea. We want to clean up properly after each test.
Tests should not leave blocked threads in the executor, so we should be able to
wait for the executor.
Line 51: self.scheduler.stop()
Line 52:
Line 53: def test_dispatch_not_running(self):
Line 54: self.executor.stop()
Line 137: time.sleep(0.3)
Line 138: for task in tasks:
Line 139: self.assertTrue(task.executed.is_set())
Line 140:
Line 141: def _wait_for_queue(self):
We can remove this if we fix max_tasks.
Line 142: timeout = 1
Line 143: queue = self.executor._tasks
Line 144: start = utils.monotonic_time()
Line 145: while len(queue._tasks) >= queue._max_tasks:
Line 158: tasks = [Task(event=blocked_forever,
start_barrier=start_barrier)
Line 159: for n in range(limit - 1)]
Line 160: tasks.append(Task(event=blocked, start_barrier=start_barrier))
Line 161: for t in tasks:
Line 162: self._wait_for_queue() # prevent TooManyTasks error
Using bigger max_tasks will make this unneeded.
Line 163: self.executor.dispatch(t, 0)
Line 164: # Wait until all tasks are started, i.e. the executor reaches
its
Line 165: # maximum number of workers
Line 166: start_barrier.wait(timeout=3)
Line 169: # workers is reached
Line 170: n_extra_tasks = 2
Line 171: extra_tasks = [Task() for i in range(n_extra_tasks)]
Line 172: for t in extra_tasks:
Line 173: self._wait_for_queue() # prevent TooManyTasks error
Same, not needed.
Line 174: self.executor.dispatch(t, 0)
Line 175: # Check that none of the new tasks got executed (the number
of the
Line 176: # executor workers is at the maximum limit, so nothing more
may be run)
Line 177: self.assertEqual([t for t in extra_tasks if
t.executed.wait(1)], [])
Line 170: n_extra_tasks = 2
Line 171: extra_tasks = [Task() for i in range(n_extra_tasks)]
Line 172: for t in extra_tasks:
Line 173: self._wait_for_queue() # prevent TooManyTasks error
Line 174: self.executor.dispatch(t, 0)
Add blank line to separate logical blocks.
Line 175: # Check that none of the new tasks got executed (the number
of the
Line 176: # executor workers is at the maximum limit, so nothing more
may be run)
Line 177: self.assertEqual([t for t in extra_tasks if
t.executed.wait(1)], [])
Line 178:
Line 178:
Line 179: # Unblock one of the tasks and check the new tasks run
Line 180: blocked.set()
Line 181: # The last task, the only unblocked one, should be executed
now
Line 182: self.assertTrue(tasks[-1].executed.wait(1))
Add blank line to make it easier to follow.
Line 183: # The other tasks shouldn't be unblocked and executed, let's
check
Line 184: # things go as expected before proceeding (however we don't
want to
Line 185: # stop and wait on each of the tasks, the first one is enough)
Line 186: self.assertFalse(tasks[0].executed.wait(1))
Line 184: # things go as expected before proceeding (however we don't
want to
Line 185: # stop and wait on each of the tasks, the first one is enough)
Line 186: self.assertFalse(tasks[0].executed.wait(1))
Line 187: self.assertEqual([t for t in tasks
Change in vdsm[master]: virt: Limit the number of workers in executor
Milan Zamazal has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 16: (4 comments) https://gerrit.ovirt.org/#/c/57754/15/tests/executorTests.py File tests/executorTests.py: Line 156: # later Line 157: start_barrier = concurrent.Barrier(limit + 1) Line 158: tasks = [Task(event=blocked_forever, start_barrier=start_barrier) Line 159: for n in range(limit - 1)] Line 160: tasks.append(Task(event=blocked, start_barrier=start_barrier)) > Why do we need this wait? To prevent TooManyTasks error. Line 161: for t in tasks: Line 162: self._wait_for_queue() # prevent TooManyTasks error Line 163: self.executor.dispatch(t, 0) Line 164: # Wait until all tasks are started, i.e. the executor reaches its Line 158: tasks = [Task(event=blocked_forever, start_barrier=start_barrier) Line 159: for n in range(limit - 1)] Line 160: tasks.append(Task(event=blocked, start_barrier=start_barrier)) Line 161: for t in tasks: Line 162: self._wait_for_queue() # prevent TooManyTasks error > This wait until all tasks started? Please comment this. Indeed, barrier can be utilized here, thanks for the tip. Done. Line 163: self.executor.dispatch(t, 0) Line 164: # Wait until all tasks are started, i.e. the executor reaches its Line 165: # maximum number of workers Line 166: start_barrier.wait(timeout=3) Line 160: tasks.append(Task(event=blocked, start_barrier=start_barrier)) Line 161: for t in tasks: Line 162: self._wait_for_queue() # prevent TooManyTasks error Line 163: self.executor.dispatch(t, 0) Line 164: # Wait until all tasks are started, i.e. the executor reaches its > We assume that all workers are discarded now, right? please explain this in Comments added. Line 165: # maximum number of workers Line 166: start_barrier.wait(timeout=3) Line 167: Line 168: # Try to run new tasks on the executor now, when the maximum number of Line 166: start_barrier.wait(timeout=3) Line 167: Line 168: # Try to run new tasks on the executor now, when the maximum number of Line 169: # workers is reached Line 170: n_extra_tasks = 2 > What are you trying to test here, that the new tasks are queued? Comments added. Line 171: extra_tasks = [Task() for i in range(n_extra_tasks)] Line 172: for t in extra_tasks: Line 173: self._wait_for_queue() # prevent TooManyTasks error Line 174: self.executor.dispatch(t, 0) -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 16 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: virt: Limit the number of workers in executor
gerrit-hooks has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 16: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0']) -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 16 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: virt: Limit the number of workers in executor
Nir Soffer has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 15: (4 comments) Partial review https://gerrit.ovirt.org/#/c/57754/15/tests/executorTests.py File tests/executorTests.py: Line 156: # later Line 157: tasks = [Task(event=blocked_forever) for n in range(limit - 1)] Line 158: tasks.append(Task(event=blocked)) Line 159: for t in tasks: Line 160: self._wait_for_queue() Why do we need this wait? Line 161: self.executor.dispatch(t, 0) Line 162: self.assertEqual([t for t in tasks if not t.started.wait(1)], []) Line 163: Line 164: # Try to run new tasks on the overflown executor Line 158: tasks.append(Task(event=blocked)) Line 159: for t in tasks: Line 160: self._wait_for_queue() Line 161: self.executor.dispatch(t, 0) Line 162: self.assertEqual([t for t in tasks if not t.started.wait(1)], []) This wait until all tasks started? Please comment this. This can be simplified by using concurrent.Barrier. You pass the barrier to all tasks and wait: barrier = concurrent.Barrier(limit + 1) # +1 for current thread tasks = [Task(event=blocked_forever, started=barrier) for n in range(limit - 1)] tasks.append(Task(event=blocked, started=barrier)) for t in tasks: self.executor.dispatch(t, 0) barrier.wait() # All threads are running here Assuming that Task accept a barrier, and wait on it: class Task(object): def __init__(..., started=None): self.started = started def __call__(self): if self.started: self.started.wait() Line 163: Line 164: # Try to run new tasks on the overflown executor Line 165: n_extra_tasks = 2 Line 166: extra_tasks = [Task() for i in range(n_extra_tasks)] Line 160: self._wait_for_queue() Line 161: self.executor.dispatch(t, 0) Line 162: self.assertEqual([t for t in tasks if not t.started.wait(1)], []) Line 163: Line 164: # Try to run new tasks on the overflown executor We assume that all workers are discarded now, right? please explain this in the comments. Line 165: n_extra_tasks = 2 Line 166: extra_tasks = [Task() for i in range(n_extra_tasks)] Line 167: for t in extra_tasks: Line 168: self._wait_for_queue() Line 166: extra_tasks = [Task() for i in range(n_extra_tasks)] Line 167: for t in extra_tasks: Line 168: self._wait_for_queue() Line 169: self.executor.dispatch(t, 0) Line 170: self.assertEqual([t for t in extra_tasks if t.executed.wait(1)], []) What are you trying to test here, that the new tasks are queued? Please explain what is the expected state at the end of the block. Line 171: Line 172: # Unblock one of the tasks and check the new tasks run Line 173: blocked.set() Line 174: self.assertTrue(tasks[-1].executed.wait(1)) -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: virt: Limit the number of workers in executor
Nir Soffer has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 15: The code looks fine, need to review the tests again. -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: virt: Limit the number of workers in executor
gerrit-hooks has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 15: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0']) -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: virt: Limit the number of workers in executor
Nir Soffer has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 14: (1 comment) https://gerrit.ovirt.org/#/c/57754/14/lib/vdsm/executor.py File lib/vdsm/executor.py: Line 105: # Serving workers Line 106: Line 107: @property Line 108: def _active_workers(self): Line 109: return len([w for w in self._workers if not w.discarded]) > have you evaluated a genexp? not sure if safe/faster here, but still worth It does not work, len does not work with iterators. Line 110: Line 111: @property Line 112: def _total_workers(self): Line 113: return len(self._workers) -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: virt: Limit the number of workers in executor
Francesco Romani has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 14: Code-Review+1 (1 comment) https://gerrit.ovirt.org/#/c/57754/14/lib/vdsm/executor.py File lib/vdsm/executor.py: Line 105: # Serving workers Line 106: Line 107: @property Line 108: def _active_workers(self): Line 109: return len([w for w in self._workers if not w.discarded]) have you evaluated a genexp? not sure if safe/faster here, but still worth a thought. Line 110: Line 111: @property Line 112: def _total_workers(self): Line 113: return len(self._workers) -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: virt: Limit the number of workers in executor
Milan Zamazal has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 14: Missing comma in a tuple added. -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: virt: Limit the number of workers in executor
gerrit-hooks has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 14: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0']) -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: virt: Limit the number of workers in executor
Milan Zamazal has posted comments on this change.
Change subject: virt: Limit the number of workers in executor
..
Patch Set 13:
(13 comments)
https://gerrit.ovirt.org/#/c/57754/12/lib/vdsm/executor.py
File lib/vdsm/executor.py:
Line 115: def _may_add_workers(self):
Line 116: return (self._active_workers < self._workers_count and
Line 117: (self._max_workers is None or
Line 118: self._total_workers < self._max_workers))
Line 119:
> Nice! I would call this _can_add_worker or _should_add_worker, and it we sh
Done.
(I used "may" instead of "can", making more clear that even when workers can be
added technically, it needn't be permitted.)
Line 120: def _worker_discarded(self, worker):
Line 121: """
Line 122: Called from scheduler thread when worker was discarded. The
worker
Line 123: thread is blocked on a task, and will exit when the task
finishes.
Line 122: Called from scheduler thread when worker was discarded. The
worker
Line 123: thread is blocked on a task, and will exit when the task
finishes.
Line 124: """
Line 125: worker_added = False
Line 126:
> Maybe use worker_added like the other method?
Done
Line 127: with self._lock:
Line 128: if not self._running:
Line 129: return
Line 130: if self._may_add_workers():
Line 133:
Line 134: # intentionally done outside the lock
Line 135: if not worker_added:
Line 136: self._log.debug("Too many workers (limit=%s), not adding
more",
Line 137: self._max_workers)
> And this becomes:
Done
Line 138: # this is a debug helper, it is not that important to be
precise
Line 139: self._log.debug("executor state: count=%d workers=%s",
Line 140: self._total_workers, self._workers)
Line 141:
https://gerrit.ovirt.org/#/c/57754/12/lib/vdsm/virt/periodic.py
File lib/vdsm/virt/periodic.py:
Line 41: # TODO: make them tunable through private, unsupported configuration
items
Line 42: _WORKERS = config.getint('sampling', 'periodic_workers')
Line 43: _TASK_PER_WORKER = config.getint('sampling',
'periodic_task_per_worker')
Line 44: _TASKS = _WORKERS * _TASK_PER_WORKER
Line 45: _MAX_WORKERS = config.getint('sampling' 'max_workers')
> I want to fail fast when we enter this situation, lets use 30 here, and mak
Done
Line 46:
Line 47:
Line 48: _operations = []
Line 49: _executor = None
https://gerrit.ovirt.org/#/c/57754/11/tests/executorTests.py
File tests/executorTests.py:
Line 143: queue = self.executor._tasks
Line 144: start = utils.monotonic_time()
Line 145: while len(queue._tasks) >= queue._max_tasks:
Line 146: if utils.monotonic_time() - start > timeout:
Line 147: raise Exception("Task queue blocked")
> Please use the existing Task class, so we can create more then one test for
Done
Line 148:
Line 149: @slowtest
Line 150: def test_max_workers(self):
Line 151: limit = self.max_workers
Line 146: if utils.monotonic_time() - start > timeout:
Line 147: raise Exception("Task queue blocked")
Line 148:
Line 149: @slowtest
Line 150: def test_max_workers(self):
> You can add this to the existing Task class.
Done
Line 151: limit = self.max_workers
Line 152: blocked_forever = threading.Event()
Line 153: blocked = threading.Event()
Line 154:
Line 147: raise Exception("Task queue blocked")
Line 148:
Line 149: @slowtest
Line 150: def test_max_workers(self):
Line 151: limit = self.max_workers
> Why do we need this? Better use something generic like waiting on an event
Done
Line 152: blocked_forever = threading.Event()
Line 153: blocked = threading.Event()
Line 154:
Line 155: # Fill the executor with stuck tasks, one of them can be
unblocked
Line 152: blocked_forever = threading.Event()
Line 153: blocked = threading.Event()
Line 154:
Line 155: # Fill the executor with stuck tasks, one of them can be
unblocked
Line 156: # later
> We can replace this by self.done.wait(), assuming that self.done is an even
Done
Line 157: tasks = [Task(event=blocked_forever) for n in range(limit -
1)]
Line 158: tasks.append(Task(event=blocked))
Line 159: for t in tasks:
Line 160: self._wait_for_queue()
Line 154:
Line 155: # Fill the executor with stuck tasks, one of them can be
unblocked
Line 156: # later
Line 157: tasks = [Task(event=blocked_forever) for n in range(limit -
1)]
Line 158: tasks.append(Task(event=blocked))
> Using nested class to access names from the tests scope is smart but make t
Done
Line 159: for t in tasks:
Line 160: self._wait_for_queue()
Lin
Change in vdsm[master]: virt: Limit the number of workers in executor
gerrit-hooks has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 13: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0']) -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: virt: Limit the number of workers in executor
Francesco Romani has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 2: (2 comments) Initial review. Mostly OK, but the test could be perhaps improved. Will follow up with suggestions ASAP. https://gerrit.ovirt.org/#/c/57754/2/tests/executorTests.py File tests/executorTests.py: PS2, Line 163: self.assertEqual(len([t for t in tasks if t.state == 'started']), : limit) self.assertTrue(all(t.state == 'started' for t in tasks)) ? PS2, Line 172: self.assertEqual(len([t for t in extra_tasks if t.executed.is_set()]), : 0) self.assertFalse(any(t.executed.is_set() for t in extra_tasks)) ? -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: Limit the number of workers in executor
Francesco Romani has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 2: (3 comments) https://gerrit.ovirt.org/#/c/57754/2/lib/vdsm/executor.py File lib/vdsm/executor.py: Line 53: Line 54: def __init__(self, name, workers_count, max_tasks, scheduler): Line 55: self._name = name Line 56: self._workers_count = workers_count Line 57: self._max_workers = max(workers_count, max_tasks) * 3 max_tasks is expected to be in the order of the hundreds, while workers_count is expected to be in the order of the dozens. I'd just add a new parameter here, perhaps defaulting to None for backward compatibility (= unbounded). Line 58: self._worker_id = 0 Line 59: self._tasks = TaskQueue(max_tasks) Line 60: self._scheduler = scheduler Line 61: self._workers = set() PS2, Line 115: mark_as_replaced silly nit: set_replaced() ? PS2, Line 132: "worker thread recovered" I find this misleading, perhaps just the wording. Here you replaced the exiting worker thread with a fresh clone, so from the worker perspective there is no recovery, but a replacement. -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: Limit the number of workers in executor
gerrit-hooks has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 2: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6']) -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: Limit the number of workers in executor
gerrit-hooks has posted comments on this change. Change subject: virt: Limit the number of workers in executor .. Patch Set 1: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6']) -- To view, visit https://gerrit.ovirt.org/57754 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba56d91474c6b14a1cfe2db827b6fd61843a1db2 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
