Adam Litke has posted comments on this change.
Change subject: jobs: Fix abort semantics ......................................................................
Patch Set 3:
(7 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?
Done
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
Done
Line 152: return Line 153: try: Line 154: self._run() Line 155: except exception.ActionStopped: Line 156: self._handle_completed_abort()
Maybe _abort_completed?
Done 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?
Done 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 pr
Done 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!
Thanks! 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?
Done 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