Change in vdsm[master]: v2v: Convert VM from external source to Data Domain

danken at redhat.com danken at redhat.com
Tue Feb 17 11:47:34 UTC 2015


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 <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: Saggi Mizrahi <smizrahi at redhat.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