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

shavivi at redhat.com shavivi at redhat.com
Wed Mar 11 11:11:25 UTC 2015


Shahar Havivi 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.
Done
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.
Done
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
Done
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"
Done
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:
Done
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
Done
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 s
if the status is error there is no error we just need to delete the job since its done (with errors) but we have no ovf.
the delete job will be at a new verb as suggested in the next comment
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 t
in this case (which is right)
we need to add another call for vdsm and change the logic of this method
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.
Done
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
Done
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()
I didn't put read_ovf in a method since it also check if the file exists and return invalidV2VOvfPath if not.
I don't want the method to return error an check for errors when it suppose to return 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
Done
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.
Done
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 
Done
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:
Done
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.
Done
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.
Done
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
Done
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