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 <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[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: Yaniv Bronhaim <[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
