Dan Kenigsberg has posted comments on this change. Change subject: v2v: Convert VM from external source to Data Domain ......................................................................
Patch Set 12: Code-Review-1 (10 comments) partial review 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' Do we really need to differentiate between "initializing" and "starting"? Why? What each of the states mean? Could you add a docstring? 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. Line 68: ERROR = 'error' Line 69: DISK_COPY = 'copy' Line 70: Line 71: Line 196: Line 197: def jobs(): Line 198: ret = {} Line 199: with _lock: Line 200: items = _jobs.items() in Python3 dict.items() returns a iterator. To force list copy use list(_jobs.items()). 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): id() is a built-in function in python. Please do not shadow it with your own argument name. Line 212: with _lock: Line 213: if id not in _jobs: Line 214: raise NoSuchJob() Line 215: return _jobs[id] 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_volumes, vm_properties, job_id 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 331: self._vmProperties['vmName']]) Line 332: Line 333: logging.info('Import vm, (jobId %s) started, cmd: %s', self.id, cmd) Line 334: Line 335: self._proc = execCmd(cmd, sync=False, deathSignal=15, use signal.SIGTERM instead of number. But why not SIGKILL? Line 336: env={'LIBGUESTFS_BACKEND': 'direct'}) Line 337: Line 338: self._proc.blocking = True Line 339: self._watch_process_output() Line 338: self._proc.blocking = True Line 339: self._watch_process_output() Line 340: Line 341: if self._proc.returncode != 0: Line 342: self._status = STATUS.ERROR Please log returncode and stderr. Line 343: raise V2VProcessError("Process failed: %s" % Line 344: self._proc.stderr.read(5120)) Line 345: self._status = STATUS.DONE Line 346: 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 438: self._preparedVolumes.append([drive]) on exception, we should tear down those volumes that we have already prepared. Line 439: Line 440: return self._extract_storage_path(self._preparedVolumes[0]['path']) Line 441: Line 442: def _extract_storage_path(self, path): Line 455: except Exception as e: Line 456: logging.error('Error teardownVolumePath: %s', e) Line 457: Line 458: Line 459: class OutputParser(object): This class has no data members. Why is it a class at all? "self" means utterly nothing. Please make the parse function stand-alone. Also, it would be simpler to review the code if you introduce this parsing functions (and their patches) in a separate patch. Line 460: COPY_DISK_RE = re.compile(r'.*(Copying disk (\d+)/(\d+)).*') Line 461: DISK_PROGRESS_RE = re.compile(r'\s+\((\d+).*') Line 462: Line 463: def parse(self, stream): 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: this seems very clunky. stream.readline() is buggy? even if so, the following is a bit less wasteful: while True: c = read(1) bug += c if c == \r: break 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 <[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
