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 <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Shahar Havivi <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
