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

alitke at redhat.com alitke at redhat.com
Tue Aug 18 20:49:04 UTC 2015


Adam Litke has posted comments on this change.

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


Patch Set 2:

(27 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.
Done
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()
Done
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.
Done
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()
Why?  Seems better to have the values set consistently from one place.  If it helps I can move the initialize function up here.
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"?
fine.
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.
yep, fixed.
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?
ok
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 patc
Done
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?
Done
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"?
Done
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?
Done
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 implemen
I think we should raise.
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.
Done
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
Done
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:
Yes.
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 differ
Done
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()
Done
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()
Done
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()
Done
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 co
Done
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 stat
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 st
Done
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:
Done


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:
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
moved to the Job class.
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.FA
Done
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
Done
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 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