Nir Soffer has posted comments on this change. Change subject: jobs: Autodelete ......................................................................
Patch Set 7: Code-Review-1 (2 comments) https://gerrit.ovirt.org/#/c/62002/7/lib/vdsm/jobs.py File lib/vdsm/jobs.py: Line 117 Line 118 Line 119 Line 120 Line 121 > Please add a comment explaining why we *must* not delete the job if _abort( We have major error here - switching state to ABORTED before the job was aborted. If _abort() fails, we already switch states, and we cannot abort again (in the next patch we fixed state switching to prevent that). Even worse, when engine will check job status, it will be ABORTED, while we are still accessing storage. This is the worst bug we can have. There is also smaller issue, if _abort is not implemented by a subclass, it will raise, but the job will be in ABORTED state. I think we should change this to: def abort(self): logging.info('Job %r aborting...', self._id) self._abort() self._status = STATUS.ABORTED if self.autodelete: self._autodelete() If abort failed, we are still in running state, and engine can try again. If engine cannot abort a job, it can fence vdsm or the host, or just wait for the job. Line 135 Line 136 Line 137 Line 138 Line 139 We must document here that: - _abort must raise if the job could not be aborted - _abort must not raise if the job was aborted (so the job would not leak) -- To view, visit https://gerrit.ovirt.org/62002 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4aafd2271397bd9bc4289dc1aab723398d475add Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
