Nir Soffer has posted comments on this change.
Change subject: jobs: Fix abort semantics ......................................................................
Patch Set 1: Code-Review-1
(8 comments)
https://gerrit.ovirt.org/#/c/65102/1/lib/vdsm/jobs.py File lib/vdsm/jobs.py:
Line 146 Line 147 Line 148 Line 149 Line 150 Note to self: status may change at this point to ABORTING
Line 169 Line 170 Line 171 Line 172 Line 173 These do not matter much now, maybe change Must to Should.
Line 136: elif self.status == STATUS.RUNNING: Line 137: self._status = STATUS.ABORTING Line 138: logging.info("Aborting job %r...", self.id) Line 139: self._abort() Line 140: # Autodelete is handled by run when the operation is terminated This comment is relevant for both running and aborting state, maybe move to the pending section, or add note on the top of the function. Line 141: elif self.status == STATUS.ABORTING: Line 142: logging.info("Retrying abort job %r...", self.id) Line 143: self._abort() Line 144: else:
Line 158: except Exception as e: Line 159: with self._status_lock: Line 160: if self.status == STATUS.ABORTING: Line 161: logging.exception("Failed to abort job (id=%s desc=%s)", Line 162: self.id, self.description) Why this is failure to abort?
If run raised, it may be because an opeation was aborted (raised ActionStopped).
I think this means that user requested an abort, so we don't care if the job failed or aborted, and we should change state to ABORTED. Line 163: # We do not autodelete failed aborting jobs because it is Line 164: # likely that something is still running on the system. Line 165: else: Line 166: self._status = STATUS.FAILED
Line 166: self._status = STATUS.FAILED Line 167: logging.exception("Job (id=%s desc=%s) failed", Line 168: self.id, self.description) Line 169: if self.autodelete: Line 170: self._autodelete() We need autodelete both for job errors and job success, better move it down to the finally block. Line 171: if not isinstance(e, exception.VdsmException): Line 172: e = exception.GeneralException(str(e)) Line 173: self._error = e Line 174: else:
Line 169: if self.autodelete: Line 170: self._autodelete() Line 171: if not isinstance(e, exception.VdsmException): Line 172: e = exception.GeneralException(str(e)) Line 173: self._error = e We can wrap the exception first, regardless of the status. Line 174: else: Line 175: with self._status_lock: Line 176: if self.status == STATUS.ABORTING: Line 177: self._status = STATUS.ABORTED
Line 175: with self._status_lock: Line 176: if self.status == STATUS.ABORTING: Line 177: self._status = STATUS.ABORTED Line 178: else: Line 179: self._status = STATUS.DONE Checking aborting state twice is little ugly. Maybe we can move this to the common section?
something like:
try: run... except: wrap error finally: take lock if status is aborting: set status to aborted eif error: set status to failed set error else: self status to done if autodelete: autodelete... Line 180: if self.autodelete: Line 181: self._autodelete() Line 182: Line 183: def _abort(self):
Line 177: self._status = STATUS.ABORTED Line 178: else: Line 179: self._status = STATUS.DONE Line 180: if self.autodelete: Line 181: self._autodelete() We want to do autodelete in a finally block. Line 182: Line 183: def _abort(self): Line 184: """ Line 185: May be implemented by child class. This is an asynchronous operation