Change in vdsm[master]: Refactor v2v jobs for reusability

alitke at redhat.com alitke at redhat.com
Mon Aug 24 20:52:10 UTC 2015


Adam Litke has posted comments on this change.

Change subject: Refactor v2v jobs for reusability
......................................................................


Patch Set 5:

(8 comments)

https://gerrit.ovirt.org/#/c/44857/5/tests/jobsTests.py
File tests/jobsTests.py:

Line 28: class TestingJob(jobs.Job):
Line 29:     _JOB_TYPE = 'testing'
Line 30: 
Line 31:     def __init__(self, job_id):
Line 32:         jobs.Job.__init__(self, job_id)
> To make it easier to test, I would generate the job_id here, so tests do
Done
Line 33:         self._progress = 0
Line 34:         self._aborted = False
Line 35: 
Line 36:     @property


Line 38:         return self._progress
Line 39: 
Line 40:     def _abort(self):
Line 41:         self._aborted = True
Line 42: 
> Lets add some job subclasses to make it easier to test filtering:
Done
Line 43: 
Line 44: @expandPermutations
Line 45: class JobsTests(VdsmTestCase):
Line 46:     TIMEOUT = 1


Line 46:     TIMEOUT = 1
Line 47: 
Line 48:     def setUp(self):
Line 49:         jobs._clear()
Line 50:         self.job_id = str(uuid.uuid4())
> Should be in TestingJob, this work only for single job, which is not correc
Done
Line 51: 
Line 52:     def test_job_initial_state(self):
Line 53:         job = TestingJob(self.job_id)
Line 54:         self.assertEqual(jobs.STATUS.RUNNING, job.status)


Line 79:             job = TestingJob(job_id)
Line 80:             job._progress = values['progress']
Line 81:             job._description = values['description']
Line 82:             job._status = values['status']
Line 83:             jobs.add(job)
> I don't think we need this helper.
Done
Line 84: 
Line 85:     def test_get_jobs_info(self):
Line 86:         job_info = {'dfc3f588-d122-4443-a0c2-51fe969be8f5':
Line 87:                     {'status': jobs.STATUS.RUNNING,


Line 92:                     {'status': jobs.STATUS.DONE,
Line 93:                      'description': 'another job',
Line 94:                      'progress': 100,
Line 95:                      'job_type': 'testing'}}
Line 96:         self._add_jobs(job_info)
> We don't check all these values, so we can do instead (assuming that we mod
Done
Line 97:         self.assertEqual(job_info, jobs.get_jobs_info())
Line 98: 
Line 99:     def test_get_jobs_info_empty(self):
Line 100:         self.assertEqual({}, jobs.get_jobs_info())


Line 109:                     {'status': jobs.STATUS.DONE,
Line 110:                      'description': 'another job',
Line 111:                      'progress': 100,
Line 112:                      'job_type': 'bar'}}
Line 113:         self._add_jobs(job_info)
> This could be simplified like the test above:
Done
Line 114:         self.assertNotIn('9f40a5d9-301e-4244-9a73-4d49d827d0b9',
Line 115:                          jobs.get_jobs_info(type_filter='foo'))
Line 116: 
Line 117:     def test_abort_job(self):


https://gerrit.ovirt.org/#/c/44857/5/vdsm/jobs.py
File vdsm/jobs.py:

Line 142:         return response.error(e.name)
Line 143:     return response.success()
Line 144: 
Line 145: 
Line 146: def get_jobs_info(type_filter=None):
> Since all other names are short and nice, I think we get shorten it to info
Done
Line 147:     with _lock:
Line 148:         jobs = _jobs.values()
Line 149:     return {job.id: job.info() for job in jobs
Line 150:             if not type_filter or job.job_type == type_filter}


https://gerrit.ovirt.org/#/c/44857/5/vdsm/v2v.py
File vdsm/v2v.py:

Line 107: 
Line 108: def get_jobs_status():
Line 109:     all_jobs = jobs.get_jobs_info()
Line 110:     return {job_id: status for (job_id, status) in all_jobs.items()
Line 111:             if status['job_type'] == _V2V_JOB_TYPE}
> Why not use your new shiny filtering in jobs.get_jobs_info() ?
Done
Line 112: 
Line 113: 
Line 114: def get_external_vms(uri, username, password):
Line 115:     if not supported():


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ida6b1c460c5030c820c540e836e423d4632410df
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Arik Hadas <ahadas at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <mskrivan at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list