Nir Soffer has posted comments on this change.
Change subject: jobs: Fix abort semantics ......................................................................
Patch Set 3: Code-Review+1
(7 comments)
Nice and simple!
Please check the comments.
https://gerrit.ovirt.org/#/c/65102/3/lib/vdsm/jobs.py File lib/vdsm/jobs.py:
Line 168 Line 169 Line 170 Line 171 Line 172 Should raise?
Line 169 Line 170 Line 171 Line 172 Line 173 We must remove this line, _abort cannot know if the job was aborted, it is async.
Line 152: return Line 153: try: Line 154: self._run() Line 155: except exception.ActionStopped: Line 156: self._handle_completed_abort() Maybe _abort_completed? Line 157: except Exception as e: Line 158: self._handle_execution_error(e) Line 159: else: Line 160: # We always mark the job done. Even if we were in aborting state
Line 154: self._run() Line 155: except exception.ActionStopped: Line 156: self._handle_completed_abort() Line 157: except Exception as e: Line 158: self._handle_execution_error(e) Maybe _run_failed? Line 159: else: Line 160: # We always mark the job done. Even if we were in aborting state Line 161: # _run might finish normally before the abort action executes. Line 162: with self._status_lock:
Line 157: except Exception as e: Line 158: self._handle_execution_error(e) Line 159: else: Line 160: # We always mark the job done. Even if we were in aborting state Line 161: # _run might finish normally before the abort action executes. I think this comment is confusing, setting status to DONE in the else is pretty clear to me. Line 162: with self._status_lock: Line 163: self._status = STATUS.DONE Line 164: finally: Line 165: if self.autodelete:
Line 162: with self._status_lock: Line 163: self._status = STATUS.DONE Line 164: finally: Line 165: if self.autodelete: Line 166: self._autodelete() Cannot be more elegant! Line 167: Line 168: def _prepare_to_run(self): Line 169: with self._status_lock: Line 170: if self.status == STATUS.ABORTED:
Line 164: finally: Line 165: if self.autodelete: Line 166: self._autodelete() Line 167: Line 168: def _prepare_to_run(self): May be _may_run? Line 169: with self._status_lock: Line 170: if self.status == STATUS.ABORTED: Line 171: logging.debug('Refusing to run aborted job %r', self._id) Line 172: return False