Change in vdsm[master]: v2v: Convert VM from external source to Data Domain
fromani at redhat.com
fromani at redhat.com
Fri Feb 13 12:39:50 UTC 2015
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 <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