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