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

fromani at redhat.com fromani at redhat.com
Thu Dec 10 16:14:45 UTC 2015


Francesco Romani has posted comments on this change.

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


Patch Set 4:

(5 comments)

partial review, overall looks OK and less scary than the size suggests.
I'll have another look later to be sure I'm not missing something important. Meanwhile, some initial comments.

https://gerrit.ovirt.org/#/c/49951/4/tests/v2vTests.py
File tests/v2vTests.py:

Line 354:         self.assertEquals(network['macAddr'], _mac_from_uuid(spec.vmid))
Line 355:         self.assertEquals(network['bridge'], 'VM Network')
Line 356: 
Line 357:     @MonkeyPatch(v2v, '_VIRT_V2V', FAKE_VIRT_V2V)
Line 358:     @MonkeyPatch(v2v, '_V2V_DIR', '/tmp/')
please check if elsewhere we use NamedTemporaryDir(). If so, please use it as well, is possible (if not, please add a comment explaining why)
Line 359:     def testSuccessfulImport(self):
Line 360:         vminfo = {'vmName': self.vm_name,
Line 361:                   'poolID': self.pool_id,
Line 362:                   'domainID': self.domain_id,


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 Command (new code)?
It seems to me that it belongs more on the ImportVm object, but maybe I'm missing something here.
Line 297: 
Line 298:     @property
Line 299:     def proc(self):
Line 300:         return self._proc


Line 303:     def execute(self):
Line 304:         self._generate_disk_parameters()
Line 305:         self._prepare_volumes()
Line 306:         try:
Line 307:             self._proc = execCmd(self._command(), sync=False,
VERY minor nit

If we need to put the majority of arguments on separate line, I'd bite the bullet and put all of them on its line:

  self._proc = execCmd(self._command(),
                       sync=False,
                       deathSignal=signal.SIGTERM,
                       ...)

but please don't resubmit for such minor thing!
Line 308:                                  deathSignal=signal.SIGTERM,
Line 309:                                  nice=NICENESS.HIGH,
Line 310:                                  ioclass=IOCLASS.IDLE,
Line 311:                                  env=self.environments())


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 dropped
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.
Line 465:         self._command = command
Line 466:         self._thread = None
Line 467: 
Line 468:         self._status = STATUS.STARTING


-- 
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