Change in vdsm[master]: jobs: Guard against racy state changes
gerrit-hooks has posted comments on this change. Change subject: jobs: Guard against racy state changes .. Patch Set 8: * Update tracker: IGNORE, no Bug-Url found * Set MODIFIED::IGNORE, no Bug-Url found. -- To view, visit https://gerrit.ovirt.org/63712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
Change in vdsm[master]: jobs: Guard against racy state changes
Nir Soffer has posted comments on this change. Change subject: jobs: Guard against racy state changes .. Patch Set 7: Verified+1 Was verified before last changes, which was test fix, and tests pass now. -- To view, visit https://gerrit.ovirt.org/63712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
Change in vdsm[master]: jobs: Guard against racy state changes
Nir Soffer has submitted this change and it was merged. Change subject: jobs: Guard against racy state changes .. jobs: Guard against racy state changes When performing test and set operations with the job status we must use a lock to prevent multiple threads from conflicting with each other. This is most important when aborting or running a job. Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Signed-off-by: Adam Litke Reviewed-on: https://gerrit.ovirt.org/63712 Continuous-Integration: Jenkins CI Reviewed-by: Nir Soffer Tested-by: Nir Soffer --- M lib/vdsm/jobs.py M tests/jobsTests.py 2 files changed, 50 insertions(+), 16 deletions(-) Approvals: Nir Soffer: Verified; Looks good to me, approved Jenkins CI: Passed CI tests -- To view, visit https://gerrit.ovirt.org/63712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: merged Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: gerrit-hooks ___ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
Change in vdsm[master]: jobs: Guard against racy state changes
Nir Soffer has posted comments on this change. Change subject: jobs: Guard against racy state changes .. Patch Set 7: Code-Review+2 -- To view, visit https://gerrit.ovirt.org/63712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
Change in vdsm[master]: jobs: Guard against racy state changes
Adam Litke has posted comments on this change. Change subject: jobs: Guard against racy state changes .. Patch Set 7: Resolved failing test_abort_running_job() test by adding another event so that we actually wait for the thread to begin running the job before aborting it. -- To view, visit https://gerrit.ovirt.org/63712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
Change in vdsm[master]: jobs: Guard against racy state changes
gerrit-hooks has posted comments on this change. Change subject: jobs: Guard against racy state changes .. Patch Set 7: * 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/63712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
Change in vdsm[master]: jobs: Guard against racy state changes
Nir Soffer has posted comments on this change. Change subject: jobs: Guard against racy state changes .. Patch Set 6: Code-Review-1 Please check this failing test: 13:17:35 == 13:17:35 FAIL: test_abort_running_job (jobsTests.JobsTests) 13:17:35 -- 13:17:35 Traceback (most recent call last): 13:17:35 File "/home/jenkins/workspace/vdsm_master_check-patch-el7-x86_64/vdsm/tests/jobsTests.py", line 203, in test_abort_running_job 13:17:35 self.assertEqual(jobs.STATUS.RUNNING, job.status) 13:17:35 AssertionError: 'running' != 'pending' -- To view, visit https://gerrit.ovirt.org/63712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski 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]: jobs: Guard against racy state changes
gerrit-hooks has posted comments on this change. Change subject: jobs: Guard against racy state changes .. Patch Set 6: * 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/63712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski 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]: jobs: Guard against racy state changes
Nir Soffer has posted comments on this change. Change subject: jobs: Guard against racy state changes .. Patch Set 5: Code-Review+2 -- To view, visit https://gerrit.ovirt.org/63712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski 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]: jobs: Guard against racy state changes
Piotr Kliczewski has posted comments on this change. Change subject: jobs: Guard against racy state changes .. Patch Set 5: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/63712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski 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]: jobs: Guard against racy state changes
Nir Soffer has posted comments on this change. Change subject: jobs: Guard against racy state changes .. Patch Set 5: (1 comment) https://gerrit.ovirt.org/#/c/63712/5/lib/vdsm/jobs.py File lib/vdsm/jobs.py: Line 162 Line 163 Line 164 Line 165 Line 166 Please add a comment that this runs with the jobs status lock taken, and the job must not call anything that try to take this lock. -- To view, visit https://gerrit.ovirt.org/63712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski 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]: jobs: Guard against racy state changes
Nir Soffer has posted comments on this change.
Change subject: jobs: Guard against racy state changes
..
Patch Set 5:
(3 comments)
https://gerrit.ovirt.org/#/c/63712/5/tests/jobsTests.py
File tests/jobsTests.py:
Line 77: class StuckJob(TestingJob):
Line 78:
Line 79: def __init__(self):
Line 80: TestingJob.__init__(self)
Line 81: self.event = threading.Event()
To fix racy tests, we do do:
self.ready = threading.Event()
Line 82:
Line 83: def _run(self):
Line 84: self.event.wait(1)
Line 85:
Line 79: def __init__(self):
Line 80: TestingJob.__init__(self)
Line 81: self.event = threading.Event()
Line 82:
Line 83: def _run(self):
Signal waiting test that we are running:
self.ready.set()
Line 84: self.event.wait(1)
Line 85:
Line 86: def _abort(self):
Line 87: self.event.set()
Line 198:
Line 199: def test_abort_running_job(self):
Line 200: job = StuckJob()
Line 201: jobs.add(job)
Line 202: t = start_thread(job.run)
We need here:
if not job.ready.wait(1):
raise RuntimeError("Timeout waiting for job thread")
Line 203: self.assertEqual(jobs.STATUS.RUNNING, job.status)
Line 204: jobs.abort(job.id)
Line 205: t.join()
Line 206: self.assertEqual(jobs.STATUS.ABORTED, job.status)
--
To view, visit https://gerrit.ovirt.org/63712
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Piotr Kliczewski
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]: jobs: Guard against racy state changes
Nir Soffer has posted comments on this change. Change subject: jobs: Guard against racy state changes .. Patch Set 5: Code-Review+1 (1 comment) I think this is good enough for merging, we can improve locking to avoid holding the lock while aborting, see comments in version 4. https://gerrit.ovirt.org/#/c/63712/5/tests/jobsTests.py File tests/jobsTests.py: Line 199: def test_abort_running_job(self): Line 200: job = StuckJob() Line 201: jobs.add(job) Line 202: t = start_thread(job.run) Line 203: self.assertEqual(jobs.STATUS.RUNNING, job.status) This is racy, may fail if this thread runs before the job thread. To avoid this race we need an event in set in run, and wait on it here. Line 204: jobs.abort(job.id) Line 205: t.join() Line 206: self.assertEqual(jobs.STATUS.ABORTED, job.status) Line 207: -- To view, visit https://gerrit.ovirt.org/63712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski 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]: jobs: Guard against racy state changes
Nir Soffer has posted comments on this change.
Change subject: jobs: Guard against racy state changes
..
Patch Set 4:
(2 comments)
https://gerrit.ovirt.org/#/c/63712/4/lib/vdsm/jobs.py
File lib/vdsm/jobs.py:
Line 130: with self._status_lock:
Line 131: if not self.active:
Line 132: raise JobNotActive()
Line 133: logging.info('Job %r aborting...', self._id)
Line 134: self._abort()
You want to hold the lock while calling _abort()?
This can take a very long time, not a good idea, and can lead to deadlock, if
some code in _abort call something that try to take the status lock.
The status lock should be held only for very short time.
We can avoid this by adding ABORTING state - if run finish in this state it can
bail out.
Line 135: self._status = STATUS.ABORTED
Line 136: if self.autodelete:
Line 137: self._autodelete()
Line 138:
Line 156: e = exception.GeneralException(str(e))
Line 157: self._error = e
Line 158: finally:
Line 159: with self._status_lock:
Line 160: if self.status == STATUS.ABORTED:
If we add ABORTING state, we can do here:
if self.status in (ABORTING, ABORTED):
return
Line 161: return
Line 162: self._status = status
Line 163: if self.autodelete:
Line 164: self._autodelete()
--
To view, visit https://gerrit.ovirt.org/63712
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Piotr Kliczewski
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]: jobs: Guard against racy state changes
Adam Litke has posted comments on this change. Change subject: jobs: Guard against racy state changes .. Patch Set 5: Verified+1 -- To view, visit https://gerrit.ovirt.org/63712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski 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]: jobs: Guard against racy state changes
gerrit-hooks has posted comments on this change. Change subject: jobs: Guard against racy state changes .. Patch Set 5: * 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/63712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski 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]: jobs: Guard against racy state changes
gerrit-hooks has posted comments on this change. Change subject: jobs: Guard against racy state changes .. Patch Set 4: * 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/63712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski 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]: jobs: Guard against racy state changes
Adam Litke has posted comments on this change. Change subject: jobs: Guard against racy state changes .. Patch Set 2: (1 comment) https://gerrit.ovirt.org/#/c/63712/2/lib/vdsm/jobs.py File lib/vdsm/jobs.py: Line 37: RUNNING = 'running' # Job is running Line 38: DONE = 'done'# Job has finished successfully Line 39: ABORTED = 'aborted' # Job was aborted by user request Line 40: FAILED = 'failed'# Job has failed Line 41: CLEARED = 'cleared' # Job is being deleted > Lets chose one term - cleared or deleted? Done Line 42: Line 43: Line 44: class ClientError(Exception): Line 45: ''' Base class for client error ''' -- To view, visit https://gerrit.ovirt.org/63712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski 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]: jobs: Guard against racy state changes
Adam Litke has posted comments on this change. Change subject: jobs: Guard against racy state changes .. Patch Set 3: Verified+1 Functional tests and copy_data verb -- To view, visit https://gerrit.ovirt.org/63712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski 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]: jobs: Guard against racy state changes
gerrit-hooks has posted comments on this change. Change subject: jobs: Guard against racy state changes .. Patch Set 3: * 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/63712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski 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]: jobs: Guard against racy state changes
Nir Soffer has posted comments on this change.
Change subject: jobs: Guard against racy state changes
..
Patch Set 2:
(2 comments)
Partial review
https://gerrit.ovirt.org/#/c/63712/1/lib/vdsm/jobs.py
File lib/vdsm/jobs.py:
Line 147:(self._id, self.status))
Line 148: self._status = STATUS.RUNNING
Line 149: try:
Line 150: self._run()
Line 151: status = STATUS.DONE
> Throughout the module I am using the property (status) for reading and the
I would use always _status in this module, leaving status for external use, but
better keep it consistent as you suggest in this patch.
Line 152: except Exception as e:
Line 153: status = STATUS.FAILED
Line 154: logging.exception("Job (id=%s desc=%s) failed",
Line 155: self.id, self.description)
https://gerrit.ovirt.org/#/c/63712/2/lib/vdsm/jobs.py
File lib/vdsm/jobs.py:
Line 37: RUNNING = 'running' # Job is running
Line 38: DONE = 'done'# Job has finished successfully
Line 39: ABORTED = 'aborted' # Job was aborted by user request
Line 40: FAILED = 'failed'# Job has failed
Line 41: CLEARED = 'cleared' # Job is being deleted
Lets chose one term - cleared or deleted?
Since we use delete and autodelete, I think we should use DELETED.
Line 42:
Line 43:
Line 44: class ClientError(Exception):
Line 45: ''' Base class for client error '''
--
To view, visit https://gerrit.ovirt.org/63712
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Piotr Kliczewski
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]: jobs: Guard against racy state changes
gerrit-hooks has posted comments on this change. Change subject: jobs: Guard against racy state changes .. 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', 'ovirt-4.0']) -- To view, visit https://gerrit.ovirt.org/63712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski 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]: jobs: Guard against racy state changes
Adam Litke has posted comments on this change. Change subject: jobs: Guard against racy state changes .. Patch Set 1: (7 comments) https://gerrit.ovirt.org/#/c/63712/1/lib/vdsm/jobs.py File lib/vdsm/jobs.py: Line 154 Line 155 Line 156 Line 157 Line 158 > Before we set status here and in the else path, we should check if the job Done Line 158 Line 159 Line 160 Line 161 Line 162 > Here we can set the status safely: I like using the finally clause for this. Will adopt it in this patch. Line 179 Line 180 Line 181 Line 182 Line 183 > To avoid races between jobs.stop() and _delete() called from autodelete, we Implemented a variation of this idea. Please check. PS1, Line 1: 2015 > please update Done Line 146: if self.autodelete: Line 147: self._autodelete() Line 148: Line 149: def run(self): Line 150: with self._status_lock: > If job was aborted we should not fail. I think this will ensure this (needs Done in previous patch and test added. Line 151: if self.status != STATUS.PENDING: Line 152: raise CannotRunJob() Line 153: self._status = STATUS.RUNNING Line 154: try: PS1, Line 151: status > please change to _status Throughout the module I am using the property (status) for reading and the underlying variable (_status) when writing. This patch maintains that convention. PS1, Line 162: self._status = STATUS.FAILED : else: : self._status = STATUS.DONE > if we want to protect status field we need to lock here as well. Done -- To view, visit https://gerrit.ovirt.org/63712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski 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]: jobs: Guard against racy state changes
Piotr Kliczewski has posted comments on this change. Change subject: jobs: Guard against racy state changes .. Patch Set 1: Code-Review-1 (3 comments) https://gerrit.ovirt.org/#/c/63712/1/lib/vdsm/jobs.py File lib/vdsm/jobs.py: PS1, Line 1: 2015 please update PS1, Line 151: status please change to _status PS1, Line 162: self._status = STATUS.FAILED : else: : self._status = STATUS.DONE if we want to protect status field we need to lock here as well. -- To view, visit https://gerrit.ovirt.org/63712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski 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]: jobs: Guard against racy state changes
Nir Soffer has posted comments on this change. Change subject: jobs: Guard against racy state changes .. Patch Set 1: Code-Review-1 -- To view, visit https://gerrit.ovirt.org/63712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Jenkins CI 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]: jobs: Guard against racy state changes
Nir Soffer has posted comments on this change. Change subject: jobs: Guard against racy state changes .. Patch Set 1: (5 comments) https://gerrit.ovirt.org/#/c/63712/1/lib/vdsm/jobs.py File lib/vdsm/jobs.py: Line 154 Line 155 Line 156 Line 157 Line 158 Before we set status here and in the else path, we should check if the job was aborted. If we aborted, we can return, since abort() does everything needed. We can build the status here: status = STATUS.FAILED else: status = STATUS.DONE Line 158 Line 159 Line 160 Line 161 Line 162 Here we can set the status safely: with self._status_lock: if self._status == STATUS.ABORTED: return self._status = status Line 179 Line 180 Line 181 Line 182 Line 183 To avoid races between jobs.stop() and _delete() called from autodelete, we can add a new state DELETED: with self._status_lock: if self._status == STATUS.DELETED: return self._status = STATUS.DELETED And, call _delete from jobs.stop(), before clearing _jobs. Line 146: if self.autodelete: Line 147: self._autodelete() Line 148: Line 149: def run(self): Line 150: with self._status_lock: If job was aborted we should not fail. I think this will ensure this (needs a test): if self._status = STATUS.ABORTED: return Line 151: if self.status != STATUS.PENDING: Line 152: raise CannotRunJob() Line 153: self._status = STATUS.RUNNING Line 154: try: Line 147: self._autodelete() Line 148: Line 149: def run(self): Line 150: with self._status_lock: Line 151: if self.status != STATUS.PENDING: Use self._status for consistency Line 152: raise CannotRunJob() Line 153: self._status = STATUS.RUNNING Line 154: try: Line 155: self._run() -- To view, visit https://gerrit.ovirt.org/63712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Jenkins CI 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]: jobs: Guard against racy state changes
gerrit-hooks has posted comments on this change. Change subject: jobs: Guard against racy state changes .. 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', 'ovirt-4.0']) -- To view, visit https://gerrit.ovirt.org/63712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke 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]: jobs: Guard against racy state changes
Adam Litke has uploaded a new change for review.
Change subject: jobs: Guard against racy state changes
..
jobs: Guard against racy state changes
When performing test and set operations with the job status we must use
a lock to prevent multiple threads from conflicting with each other.
Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b
Signed-off-by: Adam Litke
---
M lib/vdsm/jobs.py
1 file changed, 9 insertions(+), 6 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/12/63712/1
diff --git a/lib/vdsm/jobs.py b/lib/vdsm/jobs.py
index ba71008..1f80cc2 100644
--- a/lib/vdsm/jobs.py
+++ b/lib/vdsm/jobs.py
@@ -91,6 +91,7 @@
self._id = job_id
self._status = STATUS.PENDING
self._description = description
+self._status_lock = threading.Lock()
self._error = None
@property
@@ -136,18 +137,20 @@
return self.status in (STATUS.PENDING, STATUS.RUNNING)
def abort(self):
-if not self.active:
-raise CannotAbortJob()
-self._status = STATUS.ABORTED
+with self._status_lock:
+if not self.active:
+raise CannotAbortJob()
+self._status = STATUS.ABORTED
logging.info('Job %r aborting...', self._id)
self._abort()
if self.autodelete:
self._autodelete()
def run(self):
-if self.status != STATUS.PENDING:
-raise CannotRunJob()
-self._status = STATUS.RUNNING
+with self._status_lock:
+if self.status != STATUS.PENDING:
+raise CannotRunJob()
+self._status = STATUS.RUNNING
try:
self._run()
except Exception as e:
--
To view, visit https://gerrit.ovirt.org/63712
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5366fcb962e9d34da6fca47b5171a6b132cf2e3b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
