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

nsoffer at redhat.com nsoffer at redhat.com
Wed Aug 19 18:57:00 UTC 2015


Nir Soffer has posted comments on this change.

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


Patch Set 3:

(10 comments)

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.
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 value ("starting"). If it does, I hope it is not too late to fix engine to use
the more generic term "running".
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 more clear, we can make this private, like libvirtconnection._clear_connections.
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
I agree with Francesco, we don't need to repeat "job" in these names. This module is dealing only with jobs.
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():

    for job_id, job in items:
        ret[job_id] = job.info()

Or since we support now only Python 2.7:

    with _lock:
        jobs = _jobs.values()
    return {job.id: job.info() for job in jobs}

And implement info() in Job:

    def info(self):
        return {
            'status': job.status,
            'description': job.description,
            'progress': job.progress,
	    'job_type': job.job_type,
        }

This allows jobs to return additional job-specific info, for example,
v2v could return the name of the current disk and the progress
of the current disk.

Finally get_jobs_status() is incorrect, as this return job.status, job.progress, etc. It should be called get_jobs_info() or get_jobs_stats(). This can be also done in a separate patch to keep this patch smaller.
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()
> I prefer explicit initialization, but let's see what the other reviewers li
This is not needed now, as clear() is not initializing anything. It is only need for the tests.

Everything is already initialized before line 180.


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

Line 101
Line 102
Line 103
Line 104
Line 105
This is not a client error, but not related to your patch. Hopefully Shahar will fix this later.


Line 45: import caps
Line 46: import jobs
Line 47: 
Line 48: 
Line 49: _V2V_JOB_TYPE = 'v2v'
Not needed, should be defined only in ImportVm._JOB_TYPE
Line 50: _V2V_DIR = os.path.join(P_VDSM_RUN, 'v2v')
Line 51: _VIRT_V2V = CommandPath('virt-v2v', '/usr/bin/virt-v2v')
Line 52: _OVF_RESOURCE_CPU = 3
Line 53: _OVF_RESOURCE_MEMORY = 4


Line 107: 
Line 108: def get_jobs_status():
Line 109:     all_jobs = jobs.get_jobs_status()
Line 110:     return {job_id: status for (job_id, status) in all_jobs.items()
Line 111:             if status['job_type'] == _V2V_JOB_TYPE}
> nice :)
Not nice, and not needed. job_type is set in jobs.get_jobs_status(), and this extra step is not nice for the users of the jobs module.
Line 112: 
Line 113: 
Line 114: def get_external_vms(uri, username, password):
Line 115:     if not supported():


Line 232:     def __init__(self, vminfo, job_id, irs):
Line 233:         '''
Line 234:         do not use directly, use a factory method instead!
Line 235:         '''
Line 236:         jobs.Job.__init__(self, job_id)
> why not super()?
Right, we should use super more:
https://www.youtube.com/watch?v=EiOglTERPEo
Line 237:         self._vminfo = vminfo
Line 238:         self._irs = irs
Line 239: 
Line 240:         self._disk_progress = 0


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