Change in vdsm[master]: v2v: Convert VM from external source to Data Domain
nsoffer at redhat.com
nsoffer at redhat.com
Wed Mar 25 17:11:00 UTC 2015
Nir Soffer has posted comments on this change.
Change subject: v2v: Convert VM from external source to Data Domain
......................................................................
Patch Set 12:
(10 comments)
https://gerrit.ovirt.org/#/c/37509/12/vdsm/v2v.py
File vdsm/v2v.py:
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
Line 69: DISK_COPY = 'copy'
Line 70:
Line 71:
Line 72: class InvalidVMConfiguration(ValueError):
Line 65: STARTING = 'starting'
Line 66: DONE = 'done'
Line 67: ABORT = 'abort'
Line 68: ERROR = 'error'
Line 69: DISK_COPY = 'copy'
For consistency, I would use COPYING or COPYING_DISK
Line 70:
Line 71:
Line 72: class InvalidVMConfiguration(ValueError):
Line 73: ''' Unexpected error while parsing libvirt domain xml '''
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(_jo
Better use tuple(_jobs.items()), since we do not plan to change the returned data.
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 ow
Lets call it "jobid"
Line 212: with _lock:
Line 213: if id not in _jobs:
Line 214: raise NoSuchJob()
Line 215: return _jobs[id]
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.
must be SIGKILL - v2v may ignore SIGTERM
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.
We log this when we handle this error - why log twice?
Line 343: raise V2VProcessError("Process failed: %s" %
Line 344: self._proc.stderr.read(5120))
Line 345: self._status = STATUS.DONE
Line 346:
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?!
Line 345: self._status = STATUS.DONE
Line 346:
Line 347: def _watch_process_output(self):
Line 348: parser = OutputParser()
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 prepar
Isn't this handled in _run already?
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 utte
self is used to get the XXX_RE class variables, and I think that this is a nice way to pack everything.
Without the class, we will have either too big function doing everything, or bunch of helper functions/methods in the module or ImportVm object.
I like this small and simple object - we should have more of these.
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?
readline() will not work:
>>> f = StringIO.StringIO('progress 1\rprogress 2\r progress 2\r')
>>> f.readline()
'progress 1\rprogress 2\r progress 2\r'
It would be nicer to move low level iteration code into parse_progress, and make it also a generator:
def _parse_progress(self, stream):
buf = ''
progress = 0
while progress < 100:
c = stream.read(1)
buf += c
if c == '\r':
m = self.DISK_PROGRESS_RE.match(buf)
if m is None:
raise ...
buf = ''
try:
progress = int(m.group(1))
except ValueError:
raise ...
yield progress
And now parse can do:
for progress in self._parse_progress(stream):
yield DiskProgress(progress)
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