Change in vdsm[master]: jobs: Only report progress if set
Nir Soffer has submitted this change and it was merged. Change subject: jobs: Only report progress if set .. jobs: Only report progress if set Not all Jobs have deterministic progress and some jobs will complete so quickly that progress updates are not practical. If progress is not available do not report it. Consumers of the Job info could choose to render a non-deterministic progress indication (barber pole, or spinning indicator) instead of a regular progress bar. We expect all jobs to start without any progress to report since they may be pending (waiting for a worker thread) or initializing (taking locks). Jobs that can provide progress information will start to report later. Jobs that do not support progress need not implement anything as the default behavior of Job is to never report progress. Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b Signed-off-by: Adam Litke Reviewed-on: https://gerrit.ovirt.org/50355 Reviewed-by: Nir Soffer Tested-by: Nir Soffer Reviewed-by: Francesco Romani Reviewed-by: Piotr Kliczewski Continuous-Integration: Jenkins CI Reviewed-by: Shahar Havivi --- M lib/vdsm/jobs.py M tests/jobsTests.py 2 files changed, 20 insertions(+), 5 deletions(-) Approvals: Piotr Kliczewski: Looks good to me, but someone else must approve Nir Soffer: Verified; Looks good to me, approved Shahar Havivi: Looks good to me, but someone else must approve Jenkins CI: Passed CI tests Francesco Romani: Looks good to me, but someone else must approve -- To view, visit https://gerrit.ovirt.org/50355 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: merged Gerrit-Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: gerrit-hooks ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: jobs: Only report progress if set
gerrit-hooks has posted comments on this change. Change subject: jobs: Only report progress if set .. Patch Set 6: * Update tracker: IGNORE, no Bug-Url found * Set MODIFIED::IGNORE, no Bug-Url found. -- To view, visit https://gerrit.ovirt.org/50355 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: jobs: Only report progress if set
Shahar Havivi has posted comments on this change. Change subject: jobs: Only report progress if set .. Patch Set 5: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/50355 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: jobs: Only report progress if set
Piotr Kliczewski has posted comments on this change. Change subject: jobs: Only report progress if set .. Patch Set 5: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/50355 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: jobs: Only report progress if set
Nir Soffer has posted comments on this change. Change subject: jobs: Only report progress if set .. Patch Set 2: (1 comment) https://gerrit.ovirt.org/#/c/50355/2/lib/vdsm/jobs.py File lib/vdsm/jobs.py: Line 66: def __init__(self, job_id, description=''): Line 67: self._id = job_id Line 68: self._status = STATUS.RUNNING Line 69: self._description = description Line 70: self._progress = None > Done The schema changes are here: https://gerrit.ovirt.org/49450 Line 71: self._error = None Line 72: Line 73: @property Line 74: def id(self): -- To view, visit https://gerrit.ovirt.org/50355 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: jobs: Only report progress if set
Nir Soffer has posted comments on this change. Change subject: jobs: Only report progress if set .. Patch Set 5: Verified by running the tests (we have 96% coverage). -- To view, visit https://gerrit.ovirt.org/50355 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: jobs: Only report progress if set
Francesco Romani has posted comments on this change. Change subject: jobs: Only report progress if set .. Patch Set 5: Here and in an handful of related patches I see V+1 without futher explanation. I trust both of you about this, but please add a few lines documenting how the verification is made, for future reference and for documentation purposes. -- To view, visit https://gerrit.ovirt.org/50355 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: jobs: Only report progress if set
Francesco Romani has posted comments on this change. Change subject: jobs: Only report progress if set .. Patch Set 5: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/50355 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: jobs: Only report progress if set
gerrit-hooks has posted comments on this change. Change subject: jobs: Only report progress if set .. 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-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- To view, visit https://gerrit.ovirt.org/50355 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: jobs: Only report progress if set
Nir Soffer has posted comments on this change. Change subject: jobs: Only report progress if set .. Patch Set 4: Code-Review+2 Verified+1 -- To view, visit https://gerrit.ovirt.org/50355 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: jobs: Only report progress if set
gerrit-hooks has posted comments on this change. Change subject: jobs: Only report progress if set .. 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-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- To view, visit https://gerrit.ovirt.org/50355 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: jobs: Only report progress if set
Shahar Havivi has posted comments on this change. Change subject: jobs: Only report progress if set .. Patch Set 3: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/50355 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: jobs: Only report progress if set
Nir Soffer has posted comments on this change. Change subject: jobs: Only report progress if set .. Patch Set 3: Code-Review+2 -- To view, visit https://gerrit.ovirt.org/50355 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg 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/mailman/listinfo/vdsm-patches
Change in vdsm[master]: jobs: Only report progress if set
gerrit-hooks has posted comments on this change. Change subject: jobs: Only report progress if set .. 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-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- To view, visit https://gerrit.ovirt.org/50355 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg 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/mailman/listinfo/vdsm-patches
Change in vdsm[master]: jobs: Only report progress if set
Adam Litke has posted comments on this change.
Change subject: jobs: Only report progress if set
..
Patch Set 2:
(7 comments)
https://gerrit.ovirt.org/#/c/50355/2//COMMIT_MSG
Commit Message:
Line 9: Not all Jobs have deterministic progress and some jobs will complete so
Line 10: quickly that progress updates are not practical. If progress is not
Line 11: available do not report it. Consumers of the Job info could choose to
Line 12: render a non-deterministic progress indication (barber pole, or
spinning
Line 13: indicator) instead of a regular progress bar.
> We expect all jobs to start in indeterminate mode, while the job is pending
Done
Line 14:
Line 15: Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b
https://gerrit.ovirt.org/#/c/50355/2/lib/vdsm/jobs.py
File lib/vdsm/jobs.py:
Line 66: def __init__(self, job_id, description=''):
Line 67: self._id = job_id
Line 68: self._status = STATUS.RUNNING
Line 69: self._description = description
Line 70: self._progress = None
> v2v jobs have more complicated progress:
Done
Line 71: self._error = None
Line 72:
Line 73: @property
Line 74: def id(self):
Line 83: return self._description
Line 84:
Line 85: @property
Line 86: def progress(self):
Line 87: return self._progress
> Return None, so code which does not override this will get no progress. v2v
Done
Line 88:
Line 89: @property
Line 90: def job_type(self):
Line 91: return self._JOB_TYPE
Line 98: ret = {'status': self.status,
Line 99:'description': self.description,
Line 100:'job_type': self.job_type}
Line 101:
Line 102: if self.progress is not None:
> Please make sure to reflect this change in the schema.
Made a note in https://gerrit.ovirt.org/#/c/49450 which is still open.
Line 103: ret['progress'] = self.progress
Line 104:
Line 105: if self.error:
Line 106: ret['error'] = self.error.response()
https://gerrit.ovirt.org/#/c/50355/2/tests/jobsTests.py
File tests/jobsTests.py:
Line 28
Line 29
Line 30
Line 31
Line 32
> Lets keep this and set to None, since jobs.Job does not keep it now.
Done
Line 135: self.assertIsNone(job.progress)
Line 136: self.assertNotIn('progress', job.info())
Line 137:
Line 138: job._progress = 100
Line 139: self.assertEqual(100, job.progress)
> This test is not needed, no point to test properties.
Done
Line 140: self.assertEqual(100, job.info()['progress'])
Line 141:
Line 142: def test_job_get_error(self):
Line 143: job = TestingJob()
Line 136: self.assertNotIn('progress', job.info())
Line 137:
Line 138: job._progress = 100
Line 139: self.assertEqual(100, job.progress)
Line 140: self.assertEqual(100, job.info()['progress'])
> This is correct, but lets show an expected flow that explain how progress s
Done
Line 141:
Line 142: def test_job_get_error(self):
Line 143: job = TestingJob()
Line 144: self.assertIsNone(job.error)
--
To view, visit https://gerrit.ovirt.org/50355
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Dan Kenigsberg
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/mailman/listinfo/vdsm-patches
Change in vdsm[master]: jobs: Only report progress if set
Piotr Kliczewski has posted comments on this change.
Change subject: jobs: Only report progress if set
..
Patch Set 2: Code-Review-1
(1 comment)
https://gerrit.ovirt.org/#/c/50355/2/lib/vdsm/jobs.py
File lib/vdsm/jobs.py:
Line 98: ret = {'status': self.status,
Line 99:'description': self.description,
Line 100:'job_type': self.job_type}
Line 101:
Line 102: if self.progress is not None:
Please make sure to reflect this change in the schema.
Line 103: ret['progress'] = self.progress
Line 104:
Line 105: if self.error:
Line 106: ret['error'] = self.error.response()
--
To view, visit https://gerrit.ovirt.org/50355
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke
Gerrit-Reviewer: Dan Kenigsberg
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/mailman/listinfo/vdsm-patches
Change in vdsm[master]: jobs: Only report progress if set
Nir Soffer has posted comments on this change.
Change subject: jobs: Only report progress if set
..
Patch Set 2:
(4 comments)
https://gerrit.ovirt.org/#/c/50355/2//COMMIT_MSG
Commit Message:
Line 9: Not all Jobs have deterministic progress and some jobs will complete so
Line 10: quickly that progress updates are not practical. If progress is not
Line 11: available do not report it. Consumers of the Job info could choose to
Line 12: render a non-deterministic progress indication (barber pole, or
spinning
Line 13: indicator) instead of a regular progress bar.
We expect all jobs to start in indeterminate mode, while the job is pending
(waiting for available worker thread) or initializing (taking locks). Job that
can provide progress will start to report later.
Also note that jobs without progress do not have to implement anything now,
previously you had to override the progress property.
Line 14:
Line 15: Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b
https://gerrit.ovirt.org/#/c/50355/2/tests/jobsTests.py
File tests/jobsTests.py:
Line 28
Line 29
Line 30
Line 31
Line 32
Lets keep this and set to None, since jobs.Job does not keep it now.
Line 135: self.assertIsNone(job.progress)
Line 136: self.assertNotIn('progress', job.info())
Line 137:
Line 138: job._progress = 100
Line 139: self.assertEqual(100, job.progress)
This test is not needed, no point to test properties.
Line 140: self.assertEqual(100, job.info()['progress'])
Line 141:
Line 142: def test_job_get_error(self):
Line 143: job = TestingJob()
Line 136: self.assertNotIn('progress', job.info())
Line 137:
Line 138: job._progress = 100
Line 139: self.assertEqual(100, job.progress)
Line 140: self.assertEqual(100, job.info()['progress'])
This is correct, but lets show an expected flow that explain how progress
should be used:
# Job queued or initializing, no progress yet
job._progress = None
self.assertNotIn('progress', job.info())
# Job running
for i in [0, 42, 100]:
job._progress = i
self.assertEqual(i, job.info()['progress'])
Line 141:
Line 142: def test_job_get_error(self):
Line 143: job = TestingJob()
Line 144: self.assertIsNone(job.error)
--
To view, visit https://gerrit.ovirt.org/50355
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b
Gerrit-PatchSet: 2
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/mailman/listinfo/vdsm-patches
Change in vdsm[master]: jobs: Only report progress if set
Nir Soffer has posted comments on this change. Change subject: jobs: Only report progress if set .. Patch Set 2: Code-Review-1 (2 comments) We do not need the _progress instance variable. https://gerrit.ovirt.org/#/c/50355/2/lib/vdsm/jobs.py File lib/vdsm/jobs.py: Line 66: def __init__(self, job_id, description=''): Line 67: self._id = job_id Line 68: self._status = STATUS.RUNNING Line 69: self._description = description Line 70: self._progress = None v2v jobs have more complicated progress: 393 @property 394 def progress(self): 395 ''' 396 progress is part of multiple disk_progress its 397 flat and not 100% accurate - each disk take its 398 portion ie if we have 2 disks the first will take 399 0-50 and the second 50-100 400 ''' 401 completed = (self._disk_count - 1) * 100 402 return (completed + self._disk_progress) / self._disk_count So basing progress on an instance variable is not a good idea. We should keep the concept of having to override progress, but the logic should be that job.progress should be None when you cannot report progress. For example, job copying image will return while it is blocked, taking locks and validating the operation, and once qemu-img has started, it will start to return progress value. So in copy image we would do something like this: @property def progress(self): if self._convert_operation: return self._convert_operation.progress return None So Job should have no _progress instance variable. Line 71: self._error = None Line 72: Line 73: @property Line 74: def id(self): Line 83: return self._description Line 84: Line 85: @property Line 86: def progress(self): Line 87: return self._progress Return None, so code which does not override this will get no progress. v2v ImportVm overrides this so it is a safe change. Line 88: Line 89: @property Line 90: def job_type(self): Line 91: return self._JOB_TYPE -- To view, visit https://gerrit.ovirt.org/50355 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b Gerrit-PatchSet: 2 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/mailman/listinfo/vdsm-patches
Change in vdsm[master]: jobs: Only report progress if set
Nir Soffer has posted comments on this change. Change subject: jobs: Only report progress if set .. Patch Set 2: Code-Review+2 -- To view, visit https://gerrit.ovirt.org/50355 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: jobs: Only report progress if set
gerrit-hooks has posted comments on this change. Change subject: jobs: Only report progress if set .. 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-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- To view, visit https://gerrit.ovirt.org/50355 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: jobs: Only report progress if set
Adam Litke has uploaded a new change for review.
Change subject: jobs: Only report progress if set
..
jobs: Only report progress if set
Not all Jobs have deterministic progress and some jobs will complete so
quickly that progress updates are not practical. If progress is not
available do not report it. Consumers of the Job info could choose to
render a non-deterministic progress indication (barber pole, or spinning
indicator) instead of a regular progress bar.
Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b
Signed-off-by: Adam Litke
---
M lib/vdsm/jobs.py
M tests/jobsTests.py
2 files changed, 16 insertions(+), 10 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/55/50355/1
diff --git a/lib/vdsm/jobs.py b/lib/vdsm/jobs.py
index fc4a0c8..a07041f 100644
--- a/lib/vdsm/jobs.py
+++ b/lib/vdsm/jobs.py
@@ -67,6 +67,7 @@
self._id = job_id
self._status = STATUS.RUNNING
self._description = description
+self._progress = None
self._error = None
@property
@@ -83,7 +84,7 @@
@property
def progress(self):
-raise NotImplementedError()
+return self._progress
@property
def job_type(self):
@@ -96,9 +97,11 @@
def info(self):
ret = {'status': self.status,
'description': self.description,
- 'progress': self.progress,
'job_type': self.job_type}
+if self.progress is not None:
+ret['progress'] = self.progress
+
if self.error:
ret['error'] = self.error.response()
diff --git a/tests/jobsTests.py b/tests/jobsTests.py
index 4bd6343..6855dcf 100644
--- a/tests/jobsTests.py
+++ b/tests/jobsTests.py
@@ -29,12 +29,7 @@
def __init__(self):
jobs.Job.__init__(self, str(uuid.uuid4()))
-self._progress = 0
self._aborted = False
-
-@property
-def progress(self):
-return self._progress
def _abort(self):
self._aborted = True
@@ -63,9 +58,8 @@
def test_job_info(self):
job = TestingJob()
-self.assertEqual({'status': 'running', 'progress': 0,
- 'job_type': 'testing', 'description': ''},
- job.info())
+self.assertEqual({'status': 'running', 'job_type': 'testing',
+ 'description': ''}, job.info())
def test_add_job(self):
job = TestingJob()
@@ -136,6 +130,15 @@
self.assertEqual(response.error(jobs.NoSuchJob.name),
jobs.delete('foo'))
+def test_job_get_progress(self):
+job = TestingJob()
+self.assertIsNone(job.progress)
+self.assertNotIn('progress', job.info())
+
+job._progress = 100
+self.assertEqual(100, job.progress)
+self.assertEqual(100, job.info()['progress'])
+
def test_job_get_error(self):
job = TestingJob()
self.assertIsNone(job.error)
--
To view, visit https://gerrit.ovirt.org/50355
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: jobs: Only report progress if set
gerrit-hooks has posted comments on this change. Change subject: jobs: Only report progress if set .. 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-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- To view, visit https://gerrit.ovirt.org/50355 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3cc8b66d825676045fbcb8431bb3cf11885ff02b 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/mailman/listinfo/vdsm-patches
