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

shavivi at redhat.com shavivi at redhat.com
Thu Mar 26 14:34:28 UTC 2015


Shahar Havivi has posted comments on this change.

Change subject: v2v: Convert VM from external source to Data Domain
......................................................................


Patch Set 12:

(33 comments)

https://gerrit.ovirt.org/#/c/37509/12/tests/v2vTests.py
File tests/v2vTests.py:

Line 102: 
Line 103:     def disk_copy_stage(self, msg, disk, count):
Line 104:         self.status_msg = msg
Line 105:         self.current_disk = disk
Line 106:         self.disk_count = count
> We don't use this now - right?
yes, my bad..
Line 107: 
Line 108: 
Line 109: class v2vTests(TestCaseBase):
Line 110:     @MonkeyPatch(libvirtconnection, 'open_connection', hypervisorConnect)


Line 132:     def testOutputParser(self):
Line 133:         output = '''[   0.0] Opening the source -i libvirt -ic vpx://roo...
Line 134: [   1.0] Creating an overlay to protect the source from being modified
Line 135: [  88.0] Copying disk 1/2 to /tmp/v2v/00000002-0002-0002-0002-000000000...
Line 136:     (0/100%)\r
> This is incorrect, since when using tripple quoted string in python, you ge
Done
Line 137:     (50/100%)\r
Line 138:     (100/100%)\r
Line 139: [ 180.0] Copying disk 2/2 to /tmp/v2v/10000002-0002-0002-0002-000000000...
Line 140:     (0/100%)\r


https://gerrit.ovirt.org/#/c/37509/12/vdsm/v2v.py
File vdsm/v2v.py:

Line 61: 
Line 62: 
Line 63: class STATUS:
Line 64:     INITIALIZING = 'initializing'
Line 65:     STARTING = 'starting'
> We don't
we don't
we didn't had that
you guys suggested that
I will fix again
Line 66:     DONE = 'done'
Line 67:     ABORT = 'abort'
Line 68:     ERROR = 'error'
Line 69:     DISK_COPY = 'copy'


Line 63: class STATUS:
Line 64:     INITIALIZING = 'initializing'
Line 65:     STARTING = 'starting'
Line 66:     DONE = 'done'
Line 67:     ABORT = 'abort'
> I thinked ABORTED is better English.
Done
Line 68:     ERROR = 'error'
Line 69:     DISK_COPY = 'copy'
Line 70: 
Line 71: 


Line 64:     INITIALIZING = 'initializing'
Line 65:     STARTING = 'starting'
Line 66:     DONE = 'done'
Line 67:     ABORT = 'abort'
Line 68:     ERROR = 'error'
> If we use ABORTTED, then we should use FAILED instead of ERROR
Done
Line 69:     DISK_COPY = 'copy'
Line 70: 
Line 71: 
Line 72: class InvalidVMConfiguration(ValueError):


Line 133:                                                  passwd=password)
Line 134:     except libvirt.libvirtError as e:
Line 135:         logging.error('error connection to hypervisor: %r', e.message)
Line 136:         return {'status': {'code': errCode['V2VConnection']['status']['code'],
Line 137:                 'message': e.message}}
> Not related, please remove this change.
Done
Line 138: 
Line 139:     with closing(conn):
Line 140:         vms = []
Line 141:         for vm in conn.listAllDomains():


Line 168:     try:
Line 169:         job = _get_job(jobId)
Line 170:         _validate_job_done(job)
Line 171:         ovf = _read_ovf(jobId)
Line 172:     except V2VJobError as e:
> We should log here the error, so we don't need to log every failure in the 
Done
Line 173:         return errCode[e.err_name]
Line 174:     return {'status': doneCode, 'ovf': ovf}
Line 175: 
Line 176: 


Line 178:     try:
Line 179:         job = _get_job(jobId)
Line 180:         _validate_job_finished(job)
Line 181:         _remove_job(jobId)
Line 182:     except V2VJobError as e:
> Log the error.
Done
Line 183:         return errCode[e.err_name]
Line 184:     return {'status': doneCode}
Line 185: 
Line 186: 


Line 188:     try:
Line 189:         job = _get_job(jobId)
Line 190:         job.abort()
Line 191:         _remove_job(jobId)
Line 192:     except V2VJobError as e:
> Log the error
Done
Line 193:         return errCode[e.err_name]
Line 194:     return {'status': doneCode}
Line 195: 
Line 196: 


Line 196: 
Line 197: def jobs():
Line 198:     ret = {}
Line 199:     with _lock:
Line 200:         items = _jobs.items()
> Better use tuple(_jobs.items()), since we do not plan to change the returne
Done
Line 201:     for jobId, job in items:
Line 202:         ret[jobId] = {
Line 203:             'status': job.status,
Line 204:             'status_msg': job.status_msg,


Line 207:         }
Line 208:     return ret
Line 209: 
Line 210: 
Line 211: def _get_job(id):
> Lets call it "jobid"
Done
Line 212:     with _lock:
Line 213:         if id not in _jobs:
Line 214:             raise NoSuchJob()
Line 215:         return _jobs[id]


Line 210: 
Line 211: def _get_job(id):
Line 212:     with _lock:
Line 213:         if id not in _jobs:
Line 214:             raise NoSuchJob()
> Error message:
Done
Line 215:         return _jobs[id]
Line 216: 
Line 217: 
Line 218: def _add_job(id, job):


Line 217: 
Line 218: def _add_job(id, job):
Line 219:     with _lock:
Line 220:         if id in _jobs:
Line 221:             raise JobExists()
> Error message:
Done
Line 222:         _jobs[id] = job
Line 223: 
Line 224: 
Line 225: def _remove_job(id):


Line 224: 
Line 225: def _remove_job(id):
Line 226:     with _lock:
Line 227:         if id not in _jobs:
Line 228:             raise NoSuchJob()
> Error message:
Done
Line 229:         del _jobs[id]
Line 230: 
Line 231: 
Line 232: def _validate_job_done(job):


Line 230: 
Line 231: 
Line 232: def _validate_job_done(job):
Line 233:     if job.status != STATUS.DONE:
Line 234:         raise JobNotDone()
> Please use an error message:
Done
Line 235: 
Line 236: 
Line 237: def _validate_job_finished(job):
Line 238:     if job.status not in (STATUS.DONE, STATUS.ERROR, STATUS.ABORT):


Line 235: 
Line 236: 
Line 237: def _validate_job_finished(job):
Line 238:     if job.status not in (STATUS.DONE, STATUS.ERROR, STATUS.ABORT):
Line 239:         raise JobNotDone()
> Please use an error message:
Done
Line 240: 
Line 241: 
Line 242: def _read_ovf(jobId):
Line 243:     file_name = os.path.join(_V2V_DIR, "%s.ovf" % jobId)


Line 248:         if e.errno == errno.ENOENT:
Line 249:             raise NoSuchOvf()
Line 250:         else:
Line 251:             logging.error('Error reading file "%r" error: %s message %s',
Line 252:                           file_name, e.errno, e.message)
> We don't have to log here, logging should be done in the upper level, once 
Done
Line 253:             raise
Line 254: 
Line 255: 
Line 256: class ImportVm(object):


Line 249:             raise NoSuchOvf()
Line 250:         else:
Line 251:             logging.error('Error reading file "%r" error: %s message %s',
Line 252:                           file_name, e.errno, e.message)
Line 253:             raise
> Better switch the order (as I suggested in a previous version):
Done
Line 254: 
Line 255: 
Line 256: class ImportVm(object):
Line 257:     ABORT_RETRIES = 3


Line 256: class ImportVm(object):
Line 257:     ABORT_RETRIES = 3
Line 258:     ABORT_DELAY = 10
Line 259: 
Line 260:     def __init__(self, uri, username, password, vmProperties, jobId, cif):
> vmProperties -> vm_properties
Done
Line 261:         self._uri = uri
Line 262:         self._username = username
Line 263:         self._password = password
Line 264:         self._vmProperties = vmProperties


Line 260:     def __init__(self, uri, username, password, vmProperties, jobId, cif):
Line 261:         self._uri = uri
Line 262:         self._username = username
Line 263:         self._password = password
Line 264:         self._vmProperties = vmProperties
> self._vmProperties -> self._vm_properties
Done
Line 265:         self.id = jobId
Line 266:         self._cif = cif
Line 267:         self._status = STATUS.INITIALIZING
Line 268:         self._disk_progress = 0


Line 269:         self._disk_count = 1
Line 270:         self._current_disk = 1
Line 271:         self._aborted = False
Line 272:         self._status_msg = ''
Line 273:         self._preparedVolumes = []
> in this new module, please use pep8-complient names, such as self._prepared
Done
Line 274:         self._pass_file = os.path.join(_V2V_DIR, "%s.tmp" % jobId)
Line 275: 
Line 276:     def start(self):
Line 277:         self._thread = threading.Thread(target=self._run)


Line 295:         ''' progress is part of multiple disk_progress its
Line 296:             flat and not 100% accurate - each disk take its
Line 297:             portion ie if we have 2 disks the first will take
Line 298:             0-50 and the second 50-100
Line 299:         '''
> Use standard docstring:
Done
Line 300:         completed = (self._disk_count - 1) * 100
Line 301:         return (completed + self._disk_progress) / self._disk_count
Line 302: 
Line 303:     @traceback(msg="Error importing vm")


Line 340: 
Line 341:         if self._proc.returncode != 0:
Line 342:             self._status = STATUS.ERROR
Line 343:             raise V2VProcessError("Process failed: %s" %
Line 344:                                   self._proc.stderr.read(5120))
> 5MB error message?!
only 64 full lines
Line 345:         self._status = STATUS.DONE
Line 346: 
Line 347:     def _watch_process_output(self):
Line 348:         parser = OutputParser()


Line 409:             logging.error("Error killing virt-v2v (pid: %d)", self._proc.pid)
Line 410:             zombiereaper.autoReapPID(self._proc.pid)
Line 411: 
Line 412:     def _generate_disk_parameters(self):
Line 413:         images = []
> images -> parameters
Done
Line 414:         for disk in self._vmProperties['disks']:
Line 415:             if 'imageID' in disk:
Line 416:                 images.append('--vdsm-image-uuid')
Line 417:                 images.append(disk['imageID'])


Line 416:                 images.append('--vdsm-image-uuid')
Line 417:                 images.append(disk['imageID'])
Line 418:             if 'volumeID' in disk:
Line 419:                 images.append('--vdsm-vol-uuid')
Line 420:                 images.append(disk['volumeID'])
> Is it possible that disk does not contain both 'imageID' and 'volumeID'?
no its not possible
Line 421:             return images
Line 422: 
Line 423:     def _prepare_volumes(self):
Line 424:         ''' method prepare the images and return storage domain mounted path


Line 422: 
Line 423:     def _prepare_volumes(self):
Line 424:         ''' method prepare the images and return storage domain mounted path
Line 425:             since all images are in the same domain we return arbitrary image
Line 426:             for the return path (res['path']) '''
> This is non-standard docstring format. Please use the common format in vdsm
Done
Line 427:         for disk in self._vmProperties['disks']:
Line 428:             res = self._cif.irs.prepareImage(self._vmProperties['domainID'],
Line 429:                                              self._vmProperties['poolID'],
Line 430:                                              disk['imageID'], disk['volumeID'])


Line 429:                                              self._vmProperties['poolID'],
Line 430:                                              disk['imageID'], disk['volumeID'])
Line 431:             drive = {'poolID': self._vmProperties['poolID'],
Line 432:                      'domainID': self._vmProperties['domainID'],
Line 433:                      'imageID': disk['imageID']}
> We are copying values from _vmProperties twice for no reason. Lets create a
Done
Line 434:             if res['status']['code']:
Line 435:                 self._status = STATUS.ERROR
Line 436:                 self._status_msg = 'Bad volume specification: %s' % drive
Line 437:                 raise VolumeError(drive)


Line 433:                      'imageID': disk['imageID']}
Line 434:             if res['status']['code']:
Line 435:                 self._status = STATUS.ERROR
Line 436:                 self._status_msg = 'Bad volume specification: %s' % drive
Line 437:                 raise VolumeError(drive)
> Please eliminate all lines setting self._status and self._status_msg.
Done
Line 438:             self._preparedVolumes.append([drive])
Line 439: 
Line 440:         return self._extract_storage_path(self._preparedVolumes[0]['path'])
Line 441: 


Line 440:         return self._extract_storage_path(self._preparedVolumes[0]['path'])
Line 441: 
Line 442:     def _extract_storage_path(self, path):
Line 443:         ''' prepareImage returns /prefix/sdUUID/images/imgUUID/volUUID
Line 444:             we need storage domain absolute path so we go up 3 levels '''
> Use standard docstring (see above)
Done
Line 445:         path = os.path.split(path)
Line 446:         path = os.path.split(path[0])
Line 447:         return os.path.split(path[0])[0]
Line 448: 


Line 443:         ''' prepareImage returns /prefix/sdUUID/images/imgUUID/volUUID
Line 444:             we need storage domain absolute path so we go up 3 levels '''
Line 445:         path = os.path.split(path)
Line 446:         path = os.path.split(path[0])
Line 447:         return os.path.split(path[0])[0]
> This works, but is more complicated than needed- why not:
Dan?
Line 448: 
Line 449:     def _teardown_volumes(self):
Line 450:         for drive in self._preparedVolumes:
Line 451:             try:


Line 450:         for drive in self._preparedVolumes:
Line 451:             try:
Line 452:                 self._cif.teardownImage(drive['domainID'],
Line 453:                                         drive['poolID'],
Line 454:                                         drive['imageID'])
> There is no such method. You mean:
Done
Line 455:             except Exception as e:
Line 456:                 logging.error('Error teardownVolumePath: %s', e)
Line 457: 
Line 458: 


Line 452:                 self._cif.teardownImage(drive['domainID'],
Line 453:                                         drive['poolID'],
Line 454:                                         drive['imageID'])
Line 455:             except Exception as e:
Line 456:                 logging.error('Error teardownVolumePath: %s', e)
> We don't call here teardownVolumePath.
Done
Line 457: 
Line 458: 
Line 459: class OutputParser(object):
Line 460:     COPY_DISK_RE = re.compile(r'.*(Copying disk (\d+)/(\d+)).*')


Line 467:                 yield ImportProgress(int(current_disk), int(disk_count),
Line 468:                                      description)
Line 469:                 while True:
Line 470:                     buf = ""
Line 471:                     while '\r' not in buf:
> readline() will not work:
done as described in v2vTests.py
Line 472:                         buf += stream.read(1)
Line 473:                     progress = int(self._parse_progress(buf))
Line 474:                     yield DiskProgress(progress)
Line 475:                     if progress == 100:


-- 
To view, visit https://gerrit.ovirt.org/37509
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I34bd86d5a87ea8c42113c4a732f87ddd4ceab9ea
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce 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: Yaniv Bronhaim <ybronhei 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