Dan Kenigsberg has posted comments on this change. Change subject: v2v: Convert VM from external source to Data Domain ......................................................................
Patch Set 4: Code-Review-1 (3 comments) partial review http://gerrit.ovirt.org/#/c/37509/4/vdsm/v2v.py File vdsm/v2v.py: Line 92: return errCode['invalidV2VJob'] Line 93: Line 94: try: Line 95: file_name = os.path.join(_P_V2V_DIR, "%s.ovf" % jobId) Line 96: with open(file_name, 'r') as f: this removes partially-written ovfs. We must first verify that the job at hand finished successfully. We must first check if the virt-v2v command returned with a 0 return code. Line 97: ovf = f.read() Line 98: os.remove(file_name) Line 99: except IOError as exc: Line 100: if exc.errno == errno.ENOENT: Line 206: self._vmProperties['vmName']) Line 207: Line 208: logging.info('import vm, (jobId %s) started, cmd: %r', self._job_id, Line 209: cmd) Line 210: proc = subprocess.Popen(cmd, env={'LIBGUESTFS_BACKEND': 'direct'}, > we'd probably need some input from infra here to make sure we are talking w yes, you should use utils.execCmd(), and must never touch shell=True. deathSingnal=15 would make sure that no stale processes remain after a vdsm crash. Line 211: shell=True, Line 212: stdout=subprocess.PIPE, Line 213: stderr=subprocess.PIPE) Line 214: Line 313: 'domainID': self._vmProperties['domainID'], Line 314: 'imageID': disk['imageID'], Line 315: 'volumeID': disk['volumeID']} Line 316: Line 317: volPath = self._cif.getInstance().prepareVolumePath(drive) if you pass ._cif all the way into here, you don't need getInstance. I don't know if its any better, but you could from clientIf import clientIf clientIf.getInstance().... Better call cif.irs.prepareImage() directly, without the virt-only wrapper. remember to teardown the image afterwards. Line 318: volPath = volPath.split('/images/')[0] Line 319: return volPath Line 320: Line 321: -- 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: 4 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: Saggi Mizrahi <[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
