Nir Soffer has posted comments on this change.
Change subject: Refactor v2v jobs for reusability
......................................................................
Patch Set 2: Code-Review-1
(27 comments)
Looks good, see inline comments.
https://gerrit.ovirt.org/#/c/44857/2/tests/jobsTests.py
File tests/jobsTests.py:
Line 19:
Line 20: import uuid
Line 21:
Line 22: import jobs
Line 23: from vdsm.define import errCode, doneCode
Replace with response module.
Line 24:
Line 25: from testlib import VdsmTestCase, expandPermutations, permutations
Line 26:
Line 27:
Line 44: class JobsTests(VdsmTestCase):
Line 45: TIMEOUT = 1
Line 46:
Line 47: def setUp(self):
Line 48: jobs.initialize()
clear()
Line 49: self.job_id = str(uuid.uuid4())
Line 50:
Line 51: def test_job_initial_state(self):
Line 52: job = TestingJob(self.job_id)
https://gerrit.ovirt.org/#/c/44857/2/vdsm/jobs.py
File vdsm/jobs.py:
Line 19:
Line 20: import logging
Line 21: import threading
Line 22:
Line 23: from vdsm.define import errCode, doneCode
Please remove and import the response module instead.
Line 24:
Line 25:
Line 26: _lock = None
Line 27: _jobs = None
Line 23: from vdsm.define import errCode, doneCode
Line 24:
Line 25:
Line 26: _lock = None
Line 27: _jobs = None
Lets initialize these here instead of calling initialize()
Line 28:
Line 29:
Line 30: class STATUS:
Line 31: '''
Line 28:
Line 29:
Line 30: class STATUS:
Line 31: '''
Line 32: STARTING: request granted and starting the import process
Starting is not very good term, how about "running"?
Also this is not about import any more.
Line 33: ABORTED: user initiated aborted
Line 34: FAILED: error during import process
Line 35: DONE: convert process successfully finished
Line 36: '''
Line 31: '''
Line 32: STARTING: request granted and starting the import process
Line 33: ABORTED: user initiated aborted
Line 34: FAILED: error during import process
Line 35: DONE: convert process successfully finished
This is not about import or convert now.
Line 36: '''
Line 37: STARTING = 'starting'
Line 38: ABORTED = 'aborted'
Line 39: FAILED = 'error'
Line 44: '''
Line 45: UNKNOWN: the job type is unknown
Line 46: V2V: the job is a v2v VM conversion job
Line 47: '''
Line 48: UNKNOWN = 'unknown'
Why do we need UNKNOWN type?
And why do we need to keep the type in a central location? Why not define this in the Job
class, so each job can define its own type, without modifying the generic jobs module?
Line 49: V2V = 'v2v'
Line 50:
Line 51:
Line 52: class ClientError(Exception):
Line 54:
Line 55:
Line 56: class JobExistsError(ClientError):
Line 57: ''' Job already exists in _jobs collection '''
Line 58: err_name = 'JobExistsError'
This name is bad. When it was some v2v error class, and part of a huge patch, we had to
ignore it, but now we can expect better names.
Lets call is "name".
Line 59:
Line 60:
Line 61: class NoSuchJob(ClientError):
Line 62: ''' Job not exists in _jobs collection '''
Line 58: err_name = 'JobExistsError'
Line 59:
Line 60:
Line 61: class NoSuchJob(ClientError):
Line 62: ''' Job not exists in _jobs collection '''
not -> does not?
Line 63: err_name = 'NoSuchJob'
Line 64:
Line 65:
Line 66: class JobNotDone(ClientError):
Line 63: err_name = 'NoSuchJob'
Line 64:
Line 65:
Line 66: class JobNotDone(ClientError):
Line 67: ''' Import process still in progress '''
"Job still in progress"?
Line 68: err_name = 'JobNotDone'
Line 69:
Line 70:
Line 71: class Job(object):
Line 68: err_name = 'JobNotDone'
Line 69:
Line 70:
Line 71: class Job(object):
Line 72: def __init__(self, job_id):
Lets add optional description argument?
Line 73: self._id = job_id
Line 74: self._status = STATUS.STARTING
Line 75: self._description = ''
Line 76:
Line 90: def progress(self):
Line 91: """
Line 92: Implemented by child class
Line 93: """
Line 94: return 0
Should we include a default implementation, or require subclass to implement it by raising
here NotImplementedError?
Line 95:
Line 96: @property
Line 97: def job_type(self):
Line 98: return TYPE.UNKNOWN
Line 94: return 0
Line 95:
Line 96: @property
Line 97: def job_type(self):
Line 98: return TYPE.UNKNOWN
We can return self._type, so each job can define its type.
Line 99:
Line 100: def abort(self):
Line 101: self._status = STATUS.ABORTED
Line 102: logging.info('Job %r aborting...', self._id)
Line 105: def _abort(self):
Line 106: """
Line 107: Implemented by child class
Line 108: """
Line 109: pass
The pass is not required, having a comment is enough for a method that does nothing, and
is better since it can explain why this does nothing.
To make it more clear that this *may* be implemented (otherwise you would raise here
NotImplementedError), lets write "May be implemented ...".
Line 110:
Line 111:
Line 112: def initialize():
Line 113: global _lock, _jobs
Line 111:
Line 112: def initialize():
Line 113: global _lock, _jobs
Line 114: _lock = threading.Lock()
Line 115: _jobs = {}
Lets simplify this by doing:
def clear():
with _lock:
_jobs.clear()
This is only for testing - right?
Line 116:
Line 117:
Line 118: def delete_job(job_id):
Line 119: try:
Line 118: def delete_job(job_id):
Line 119: try:
Line 120: job = get_job(job_id)
Line 121: validate_job_finished(job)
Line 122: _remove_job(job_id)
This should be called _delete_job - it is not clear why we are using different terms in
the public and the private functions.
Line 123: except ClientError as e:
Line 124: logging.info('Cannot delete job, error: %s', e)
Line 125: return errCode[e.err_name]
Line 126: return {'status': doneCode}
Line 121: validate_job_finished(job)
Line 122: _remove_job(job_id)
Line 123: except ClientError as e:
Line 124: logging.info('Cannot delete job, error: %s', e)
Line 125: return errCode[e.err_name]
Use response.error()
Line 126: return {'status': doneCode}
Line 127:
Line 128:
Line 129: def abort_job(job_id):
Line 131: job = get_job(job_id)
Line 132: job.abort()
Line 133: except ClientError as e:
Line 134: logging.info('Cannot abort job, error: %s', e)
Line 135: return errCode[e.err_name]
response.error()
Line 136: return {'status': doneCode}
Line 137:
Line 138:
Line 139: def get_jobs_status():
Line 132: job.abort()
Line 133: except ClientError as e:
Line 134: logging.info('Cannot abort job, error: %s', e)
Line 135: return errCode[e.err_name]
Line 136: return {'status': doneCode}
response.success()
Line 137:
Line 138:
Line 139: def get_jobs_status():
Line 140: ret = {}
Line 149: }
Line 150: return ret
Line 151:
Line 152:
Line 153: def add_job(job_id, job):
We don't need the job_id parameter, we should use job.id attribute. This code make it
possible to register a job with id "foo" for the id "bar", which is
unwanted property.
Line 154: with _lock:
Line 155: if job_id in _jobs:
Line 156: raise JobExistsError("Job %r exists" % job_id)
Line 157: _jobs[job_id] = job
Line 172:
Line 173:
Line 174: def validate_job_done(job):
Line 175: if job.status != STATUS.DONE:
Line 176: raise JobNotDone("Job %r is %s" % (job.id, job.status))
This should be implemented by the job class, so each job can different statuses, and tell
when it is considered done.
Line 177:
Line 178:
Line 179: def validate_job_finished(job):
Line 180: if job.status not in (STATUS.DONE, STATUS.FAILED, STATUS.ABORTED):
Line 177:
Line 178:
Line 179: def validate_job_finished(job):
Line 180: if job.status not in (STATUS.DONE, STATUS.FAILED, STATUS.ABORTED):
Line 181: raise JobNotDone("Job %r is %s" % (job.id, job.status))
Same, each job should have its own logic considering status and finished state.
Line 182:
Line 183:
Line 184: # Automatically initialize on first import
https://gerrit.ovirt.org/#/c/44857/2/vdsm/v2v.py
File vdsm/v2v.py:
Line 313
Line 314
Line 315
Line 316
Line 317
Lets add the type here:
_JOB_TYPE = "v2v"
(I would use _TYPE but I know you have a problem with this word)
Line 175:
Line 176: def get_converted_vm(job_id):
Line 177: try:
Line 178: job = jobs.get_job(job_id)
Line 179: jobs.validate_job_done(job)
This would read better as:
job.validate_done()
Line 180: ovf = _read_ovf(job_id)
Line 181: except jobs.ClientError as e:
Line 182: logging.info('Converted VM error %s', e)
Line 183: return errCode[e.err_name]
Line 288: return (completed + self._disk_progress) / self._disk_count
Line 289:
Line 290: @property
Line 291: def job_type(self):
Line 292: return jobs.TYPE.V2V
return self._JOB_TYPE
Line 293:
Line 294: def _run_with_password(self):
Line 295: with password_file(self._id, self._passwd_file, self._password):
Line 296: self._run()
Line 303: if self._aborted:
Line 304: logging.debug("Job %r was aborted", self._id)
Line 305: else:
Line 306: logging.exception("Job %r failed", self._id)
Line 307: self._status = jobs.STATUS.FAILED
Not needed, as V2VSTATUS inherits from jobs.STATUS. We can use V2VSTATUS.FAILED instead.
Line 308: self._description = ex.message
Line 309: try:
Line 310: self._abort()
Line 311: except Exception as e:
Line 333: (self._id, self._proc.returncode,
Line 334: self._proc.stderr.read(1024)))
Line 335:
Line 336: if self._status != jobs.STATUS.ABORTED:
Line 337: self._status = jobs.STATUS.DONE
use V2VSTATUS
Line 338: logging.info('Job %r finished import successfully',
self._id)
Line 339:
Line 340: def _execution_environments(self):
Line 341: env = {'LIBGUESTFS_BACKEND': 'direct'}
--
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Arik Hadas <ahadas(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes