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

shavivi at redhat.com shavivi at redhat.com
Sun Feb 15 13:56:39 UTC 2015


Shahar Havivi has posted comments on this change.

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


Patch Set 3:

(13 comments)

http://gerrit.ovirt.org/#/c/37509/3/configure.ac
File configure.ac:

Line 334: AC_PATH_PROG([TOUCH_PATH], [touch], [/bin/touch])
Line 335: AC_PATH_PROG([TUNE2FS_PATH], [tune2fs], [/sbin/tune2fs])
Line 336: AC_PATH_PROG([UDEVADM_PATH], [udevadm], [/sbin/udevadm])
Line 337: AC_PATH_PROG([UMOUNT_PATH], [umount], [/bin/umount])
Line 338: AC_PATH_PROG([VIRT_V2V_PATH], [virt-v2v], [/bin/bin/virt-v2v])
> /usr/bin or maybe /bin
/usr/bin/....
Line 339: AC_PATH_PROG([WGET_PATH], [wget], [/usr/bin/wget])
Line 340: AC_PATH_PROG([YUM_PATH], [yum], [/usr/bin/yum])
Line 341: 
Line 342: # Keep sorted


http://gerrit.ovirt.org/#/c/37509/3/lib/vdsm/define.py
File lib/vdsm/define.py:

Line 148:         'code': 64,
Line 149:         'message': 'Failed to update ioTune values'}},
Line 150:     'invalidV2VJob': {'status': {
Line 151:         'code': 65,
Line 152:         'message': 'Job Id is not exists'}},
> typo: 'does not exist'
Done
Line 153:     'invalidV2VOvfPath': {'status': {
Line 154:         'code': 66,
Line 155:         'message': 'OVF file is not exists'}},
Line 156:     'recovery': {'status': {


Line 151:         'code': 65,
Line 152:         'message': 'Job Id is not exists'}},
Line 153:     'invalidV2VOvfPath': {'status': {
Line 154:         'code': 66,
Line 155:         'message': 'OVF file is not exists'}},
> same
Done
Line 156:     'recovery': {'status': {
Line 157:         'code': 99,
Line 158:         'message': 'Recovering from crash or Initializing'}},
Line 159: }


http://gerrit.ovirt.org/#/c/37509/3/vdsm/API.py
File vdsm/API.py:

Line 1411:         return {'status': doneCode, 'ovf': ovf}
Line 1412: 
Line 1413:     def abortV2VJob(self, jobId):
Line 1414:         v2v.abort_job(jobId)
Line 1415:         return {'status': doneCode}
> this will always return success, you just ignore any errCode possibly retur
no, I will return it from the v2v.abort_job
Line 1416: 
Line 1417:     # Networking-related functions
Line 1418:     def setupNetworks(self, networks, bondings, options):
Line 1419:         """Add a new network to this vds, replacing an old one."""


http://gerrit.ovirt.org/#/c/37509/3/vdsm/v2v.py
File vdsm/v2v.py:

Line 34: 
Line 35: import caps
Line 36: 
Line 37: 
Line 38: _jobs = {}
> do we need any locking here?
looks like it...
Line 39: _P_V2V_DIR = os.path.join(P_VDSM_RUN, 'v2v')
Line 40: 
Line 41: 
Line 42: class InvalidVMConfiguration(ValueError):


Line 76: 
Line 77: 
Line 78: def convert(uri, username, password, vmProperties, jobId, cif):
Line 79:     global _jobs
Line 80:     t = _import_vm_thread(uri, username, password, vmProperties, jobId, cif)
> pep8 requires that classes should be NamedInCapsLikeThis.
Done,
is a private class should start with _MyClass?
Line 81:     _jobs[jobId] = t
Line 82:     t.start()
Line 83: 
Line 84: 


Line 83: 
Line 84: 
Line 85: def get_converted_vm(jobId):
Line 86:     global _jobs
Line 87:     if jobId not in _jobs.keys():
> just use
Done
Line 88:         return errCode['invalidV2VJob']
Line 89: 
Line 90:     file_name = os.path.join(_P_V2V_DIR, "%s.ovf" % jobId)
Line 91:     if not os.path.exists(file_name):


Line 91:     if not os.path.exists(file_name):
Line 92:         return errCode['invalidV2VOvfPath']
Line 93: 
Line 94:     f = open(file_name, 'r')
Line 95:     with closing(f):
> closing() should not be needed
why not?
who will close the file?
Line 96:         ovf = '\n'.join(f.readlines())
Line 97:     os.remove(file_name)
Line 98: 
Line 99:     _jobs.pop(jobId)


Line 93: 
Line 94:     f = open(file_name, 'r')
Line 95:     with closing(f):
Line 96:         ovf = '\n'.join(f.readlines())
Line 97:     os.remove(file_name)
> I think it is safer to do
ok, how is it best to handle returning a string (the ovf) and raising an error which we want to return to the caller such as: return errCode['invalidV2VOvfPath']
Line 98: 
Line 99:     _jobs.pop(jobId)
Line 100:     if len(_jobs) > 0:
Line 101:         kill_zombie_jobs()


Line 97:     os.remove(file_name)
Line 98: 
Line 99:     _jobs.pop(jobId)
Line 100:     if len(_jobs) > 0:
Line 101:         kill_zombie_jobs()
> why here? this somehow suggest that only one import using virt-v2v may be a
It is called once on vdsm start,
I thought to put it here as well because we are removing a done task.
I don't think it can be in other method such as convert since we are creating a new virt-v2v task.
documented mean comment?
Line 102:     return ovf
Line 103: 
Line 104: 
Line 105: def kill_zombie_jobs():


Line 101:         kill_zombie_jobs()
Line 102:     return ovf
Line 103: 
Line 104: 
Line 105: def kill_zombie_jobs():
> this looks fragile, or maybe it's just me. Doesn't virt-v2v offer any bette
this idea was suggested for case such as vdsm failing while there are running virt-v2v tasks (the startup call will handle that)
or general io errors that can cause virt-v2v to hang.
Line 106:     ret, out, err = execCmd([EXT_PGREP, 'virt-v2v'], sudo=True)
Line 107:     if len(err) > 0:
Line 108:         logging.error('error while trying to grep virt-v2v processes: %r', err)
Line 109:         return


Line 127:     global _jobs
Line 128:     if jobId not in _jobs.keys():
Line 129:         return errCode['invalidV2VJob']
Line 130:     t = _jobs[jobId]
Line 131:     t.abort_job()
> in case of success this returns None, which is unusual and surprising.
Done
Line 132: 
Line 133: 
Line 134: def jobs():
Line 135:     global _jobs


Line 137:     for jobId in _jobs.keys():
Line 138:         ret[jobId] = {}
Line 139:         ret[jobId]['status'] = _jobs[jobId].status
Line 140:         ret[jobId]['progress'] = _jobs[jobId].progress
Line 141:     return ret
> could be made nicer:
Done
Line 142: 
Line 143: 
Line 144: class _import_vm_thread(threading.Thread):
Line 145:     """


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I34bd86d5a87ea8c42113c4a732f87ddd4ceab9ea
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken 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: Shahar Havivi <shavivi 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