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

shavivi at redhat.com shavivi at redhat.com
Wed Mar 25 14:54:02 UTC 2015


Shahar Havivi has posted comments on this change.

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


Patch Set 11:

(14 comments)

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

Line 153: 13
> Please add more progress data, this does not test the progress parsing code
this is not the progress check,
the parse_progress in line 164 is checking the progress.
I will add another disk for the test


Line 155: 256
> What is 256.0 - time from start of the operation?
seconds that pass since the start


Line 163: 
Line 164:         parser.parse_progress('    (13.16/100%)')
Line 165:         self.assertEquals(importVm.progress, 13)
Line 166:         parser.parse_progress('    (100/100%)')
Line 167:         self.assertEquals(importVm.progress, 100)
> We should convert the parser to generator, accepting lines from v2v process
Very nice code,
Done


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

Line 91: 
Line 92: 
Line 93: class NoSuchOvf(V2VJobError):
Line 94:     ''' Ovf path is not exists in /var/run/vdsm/v2v/ '''
Line 95:     err_name = 'invalidV2VOvfPath'
> I think we should fix also the error name.
Done
Line 96: 
Line 97: 
Line 98: class VolumeError(RuntimeError):
Line 99:     def __str__(self):


Line 260:         self._preparedVolumes = []
Line 261:         self._parser = OutputParser(self)
Line 262:         self._pass_file = os.path.join(_V2V_DIR, "%s.tmp" % jobId)
Line 263:         self.ABORT_RETRIES = 3
Line 264:         self.ABORT_DELAY = 30
> Should be class variable, why create another instance variable for each job
Done
Line 265: 
Line 266:     def start(self):
Line 267:         self._thread = threading.Thread(target=self._run)
Line 268:         self._thread.daemon = True


Line 330: 
Line 331:         if self._proc.returncode != 0:
Line 332:             self._status = STATUS.ERROR
Line 333:             error = self._get_process_errors()
Line 334:             self._status_msg = error
> You don't need to set self._status_msg as it is done in _run. This is the b
Done
Line 335:             raise V2VProcessError(error)
Line 336:         self._status = STATUS.DONE
Line 337: 
Line 338:     def disk_copy_stage(self, status_msg, current_disk, disk_count):


Line 331:         if self._proc.returncode != 0:
Line 332:             self._status = STATUS.ERROR
Line 333:             error = self._get_process_errors()
Line 334:             self._status_msg = error
Line 335:             raise V2VProcessError(error)
> So this can simplified to:
Done
Line 336:         self._status = STATUS.DONE
Line 337: 
Line 338:     def disk_copy_stage(self, status_msg, current_disk, disk_count):
Line 339:         self._status = STATUS.DISK_COPY


Line 346:     def progress_stage(self, progress):
Line 347:         try:
Line 348:             self._disk_progress = int(progress)
Line 349:         except ValueError:
Line 350:             self._abort()
> If this exception is bubbled up to _run - we can let _run do the abort.
Done
Line 351:             raise JobProgressError("Error parsing progress: %r" % progress)
Line 352: 
Line 353:     def _watch_process_output(self):
Line 354:         while not self._aborted and self._proc.returncode is None:


Line 371:     def _get_process_errors(self):
Line 372:         error = self._proc.stderr.readlines()
Line 373:         if len(error) > 0:
Line 374:             msg = 'error process ended with errors: %s' % error
Line 375:             return msg
> This will return None if there are no errors, so we don't get any context i
I don't know the exact number too since the error can be from external source such as qemu-img, 
I would go with 5120
Line 376: 
Line 377:     def _create_v2v_dir(self):
Line 378:         try:
Line 379:             os.mkdir(_V2V_DIR, 0o700)


Line 421:                                       self._proc.returncode)
Line 422:                         return
Line 423:                     raise
Line 424:                 time.sleep(self.ABORT_DELAY)
Line 425:             logging.error("Error killing virt-v2v (pid: %d)", self._proc.pid)
> If we failed, we should pass the pid to zombiereaper, so it would reap the 
Done
Line 426: 
Line 427:     def _abort_msg(self, msg):
Line 428:         self._status_msg = msg
Line 429:         logging.error(msg)


Line 429: logging
> Why do we need this method now? self._status_msg is set in _run and the mes
Done


Line 462:         ''' prepareImage returns /prefix/sdUUID/images/imgUUID/volUUID
Line 463:             we need storage domain absolute path so we go up 3 levels '''
Line 464:         path = os.path.split(path)
Line 465:         path = os.path.split(path)
Line 466:         return os.path.split(path)
> This is still broken - see my comment for version 10.
Done
Line 467: 
Line 468:     def _teardown_volumes(self):
Line 469:         for drive in self._preparedVolumes:
Line 470:             try:


Line 489: unexpected format in "Copying disk"
> Please include the unexpected line in the error message. Otherwise we will 
Done


Line 497: JobOutputParsing
> Please include the unexpected buf in the error message. Otherwise we will h
Done


-- 
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: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi 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