Francesco Romani has posted comments on this change. Change subject: v2v: Convert VM from external source to Data Domain ......................................................................
Patch Set 3: (12 comments) initial review, I need more time to read and digest all the code. 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 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' 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 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 returned by abort_job. Is that what you want? 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? 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. functions or methods should_use_underscores (they are OK already). 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 if jobId not in _jobs. 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 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 try: file_name = os.path.join(_P_V2V_DIR, "%s.ovf" % jobID) with open(file_name, 'r') as f: ovf = f.read() os.remove(file_name) except IOError as exc: # here you'll need to check for the right value of exc.errno to make sure you properly deal with inexistent files. else: _jobs.pop(jobId) # or maybe simply del _jobs[jobId] if _jobs: kill_zombie_jobs() return ovf Line 98: Line 99: _jobs.pop(jobId) Line 100: if len(_jobs) > 0: Line 101: 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 better way to control this behaviour? 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. It also seems that return value is ignored in API.py 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: def jobs(): global _jobs ret = {} for jobId, jobValue in job.iteritems(): ret[jobId] = { 'status': jobValue.status, 'progress': jobValue.progress } return ret And I think we definitely need some locking to protect _jobs. 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
