Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

shavivi at redhat.com shavivi at redhat.com
Tue Dec 15 09:39:45 UTC 2015


Shahar Havivi has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
......................................................................


Patch Set 4:

(6 comments)

https://gerrit.ovirt.org/#/c/49951/4/vdsm/v2v.py
File vdsm/v2v.py:

Line 292:         self._prepared_volumes = []
Line 293: 
Line 294:     @property
Line 295:     def id(self):
Line 296:         return self._id
> question: should `id' be tied to the ImportVm object (old code) or to the C
You right,
I will be in both - 
in the command its the vm id
in importVm its the job id
Line 297: 
Line 298:     @property
Line 299:     def proc(self):
Line 300:         return self._proc


Line 309:                                  nice=NICENESS.HIGH,
Line 310:                                  ioclass=IOCLASS.IDLE,
Line 311:                                  env=self.environments())
Line 312: 
Line 313:             yield
> Sure, ImportVm should have proc member, since it manage this process.
Done
Line 314:         finally:
Line 315:             self._teardown_volumes()
Line 316: 
Line 317:     def _get_disk_format(self):


Line 325:             try:
Line 326:                 self._disk_parameters.append('--vdsm-image-uuid')
Line 327:                 self._disk_parameters.append(disk['imageID'])
Line 328:                 self._disk_parameters.append('--vdsm-vol-uuid')
Line 329:                 self._disk_parameters.append(disk['volumeID'])
> Why keep disk_parameters instead of returning them? This is more fragile, a
Done
Line 330:             except KeyError as e:
Line 331:                 raise InvalidInputError('Job %r missing required property: %s'
Line 332:                                         % (self._id, e))
Line 333: 


Line 448:         cmd.extend(self._disk_parameters)
Line 449:         return cmd
Line 450: 
Line 451:     @contextmanager
Line 452:     def execute(self):
> this just invoked the parent's execute(), thus feels redundant and could be
Done
Line 453:         with super(self.__class__, self).execute():
Line 454:             yield
Line 455: 
Line 456: 


Line 460: 
Line 461:     def __init__(self, command):
Line 462:         '''
Line 463:         do not use directly, use a factory method instead!
Line 464:         '''
> stale comment, you killed the factory methods.
Done
Line 465:         self._command = command
Line 466:         self._thread = None
Line 467: 
Line 468:         self._status = STATUS.STARTING


Line 536:                 logging.info('Job %r finished import successfully',
Line 537:                              self._command.id)
Line 538: 
Line 539:     def _wait_for_process(self):
Line 540:         if self._command.proc.returncode is not None:
> This works, but I think this that it would be nicer if the Command classes 
Done
Line 541:             return
Line 542:         logging.debug("Job %r waiting for virt-v2v process", self._command.id)
Line 543:         if not self._command.proc.wait(timeout=self.PROC_WAIT_TIMEOUT):
Line 544:             raise V2VProcessError("Job %r timeout waiting for process pid=%s",


-- 
To view, visit https://gerrit.ovirt.org/49951
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list