oVirt Jenkins CI Server has posted comments on this change.

Change subject: task: add the support for abortEvent ......................................................................

Patch Set 1:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11775/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12719/ : ABORTED

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12564/ : SUCCESS

oVirt Jenkins CI Server has posted comments on this change.

Change subject: task: add the support for abort_callback ......................................................................

Patch Set 2:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12826/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11878/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12669/ : SUCCESS

Francesco Romani has posted comments on this change.

Change subject: task: add the support for abort_callback ......................................................................

Patch Set 3:

(1 comment)

VERY preliminary review - good chance for me to learn more about the task framework

http://gerrit.ovirt.org/#/c/33689/3/vdsm/storage/task.py File vdsm/storage/task.py:

Line 1231: Line 1232: for callback_weakref in self.abort_weakrefs: Line 1233: callback = callback_weakref() Line 1234: Line 1235: if callback is None: Is this safe or there is a risk of race? (I don't remember the internals of weakref so well) Line 1236: continue Line 1237: Line 1238: try: Line 1239: callback()

Nir Soffer has posted comments on this change.

Change subject: task: add the support for abort_callback ......................................................................

Patch Set 4:

(6 comments)

http://gerrit.ovirt.org/#/c/33689/4/vdsm/storage/task.py File vdsm/storage/task.py:

Line 489: self.error = se.TaskAborted("Unknown error encountered") Line 490: Line 491: self.mng = None Line 492: self.abort_lock = threading.Lock() Line 493: self.abort_weakrefs = set() Both should be private. Line 494: self._aborting = False Line 495: self._forceAbort = False Line 496: self.ref = 0 Line 497:

Line 1210: self._decref() Line 1211: Line 1212: @contextmanager Line 1213: def abort_callback(self, callback): Line 1214: callback_weakref = weakref.ref(callback) This does not work with instance methods, forcing the client to do ugly hacks (see http://gerrit.ovirt.org/#/c/35436/3/vdsm/clientIF.py,cm).

Since the caller must write code to work with this callback system to be able to use weak reference, we can move the responsibility to the client, and keep here raw callbacks. Line 1215: Line 1216: with self.abort_lock: Line 1217: if self.aborting(): Line 1218: callback()

Line 1214: callback_weakref = weakref.ref(callback) Line 1215: Line 1216: with self.abort_lock: Line 1217: if self.aborting(): Line 1218: callback() This can lead to deadlock, if callback call code that calls abort_callback.

We can do:

with self.abort_lock: if self.aborting(): aborting = True else: aborting = False self.abort_weakrefs.add(callback_weakref)

if aborting: callback() Line 1219: else: Line 1220: self.abort_weakrefs.add(callback_weakref) Line 1221: Line 1222: del callback # release reference

Line 1219: else: Line 1220: self.abort_weakrefs.add(callback_weakref) Line 1221: Line 1222: del callback # release reference Line 1223: yield Don't we need try-finally here to make sure we discard the callbacks?

setup... try: yield finally: teardown... Line 1224: Line 1225: with self.abort_lock: Line 1226: self.abort_weakrefs.discard(callback_weakref) Line 1227:

Line 1224: Line 1225: with self.abort_lock: Line 1226: self.abort_weakrefs.discard(callback_weakref) Line 1227: Line 1228: def abort_execute(self): Should be private, and probably will be more clear as _execute_abort_callbacks() Line 1229: with self.abort_lock: Line 1230: self._aborting = True Line 1231: Line 1232: for callback_weakref in self.abort_weakrefs:

Line 1235: if callback is None: Line 1236: continue Line 1237: Line 1238: try: Line 1239: callback() Same issue calling callback while holding the lock. Why do we need to do this?

We can do:

with self.abort_lock: abort_weakrefs = list(self.abort_weakrefs)

for ref in abort_weakrefs: invoke callbacks... Line 1240: except Exception: Line 1241: self.log.exception('failure running abort callback') Line 1242: Line 1243: def aborting(self):

oVirt Jenkins CI Server has posted comments on this change.

Change subject: task: add the support for abort_callback ......................................................................

Patch Set 5:

Build Successful

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/16808/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/16980/ : SUCCESS

automation@ovirt.org has posted comments on this change.

Change subject: task: add the support for abort_callback ......................................................................

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.5', 'ovirt-3.4', 'ovirt-3.3'])

oVirt Jenkins CI Server has posted comments on this change.

Change subject: task: add the support for abort_callback ......................................................................

Patch Set 6:

Build Started (2/2) -> http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17077/

Nir Soffer has posted comments on this change.

Change subject: task: add the support for abort_callback ......................................................................

Patch Set 6: Code-Review+1

oVirt Jenkins CI Server has posted comments on this change.

Change subject: task: add the support for abort_callback ......................................................................

Patch Set 7:

Build Started (1/2) -> http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/18267/

oVirt Jenkins CI Server has posted comments on this change.

Change subject: task: add the support for abort_callback ......................................................................

Patch Set 7:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/18267/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/1497/ : 0

Dan Kenigsberg has posted comments on this change.

Change subject: task: add the support for abort_callback ......................................................................

Patch Set 8: Code-Review+2

Dan Kenigsberg has submitted this change and it was merged.

Change subject: task: add the support for abort_callback ......................................................................

task: add the support for abort_callback

This patch adds the support for defining a callback used to abort a long running task, e.g.:

long_operation = MyLongOperation()

def abort_long_operation(): long_operation.terminate()

# abort_callback registers the callback for the time # of execution of the code block with vars.task.abort_callback(abort_long_operation): long_operation.execute()

When another thread (e.g. an external xml-rpc request) tries to abort the task (calling "stop"), the registered callbacks will be called.

This patch defines the basic infrastructure to register abort functions for long running operations, the technology used to actually implement the interruption is left to the user.

Change-Id: Ib82289e28e5ad9ea142850c31ccff3366b8397dc Signed-off-by: Federico Simoncelli fsimonce@redhat.com Reviewed-on: https://gerrit.ovirt.org/33689 Reviewed-by: Dan Kenigsberg danken@redhat.com Continuous-Integration: Dan Kenigsberg danken@redhat.com Tested-by: Dan Kenigsberg danken@redhat.com --- M vdsm/storage/task.py 1 file changed, 33 insertions(+), 1 deletion(-)

Approvals: Dan Kenigsberg: Verified; Looks good to me, approved; Passed CI tests