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