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