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

alitke at redhat.com alitke at redhat.com
Fri Aug 21 20:38:55 UTC 2015


Adam Litke has posted comments on this change.

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


Patch Set 3:

(7 comments)

https://gerrit.ovirt.org/#/c/44857/3//COMMIT_MSG
Commit Message:

Line 11: to manage long-running storage operations.  In order to make this
Line 12: possible, factor out the job management parts of v2v.py into a new
Line 13: module jobs.py and create a generic Job class which contains the
Line 14: complete interface needed by the job manager.  In addition, add some
Line 15: basic tests for Job objects and their management.
> please make sure to verify this patch using Engine.
I'm waiting for a time when the upstream engine and vdsm code actually work together :)
Line 16: 
Line 17: Change-Id: Ida6b1c460c5030c820c540e836e423d4632410df


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

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'
Line 49:     V2V = 'v2v'
> We can remove this class now.
ahh, yes... Done.
Line 50: 
Line 51: 
Line 52: class ClientError(Exception):
Line 53:     ''' Base class for client error '''


Line 73:     _JOB_TYPE = None
Line 74: 
Line 75:     def __init__(self, job_id, description=''):
Line 76:         self._id = job_id
Line 77:         self._status = STATUS.RUNNING
> We should check with v2v guys that engine is not expecting the previous val
Perhaps I should just add starting as a v2v-specific status and keep running in the generic job.  Then they can continue using their terminology but the base class will be made more generic.
Line 78:         self._description = description
Line 79: 
Line 80:     @property
Line 81:     def id(self):


Line 115:         May be implemented by child class
Line 116:         """
Line 117: 
Line 118: 
Line 119: def clear():
> Lets add a comment that this should be called only by the tests. To make it
Done
Line 120:     with _lock:
Line 121:         _jobs.clear()
Line 122: 
Line 123: 


Line 120:     with _lock:
Line 121:         _jobs.clear()
Line 122: 
Line 123: 
Line 124: def delete_job(job_id):
> jobs.delete_job() looks redundant
Agreed.  Will do.
Line 125:     try:
Line 126:         job = get_job(job_id)
Line 127:         job.validate_not_running()
Line 128:         _delete_job(job_id)


Line 150:         ret[job_id] = {
Line 151:             'status': job.status,
Line 152:             'description': job.description,
Line 153:             'progress': job.progress,
Line 154:             'job_type': job.job_type,
> This is better done in Job.info():
Done
Line 155:         }
Line 156:     return ret
Line 157: 
Line 158: 


Line 177:         del _jobs[job_id]
Line 178: 
Line 179: 
Line 180: # Automatically initialize on first import
Line 181: clear()
> This is not needed now, as clear() is not initializing anything. It is only
Done


-- 
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: 3
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