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