Change in vdsm[master]: jobs: Allow run and abort only from valid states

[automation]

gerrit-hooks has posted comments on this change.

Change subject: jobs: Allow run and abort only from valid states
..


Patch Set 6:

* Update tracker: IGNORE, no Bug-Url found
* Set MODIFIED::IGNORE, no Bug-Url found.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[nsoffer]

Nir Soffer has submitted this change and it was merged.

Change subject: jobs: Allow run and abort only from valid states
..


jobs: Allow run and abort only from valid states

Before this patch we allow abort and run from any state even when it
does not make sense.  Abort is valid only for pending or running jobs
and run is valid only for pending jobs.  When an invalid call is
attempted raise a new error.

Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Signed-off-by: Adam Litke 
Reviewed-on: https://gerrit.ovirt.org/63711
Reviewed-by: Francesco Romani 
Reviewed-by: Piotr Kliczewski 
Reviewed-by: Nir Soffer 
Continuous-Integration: Jenkins CI
---
M lib/vdsm/define.py
M lib/vdsm/exception.py
M lib/vdsm/jobs.py
M tests/jobsTests.py
4 files changed, 47 insertions(+), 4 deletions(-)

Approvals:
  Piotr Kliczewski: Looks good to me, but someone else must approve
  Nir Soffer: Looks good to me, approved
  Adam Litke: Verified
  Jenkins CI: Passed CI tests
  Francesco Romani: Looks good to me, but someone else must approve



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[automation]

gerrit-hooks has posted comments on this change.

Change subject: jobs: Allow run and abort only from valid states
..


Patch Set 5:

* 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/63711
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: jobs: Allow run and abort only from valid states
..


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[piotr . kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: jobs: Allow run and abort only from valid states
..


Patch Set 4: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[fromani]

Francesco Romani has posted comments on this change.

Change subject: jobs: Allow run and abort only from valid states
..


Patch Set 4: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: jobs: Allow run and abort only from valid states
..


Patch Set 4: Code-Review+1

Waiting for more reviews.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[alitke]

Adam Litke has posted comments on this change.

Change subject: jobs: Allow run and abort only from valid states
..


Patch Set 4: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[automation]

gerrit-hooks has posted comments on this change.

Change subject: jobs: Allow run and abort only from valid states
..


Patch Set 4:

* 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/63711
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[alitke]

Adam Litke has posted comments on this change.

Change subject: jobs: Allow run and abort only from valid states
..


Patch Set 3:

(1 comment)

https://gerrit.ovirt.org/#/c/63711/3/lib/vdsm/define.py
File lib/vdsm/define.py:

Line 87: 'migNotInProgress': exception.MigrationNotInProgress().response(),
Line 88: 'migrateLimit': exception.MigrationLimitExceeded().response(),
Line 89: 'recovery': exception.RecoveryInProgress().response(),
Line 90: 'hostdevDetachErr': exception.HostdevDetachFailed().response(),
Line 91: 'JobNotActive': exception.JobNotActive().response(),
> Why not near the other job errors?
To preserve ordering.  I don't mind moving it.
Line 92: }
Line 93: 
Line 94: 
Line 95: doneCode = {'code': 0, 'message': 'Done'}


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: jobs: Allow run and abort only from valid states
..


Patch Set 3:

(1 comment)

https://gerrit.ovirt.org/#/c/63711/3/lib/vdsm/define.py
File lib/vdsm/define.py:

Line 87: 'migNotInProgress': exception.MigrationNotInProgress().response(),
Line 88: 'migrateLimit': exception.MigrationLimitExceeded().response(),
Line 89: 'recovery': exception.RecoveryInProgress().response(),
Line 90: 'hostdevDetachErr': exception.HostdevDetachFailed().response(),
Line 91: 'JobNotActive': exception.JobNotActive().response(),
Why not near the other job errors?
Line 92: }
Line 93: 
Line 94: 
Line 95: doneCode = {'code': 0, 'message': 'Done'}


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[automation]

gerrit-hooks has posted comments on this change.

Change subject: jobs: Allow run and abort only from valid states
..


Patch Set 3:

* 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/63711
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: jobs: Allow run and abort only from valid states
..


Patch Set 2:

(1 comment)

https://gerrit.ovirt.org/#/c/63711/2/lib/vdsm/jobs.py
File lib/vdsm/jobs.py:

Line 126: return self.status in (STATUS.PENDING, STATUS.RUNNING)
Line 127: 
Line 128: def abort(self):
Line 129: if not self.active:
Line 130: raise JobNotActive()
> If _abort() failed in a previous attempt, we change the state to ABORTED, s
This should not be a problem if we fixed state changing, see comment in 
previous patch.
Line 131: self._status = STATUS.ABORTED
Line 132: logging.info('Job %r aborting...', self._id)
Line 133: self._abort()
Line 134: if self.autodelete:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[alitke]

Adam Litke has posted comments on this change.

Change subject: jobs: Allow run and abort only from valid states
..


Patch Set 2: Verified+1

Functional tests and copy_data verb

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: jobs: Allow run and abort only from valid states
..


Patch Set 2:

(1 comment)

https://gerrit.ovirt.org/#/c/63711/2/lib/vdsm/jobs.py
File lib/vdsm/jobs.py:

Line 126: return self.status in (STATUS.PENDING, STATUS.RUNNING)
Line 127: 
Line 128: def abort(self):
Line 129: if not self.active:
Line 130: raise JobNotActive()
If _abort() failed in a previous attempt, we change the state to ABORTED, so 
this check will skip the next abort.

I don't see a race free solution, and it will probably never happen, maybe 
document this issue here.
Line 131: self._status = STATUS.ABORTED
Line 132: logging.info('Job %r aborting...', self._id)
Line 133: self._abort()
Line 134: if self.autodelete:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: jobs: Allow run and abort only from valid states
..


Patch Set 2: Code-Review+1

Looks good, waiting for more reviews.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[automation]

gerrit-hooks has posted comments on this change.

Change subject: jobs: Allow run and abort only from valid states
..


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/63711
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[alitke]

Adam Litke has posted comments on this change.

Change subject: jobs: Allow run and abort only from valid states
..


Patch Set 1:

(3 comments)

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

Line 69: ''' Only pending jobs may be run '''
Line 70: name = 'CannotRunJob'
Line 71: 
Line 72: 
Line 73: class JobNotRunnable(ClientError):
> Leftover? I don't see any use.
oops.  Removing.
Line 74: ''' Job cannot be run from its current state '''
Line 75: name = 'JobNotRunnable'
Line 76: 
Line 77: 


Line 136: return self.status in (STATUS.PENDING, STATUS.RUNNING)
Line 137: 
Line 138: def abort(self):
Line 139: if not self.active:
Line 140: raise CannotAbortJob()
> CannotAbortJob means that job was in a state which can be aborted, but the 
Yeah agreed.  I was debating about going this route but your comments seal it.
Line 141: self._status = STATUS.ABORTED
Line 142: logging.info('Job %r aborting...', self._id)
Line 143: self._abort()
Line 144: if self.autodelete:


Line 145: self._autodelete()
Line 146: 
Line 147: def run(self):
Line 148: if self.status != STATUS.PENDING:
Line 149: raise CannotRunJob()
> We have two case here:
Done
Line 150: self._status = STATUS.RUNNING
Line 151: try:
Line 152: self._run()
Line 153: except Exception as e:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[piotr . kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: jobs: Allow run and abort only from valid states
..


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/63711/1/lib/vdsm/exception.py
File lib/vdsm/exception.py:

Line 345: class HostdevDetachFailed(VdsmException):
Line 346: code = 83
Line 347: message = 'Could not detach host device'
Line 348: 
Line 349: 
> After we finish with this, the new error codes should be added to engine. T
I see in the engine codes up to 77. There is bunch of missing codes which were 
already merged. I just want to make sure that those codes are properly handled.
Line 350: class CannotAbortJob(VdsmException):
Line 351: code = 84
Line 352: message = 'Job cannot be aborted from this state'
Line 353: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: jobs: Allow run and abort only from valid states
..


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/63711/1/lib/vdsm/exception.py
File lib/vdsm/exception.py:

Line 345: class HostdevDetachFailed(VdsmException):
Line 346: code = 83
Line 347: message = 'Could not detach host device'
Line 348: 
Line 349: 
> It looks like those codes are unknown to the engine and would be handled as
After we finish with this, the new error codes should be added to engine. There 
is no point in adding them at this point since we did not finish the review.
Line 350: class CannotAbortJob(VdsmException):
Line 351: code = 84
Line 352: message = 'Job cannot be aborted from this state'
Line 353: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[piotr . kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: jobs: Allow run and abort only from valid states
..


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/63711/1/lib/vdsm/exception.py
File lib/vdsm/exception.py:

Line 345: class HostdevDetachFailed(VdsmException):
Line 346: code = 83
Line 347: message = 'Could not detach host device'
Line 348: 
Line 349: 
It looks like those codes are unknown to the engine and would be handled as 
generic errors.

Is there any plan to update the engine?
Line 350: class CannotAbortJob(VdsmException):
Line 351: code = 84
Line 352: message = 'Job cannot be aborted from this state'
Line 353: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[fromani]

Francesco Romani has posted comments on this change.

Change subject: jobs: Allow run and abort only from valid states
..


Patch Set 1: -Code-Review

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[fromani]

Francesco Romani has posted comments on this change.

Change subject: jobs: Allow run and abort only from valid states
..


Patch Set 1: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: jobs: Allow run and abort only from valid states
..


Patch Set 1: Code-Review-1

(3 comments)

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

Line 69: ''' Only pending jobs may be run '''
Line 70: name = 'CannotRunJob'
Line 71: 
Line 72: 
Line 73: class JobNotRunnable(ClientError):
Leftover? I don't see any use.
Line 74: ''' Job cannot be run from its current state '''
Line 75: name = 'JobNotRunnable'
Line 76: 
Line 77: 


Line 136: return self.status in (STATUS.PENDING, STATUS.RUNNING)
Line 137: 
Line 138: def abort(self):
Line 139: if not self.active:
Line 140: raise CannotAbortJob()
CannotAbortJob means that job was in a state which can be aborted, but the 
operation failed, while this job was simply finished.

Maybe JobNotActive? (similar to JobNotDone)

This  name make it more clear that there is no error on the side of the client 
or the server.
Line 141: self._status = STATUS.ABORTED
Line 142: logging.info('Job %r aborting...', self._id)
Line 143: self._abort()
Line 144: if self.autodelete:


Line 145: self._autodelete()
Line 146: 
Line 147: def run(self):
Line 148: if self.status != STATUS.PENDING:
Line 149: raise CannotRunJob()
We have two case here:

- job was aborted, there is no need to run it, and there is no error to raise.
- job was already finished - this a bug in the code, so we should raise 
AssertionError, or RuntimeError, not a ClientError. ClientError should be used 
when a user perform an invalid request.
Line 150: self._status = STATUS.RUNNING
Line 151: try:
Line 152: self._run()
Line 153: except Exception as e:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[automation]

gerrit-hooks has posted comments on this change.

Change subject: jobs: Allow run and abort only from valid states
..


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/63711
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: jobs: Allow run and abort only from valid states

[alitke]

Adam Litke has uploaded a new change for review.

Change subject: jobs: Allow run and abort only from valid states
..

jobs: Allow run and abort only from valid states

Before this patch we allow abort and run from any state even when it
does not make sense.  Abort is valid only for pending or running jobs
and run is valid only for pending jobs.  When an invalid call is
attempted raise a new error.

Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Signed-off-by: Adam Litke 
---
M lib/vdsm/define.py
M lib/vdsm/exception.py
M lib/vdsm/jobs.py
M tests/jobsTests.py
4 files changed, 55 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/11/63711/1

diff --git a/lib/vdsm/define.py b/lib/vdsm/define.py
index d75e919..3edb881 100644
--- a/lib/vdsm/define.py
+++ b/lib/vdsm/define.py
@@ -88,6 +88,8 @@
 'migrateLimit': exception.MigrationLimitExceeded().response(),
 'recovery': exception.RecoveryInProgress().response(),
 'hostdevDetachErr': exception.HostdevDetachFailed().response(),
+'CannotAbortJob': exception.CannotAbortJob().response(),
+'CannotRunJob': exception.CannotRunJob().response(),
 }
 
 
diff --git a/lib/vdsm/exception.py b/lib/vdsm/exception.py
index 40d13f5..671067a 100644
--- a/lib/vdsm/exception.py
+++ b/lib/vdsm/exception.py
@@ -347,6 +347,16 @@
 message = 'Could not detach host device'
 
 
+class CannotAbortJob(VdsmException):
+code = 84
+message = 'Job cannot be aborted from this state'
+
+
+class CannotRunJob(VdsmException):
+code = 85
+message = 'Job cannot be run from this state'
+
+
 class RecoveryInProgress(VdsmException):
 code = 99
 message = 'Recovering from crash or Initializing'
diff --git a/lib/vdsm/jobs.py b/lib/vdsm/jobs.py
index fb81bdd..ba71008 100644
--- a/lib/vdsm/jobs.py
+++ b/lib/vdsm/jobs.py
@@ -60,6 +60,21 @@
 name = 'JobNotDone'
 
 
+class CannotAbortJob(ClientError):
+''' Job is not running or pending '''
+name = 'CannotAbortJob'
+
+
+class CannotRunJob(ClientError):
+''' Only pending jobs may be run '''
+name = 'CannotRunJob'
+
+
+class JobNotRunnable(ClientError):
+''' Job cannot be run from its current state '''
+name = 'JobNotRunnable'
+
+
 class AbortNotSupported(ClientError):
 ''' This type of job does not support aborting '''
 name = 'AbortNotSupported'
@@ -121,7 +136,8 @@
 return self.status in (STATUS.PENDING, STATUS.RUNNING)
 
 def abort(self):
-# TODO: Don't abort if not pending and not running
+if not self.active:
+raise CannotAbortJob()
 self._status = STATUS.ABORTED
 logging.info('Job %r aborting...', self._id)
 self._abort()
@@ -129,7 +145,8 @@
 self._autodelete()
 
 def run(self):
-# TODO: Don't run if aborted or not pending
+if self.status != STATUS.PENDING:
+raise CannotRunJob()
 self._status = STATUS.RUNNING
 try:
 self._run()
diff --git a/tests/jobsTests.py b/tests/jobsTests.py
index 2cb578c..1f0aeed 100644
--- a/tests/jobsTests.py
+++ b/tests/jobsTests.py
@@ -173,12 +173,24 @@
 self.assertEqual({}, jobs.info(job_type=bar.job_type,
job_ids=[foo.id]))
 
-def test_abort_job(self):
-job = TestingJob()
+@permutations([[jobs.STATUS.PENDING], [jobs.STATUS.RUNNING]])
+def test_abort_job(self, status):
+job = TestingJob(status)
 jobs.add(job)
 jobs.abort(job.id)
 self.assertEqual(jobs.STATUS.ABORTED, job.status)
 self.assertTrue(job._aborted)
+
+@permutations([
+[jobs.STATUS.ABORTED, jobs.CannotAbortJob.name],
+[jobs.STATUS.DONE, jobs.CannotAbortJob.name],
+[jobs.STATUS.FAILED, jobs.CannotAbortJob.name]
+])
+def test_abort_from_invalid_state(self, status, err):
+job = TestingJob(status)
+jobs.add(job)
+res = jobs.abort(job.id)
+self.assertEqual(response.error(err), res)
 
 def test_abort_unknown_job(self):
 self.assertEqual(response.error(jobs.NoSuchJob.name),
@@ -265,6 +277,16 @@
 self.run_job(job)
 self.assertEqual(jobs.STATUS.DONE, job.status)
 
+@permutations([
+[jobs.STATUS.RUNNING],
+[jobs.STATUS.ABORTED],
+[jobs.STATUS.DONE],
+[jobs.STATUS.FAILED]
+])
+def test_run_from_invalid_state(self, state):
+job = TestingJob(state)
+self.assertRaises(jobs.CannotRunJob, job.run)
+
 def test_default_exception(self):
 message = "testing failure"
 job = TestingJob(exception=Exception(message))


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia8bc1ab264c5786b43fb409c3fab8913337778bf
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: