Change in vdsm[master]: v2v: Convert VM from external source to Data Domain

nsoffer at redhat.com nsoffer at redhat.com
Sun Mar 15 20:44:58 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: v2v: Convert VM from external source to Data Domain
......................................................................


Patch Set 10:

(8 comments)

New code looks very nice, but see my comments.

I did not review all the file, will continue later.

https://gerrit.ovirt.org/#/c/37509/10/vdsm/v2v.py
File vdsm/v2v.py:

Line 41: status is a way to feedback information on the job (init, error etc)
Line 42: status_msg is the descriptoin of the status
Line 43: when the job is done a call to get_converted_vm will return the VMs OVF.
Line 44: to clear a jobs once the OVF retrieved call to delete_job
Line 45: """
This should be first thing in the module after the copyright block.
Line 46: 
Line 47: _lock = threading.Lock()
Line 48: _jobs = {}
Line 49: 


Line 74: 
Line 75: class NoSuchJob(V2VJobError):
Line 76:     ''' Job not exists in _jobs collection '''
Line 77:     def __init__(self):
Line 78:         self.code = 'invalidV2VJob'
This is best done as:

    class ErrorName:
        """ Description """
        name = "errorname"

And you don't need to define __init__.

I would not use "code" for a string. This is not an error code but an error name used to get the status dict for this error.

This will be replaced soon by response.error(name, message).
Line 79: 
Line 80: 
Line 81: class JobExists(V2VJobError):
Line 82:     ''' Job already exists in _jobs collection '''


Line 97: 
Line 98: 
Line 99: class VolumeError(RuntimeError):
Line 100:     def __str__(self):
Line 101:         return "Bad volume specification " + RuntimeError.__str__(self)
Why do you need this?
Line 102: 
Line 103: 
Line 104: def supported():
Line 105:     return not (caps.getos() in (caps.OSName.RHEVH, caps.OSName.RHEL)


Line 180: def jobs():
Line 181:     ret = {}
Line 182:     with _lock:
Line 183:         jobs = _jobs
Line 184:     for jobId, jobValue in jobs.iteritems():
This is unsafe - iteritems() is good when you have 100000 jobs and you don't want to copy all of them. But we have only few (since copying disk is pretty expensive), so nobody care about copying few bytes.

    jobs = _jobs

Is adding a name "jobs" and binding it to the same dict that _jobs is bound to.

You want to copy the jobs dict contents, so:

    with _lock:
        items = _jobs.items()
    for jobId, job in items:
        ...

And jobValue is strange - and make the rest of the code looks bad.
Line 185:         ret[jobId] = {
Line 186:             'status': jobValue.status,
Line 187:             'status_msg': jobValue.status_msg,
Line 188:             'disk_progress': jobValue.disk_progress,


Line 182:     with _lock:
Line 183:         jobs = _jobs
Line 184:     for jobId, jobValue in jobs.iteritems():
Line 185:         ret[jobId] = {
Line 186:             'status': jobValue.status,
Whats can be nicer then "job.status" here?
Line 187:             'status_msg': jobValue.status_msg,
Line 188:             'disk_progress': jobValue.disk_progress,
Line 189:             'progress': jobValue.progress
Line 190:         }


Line 218: 
Line 219: 
Line 220: def _read_ovf(jobId):
Line 221:     try:
Line 222:         file_name = os.path.join(_V2V_DIR, "%s.ovf" % jobId)
This cannot raise IOError, and should be out of the try except block.
Line 223:         with open(file_name, 'r') as f:
Line 224:             ovf = f.read()
Line 225:         return ovf
Line 226:     except IOError as e:


Line 221:     try:
Line 222:         file_name = os.path.join(_V2V_DIR, "%s.ovf" % jobId)
Line 223:         with open(file_name, 'r') as f:
Line 224:             ovf = f.read()
Line 225:         return ovf
You can simplify by:

    return f.read()
Line 226:     except IOError as e:
Line 227:         if e.errno == errno.ENOENT:
Line 228:             raise InvalidOvfPath()
Line 229:         else:


Line 224:             ovf = f.read()
Line 225:         return ovf
Line 226:     except IOError as e:
Line 227:         if e.errno == errno.ENOENT:
Line 228:             raise InvalidOvfPath()
This error looks wrong - the error is here is should be something like "NoSuchOvf".

It is unlikely that the path is invalid, as you built it in line 222, and the job is was correct, otherwise we would fail before reaching this.

The only possible error is that the path is valid but the ovf is not there.
Line 229:         else:
Line 230:             logging.error('Error reading file "%r" error: %s message %s',
Line 231:                           file_name, e.errno, e.message)
Line 232:             raise


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I34bd86d5a87ea8c42113c4a732f87ddd4ceab9ea
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list