Change in vdsm[master]: v2v: Convert VM from external source to Data Domain
danken at redhat.com
danken at redhat.com
Wed Mar 25 16:01:20 UTC 2015
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 <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