Change in vdsm[master]: jobs: Add when_finished callback and wire up Job.abort

Adam Litke has uploaded a new change for review.

Change subject: jobs: Add when_finished callback and wire up Job.abort
......................................................................

jobs: Add when_finished callback and wire up Job.abort

Add a callback mechanism that will be triggered when a job is done, failed, or
aborted.  This will be used to auto-clear finished SDM jobs from the HostJobs
manager.  Since jobs.Job only implements the abort job lifecycle elements, this
patch adds support to jobs.Job for calling the callback after aborting.

Future patches will enable the callback for the SDM Job run lifecycle.

Change-Id: I73d4c167a4cf1f1da4c1e2629b7afa9c01d338fd
Signed-off-by: Adam Litke <alitke(a)redhat.com&gt;
---
M lib/vdsm/jobs.py
M tests/jobsTests.py
2 files changed, 16 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/54/61254/1

diff --git a/lib/vdsm/jobs.py b/lib/vdsm/jobs.py
index 232981c..a4d3948 100644
--- a/lib/vdsm/jobs.py
+++ b/lib/vdsm/jobs.py
@@ -19,6 +19,7 @@
 
 from __future__ import absolute_import
 
+import functools
 import logging
 import threading
 
@@ -70,6 +71,7 @@
         self._status = STATUS.PENDING
         self._description = description
         self._error = None
+        self._finished_cb = lambda: None
 
     @property
     def id(self):
@@ -117,6 +119,10 @@
         self._status = STATUS.ABORTED
         logging.info('Job %r aborting...', self._id)
         self._abort()
+        self._finished_cb()
+
+    def when_finished(self, fn, *args, **kwargs):
+        self._finished_cb = functools.partial(fn, *args, **kwargs)
 
     def _abort(self):
         """
diff --git a/tests/jobsTests.py b/tests/jobsTests.py
index cb1cb89..0b2b7a1 100644
--- a/tests/jobsTests.py
+++ b/tests/jobsTests.py
@@ -158,6 +158,16 @@
         self.assertEqual(response.error(jobs.AbortNotSupported.name),
                          jobs.abort(job.id))
 
+    def test_abort_finished_callback(self):
+        def cb():
+            called[0] = True
+
+        called = [False]
+        job = TestingJob()
+        job.when_finished(cb)
+        job.abort()
+        self.assertTrue(called[0])
+
     @permutations([
         [jobs.STATUS.PENDING, True],
         [jobs.STATUS.RUNNING, True],


-- 
To view, visit https://gerrit.ovirt.org/61254
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I73d4c167a4cf1f1da4c1e2629b7afa9c01d338fd
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke(a)redhat.com&gt;

gerrit-hooks has posted comments on this change.

Change subject: jobs: Add when_finished callback and wire up Job.abort
......................................................................


Patch Set 1:

* 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.6',
'ovirt-4.0'])

-- 
To view, visit https://gerrit.ovirt.org/61254
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I73d4c167a4cf1f1da4c1e2629b7afa9c01d338fd
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: jobs: Add when_finished callback and wire up Job.abort
......................................................................


Patch Set 1: Code-Review-1

(1 comment)

https://gerrit.ovirt.org/#/c/61254/1/lib/vdsm/jobs.py
File lib/vdsm/jobs.py:

Line 70:         self._id = job_id
Line 71:         self._status = STATUS.PENDING
Line 72:         self._description = description
Line 73:         self._error = None
Line 74:         self._finished_cb = lambda: None
I don't see a need for generic finished callback. Since we are adding autoclear only
to sdm jobs now, we should just schedule a jobs.delete(job_id) in sdm.base.Job.run.

We should create the most simple and direct code that does what we need now.
Line 75: 
Line 76:     @property
Line 77:     def id(self):
Line 78:         return self._id


-- 
To view, visit https://gerrit.ovirt.org/61254
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I73d4c167a4cf1f1da4c1e2629b7afa9c01d338fd
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Adam Litke has posted comments on this change.

Change subject: jobs: Add when_finished callback and wire up Job.abort
......................................................................


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/61254/1/lib/vdsm/jobs.py
File lib/vdsm/jobs.py:

Line 70:         self._id = job_id
Line 71:         self._status = STATUS.PENDING
Line 72:         self._description = description
Line 73:         self._error = None
Line 74:         self._finished_cb = lambda: None
>  I don't see a need for generic finished callback. Since we are
> adding autoc 
Ok.  I can make the autoclear functionality specific to sdm jobs.
Line 75: 
Line 76:     @property
Line 77:     def id(self):
Line 78:         return self._id


-- 
To view, visit https://gerrit.ovirt.org/61254
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I73d4c167a4cf1f1da4c1e2629b7afa9c01d338fd
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: jobs: Add when_finished callback and wire up abort

gerrit-hooks has posted comments on this change.

Change subject: jobs: Add when_finished callback and wire up abort
......................................................................


Patch Set 2:

* 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.6',
'ovirt-4.0'])

-- 
To view, visit https://gerrit.ovirt.org/61254
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I73d4c167a4cf1f1da4c1e2629b7afa9c01d338fd
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: jobs: Add when_finished callback and wire up abort

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: jobs: Add when_finished callback and wire up abort
......................................................................


Patch Set 2:

(2 comments)

https://gerrit.ovirt.org/#/c/61254/2/vdsm/storage/sdm/api/base.py
File vdsm/storage/sdm/api/base.py:

Line 55:         finally:
Line 56:             vars.job_id = None
Line 57: 
Line 58:     def abort(self):
Line 59:         super(Job, self).abort()
This style, calling parent implementation makes the code hard to understand and the code
fragile - anything you do in the super class may break the subclass or the other way
around.

Lets avoid it in new code.
Line 60:         self._finished_cb()
Line 61: 
Line 62:     def when_finished(self, fn, *args, **kwargs):


Line 59:         super(Job, self).abort()
Line 60:         self._finished_cb()
Line 61: 
Line 62:     def when_finished(self, fn, *args, **kwargs):
Line 63:         self._finished_cb = functools.partial(fn, *args, **kwargs)
This is not simple and direct code you mentioned in another patch.


-- 
To view, visit https://gerrit.ovirt.org/61254
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I73d4c167a4cf1f1da4c1e2629b7afa9c01d338fd
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: jobs: Add when_finished callback and wire up abort

gerrit-hooks has posted comments on this change.

Change subject: jobs: Add when_finished callback and wire up abort
......................................................................


Patch Set 2:

* Update tracker: IGNORE, no Bug-Url found

-- 
To view, visit https://gerrit.ovirt.org/61254
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I73d4c167a4cf1f1da4c1e2629b7afa9c01d338fd
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: jobs: Add when_finished callback and wire up abort

Adam Litke has abandoned this change.

Change subject: jobs: Add when_finished callback and wire up abort
......................................................................


Abandoned

Alternate solution merged.

-- 
To view, visit https://gerrit.ovirt.org/61254
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: I73d4c167a4cf1f1da4c1e2629b7afa9c01d338fd
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;