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

nsoffer at redhat.com nsoffer at redhat.com
Tue Mar 10 17:43:37 UTC 2015


Nir Soffer has posted comments on this change.

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


Patch Set 8:

(18 comments)

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

Line 37: 
Line 38: _lock = threading.Lock()
Line 39: _jobs = {}
Line 40: 
Line 41: _P_V2V_DIR = os.path.join(P_VDSM_RUN, 'v2v')
Please remove the P_ prefix.
Line 42: _VIRT_V2V = CommandPath('virt-v2v', '/usr/bin/virt-v2v')
Line 43: 
Line 44: _STATUS_INITIALIZING = 'initializing'
Line 45: _STATUS_STARTING = 'starting'


Line 45: _STATUS_STARTING = 'starting'
Line 46: _STATUS_DONE = 'done'
Line 47: _STATUS_ABORT = 'abort'
Line 48: _STATUS_ERROR = 'error'
Line 49: _STATUS_DISK_COPY = 'copy'
Using a class may be nicer.

    class STATUS:
        INITIALIZING = 'initializing'
        ...
Line 50: 
Line 51: 
Line 52: class InvalidVMConfiguration(ValueError):
Line 53:     ''' Unexpected error while parsing libvirt domain xml '''


Line 91:         return {'status': doneCode, 'vmList': vms}
Line 92: 
Line 93: 
Line 94: def convert(uri, username, password, vmProperties, jobId, cif):
Line 95:     global _jobs
Not needed
Line 96:     t = ImportVm(uri, username, password, vmProperties, jobId, cif)
Line 97:     with _lock:
Line 98:         _jobs[jobId] = t
Line 99:     t.start()


Line 92: 
Line 93: 
Line 94: def convert(uri, username, password, vmProperties, jobId, cif):
Line 95:     global _jobs
Line 96:     t = ImportVm(uri, username, password, vmProperties, jobId, cif)
Rename "t" to "job"
Line 97:     with _lock:
Line 98:         _jobs[jobId] = t
Line 99:     t.start()
Line 100:     return {'status': doneCode}


Line 95:     global _jobs
Line 96:     t = ImportVm(uri, username, password, vmProperties, jobId, cif)
Line 97:     with _lock:
Line 98:         _jobs[jobId] = t
Line 99:     t.start()
You should do:

    job.start()
    with _lock:
        _jobs[jobId] = job

So if job.start() raises, this function will fail without leaving stale job in the dict.
Line 100:     return {'status': doneCode}
Line 101: 
Line 102: 
Line 103: def get_converted_vm(jobId):


Line 104:     ''' get_converted_vm will return the VMs OVF (output of virt-v2v) and
Line 105:         delete the job.
Line 106:         if there were errors a call to get_converted_vm will only delete
Line 107:         the job'''
Line 108:     global _jobs
Not needed
Line 109:     with _lock:
Line 110:         if jobId not in _jobs:
Line 111:             return errCode['invalidV2VJob']
Line 112:         elif _jobs[jobId].status != _STATUS_ERROR:


Line 114:             return {'status': doneCode}
Line 115:         elif _jobs[jobId].status != _STATUS_DONE:
Line 116:             return errCode['V2VJobNotDone']
Line 117:         else:
Line 118:             del _jobs[jobId]
Too complex, I can hardly understand the logic here. Lets write this in a simple way:

    with _lock:
        if jobId not in _jobs:
            return errCode['invalidV2VJob']
        job = jobs[jobId]
        
    if job.status == _STATUS_ERROR:
        # Hopefully this is the correct action on errors
        # I'm guessing because the intent of the original
        # code was unclear.
        del _jobs[jobId]
        return some error?

    if job.status != _STATUS_DONE:
        return errCode['V2VJobNotDone']

    ovf = job.read_ovf()
    del _jobs[jobId]
    return {"status": doneCode, "ovf": ovf}
Line 119: 
Line 120:     try:
Line 121:         file_name = os.path.join(_P_V2V_DIR, "%s.ovf" % jobId)
Line 122:         with open(file_name, 'r') as f:


Line 120:     try:
Line 121:         file_name = os.path.join(_P_V2V_DIR, "%s.ovf" % jobId)
Line 122:         with open(file_name, 'r') as f:
Line 123:             ovf = f.read()
Line 124:         os.remove(file_name)
This is not a good idea. If the client get disconnected now, we just lost the ovf, and engine did not get yet.

Cleaning up ovf should be triggered by engine *after* it got the ovf from the server and stored it safely in the database.
Line 125:     except IOError as exc:
Line 126:         if exc.errno == errno.ENOENT:
Line 127:             return errCode['invalidV2VOvfPath']
Line 128:         else:


Line 121:         file_name = os.path.join(_P_V2V_DIR, "%s.ovf" % jobId)
Line 122:         with open(file_name, 'r') as f:
Line 123:             ovf = f.read()
Line 124:         os.remove(file_name)
Line 125:     except IOError as exc:
We use "e" instead of "exc" usually.
Line 126:         if exc.errno == errno.ENOENT:
Line 127:             return errCode['invalidV2VOvfPath']
Line 128:         else:
Line 129:             logging.error('error reading file "%s" error: %r message %r',


Line 125:     except IOError as exc:
Line 126:         if exc.errno == errno.ENOENT:
Line 127:             return errCode['invalidV2VOvfPath']
Line 128:         else:
Line 129:             logging.error('error reading file "%s" error: %r message %r',
- we capitalize log messages - I prefer your version but we should keep the log consistent.
- for file_name you should use %r, which will render as 'path', revealing leading or trailing whitespace, which may cause the code to fail.
- for numeric error code there is no need to use %r, and %s or %d is good enough.
- for message, you should not use %r, since we don't care about leading or trailing whitespace.

So this should be:

    logging.error("Error reading %r errno: %s message: %s",
                  file_name, e.errno, e.message)
Line 130:                           file_name, exc.errno, exc.message)
Line 131:             raise
Line 132:     return {'status': doneCode, 'ovf': ovf}
Line 133: 


Line 127:             return errCode['invalidV2VOvfPath']
Line 128:         else:
Line 129:             logging.error('error reading file "%s" error: %r message %r',
Line 130:                           file_name, exc.errno, exc.message)
Line 131:             raise
This part should move to job.read_ovf()
Line 132:     return {'status': doneCode, 'ovf': ovf}
Line 133: 
Line 134: 
Line 135: def abort_job(jobId):


Line 132:     return {'status': doneCode, 'ovf': ovf}
Line 133: 
Line 134: 
Line 135: def abort_job(jobId):
Line 136:     global _jobs
Not needed
Line 137:     with _lock:
Line 138:         if jobId not in _jobs:
Line 139:             return errCode['invalidV2VJob']
Line 140:         t = _jobs[jobId]


Line 144:     return {'status': doneCode}
Line 145: 
Line 146: 
Line 147: def jobs():
Line 148:     global _jobs
Not needed - the name jobs will be found in the global namespace when used. global is needed only when you want to assign to the global (jobs = new value).
Line 149:     ret = {}
Line 150:     with _lock:
Line 151:         for jobId, jobValue in _jobs.iteritems():
Line 152:             ret[jobId] = {


Line 172:     delete the job.
Line 173:     """
Line 174:     def __init__(self, uri, username, password, vmProperties, jobId, cif):
Line 175:         self._thread = threading.Thread(target=self._run)
Line 176:         self._thread.daemon = True
Creating a thread and not starting it is a bad idea. You should create the thread in start().
Line 177:         self._uri = uri
Line 178:         self._username = username
Line 179:         self._password = password
Line 180:         self._vmProperties = vmProperties


Line 203:     @property
Line 204:     def progress(self):
Line 205:         return self._progress
Line 206: 
Line 207:     @traceback()
You can add context to traceback:

    @traceback(msg="Error importing vm")
Line 208:     def _run(self):
Line 209:         try:
Line 210:             self._import()
Line 211:         except Exception as ex:


Line 204:     def progress(self):
Line 205:         return self._progress
Line 206: 
Line 207:     @traceback()
Line 208:     def _run(self):
Since you delete the password file here, it is better to create it here.
Line 209:         try:
Line 210:             self._import()
Line 211:         except Exception as ex:
Line 212:             logging.info('error in importing external vm: %r', ex)


Line 208:     def _run(self):
Line 209:         try:
Line 210:             self._import()
Line 211:         except Exception as ex:
Line 212:             logging.info('error in importing external vm: %r', ex)
Since we raise and @traceback will log a traceback, we don't need this log.
Line 213:             self._status = _STATUS_ERROR
Line 214:             self._status_msg = ex.message
Line 215:             raise
Line 216:         finally:


Line 217:             self._delete_passwd_file()
Line 218:             self._teardownVolumes()
Line 219: 
Line 220:     def _import(self):
Line 221:         self._create_passwd_file()
Move to _run to make it more clear
Line 222:         self._status = _STATUS_STARTING
Line 223:         # TODO: use the process handling http://gerrit.ovirt.org/#/c/33909/
Line 224:         cmd = [str(_VIRT_V2V), '-ic', self._uri, '-o', 'vdsm', '-of', 'raw']
Line 225:         cmd.extend(self._generate_disk_parameters())


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