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

nsoffer at redhat.com nsoffer at redhat.com
Fri Dec 18 01:57:11 UTC 2015


Nir Soffer has posted comments on this change.

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


Patch Set 7:

(8 comments)

I like this, just few tweaks and it is ready.

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

Line 284:         self._uri = uri
Line 285:         self._vminfo = vminfo
Line 286:         self._vmid = vmid
Line 287:         self._irs = irs
Line 288: 
Remove the blank line
Line 289:         self._prepared_volumes = []
Line 290: 
Line 291:     def execute(self):
Line 292:         """


Line 290: 
Line 291:     def execute(self):
Line 292:         """
Line 293:         implement in subclasses
Line 294:         """
To make it clear that this is abstract class, and subclass must implement this, do:

    def execute(self):
        raise NotImplementedError("Subclass must implement this")
Line 295: 
Line 296:     def _get_disk_format(self):
Line 297:         fmt = self._vminfo.get('format', 'raw').lower()
Line 298:         if fmt == 'cow':


Line 296:     def _get_disk_format(self):
Line 297:         fmt = self._vminfo.get('format', 'raw').lower()
Line 298:         if fmt == 'cow':
Line 299:             return 'qcow2'
Line 300:         return fmt
Can simplified to:

    fmt = self._vminfo.get('format', 'raw').lower()
    return "qcow2" if fmt == "cow" else fmt
Line 301: 
Line 302:     def _generate_disk_parameters(self):
Line 303:         parameters = []
Line 304:         for disk in self._vminfo['disks']:


Line 298:         if fmt == 'cow':
Line 299:             return 'qcow2'
Line 300:         return fmt
Line 301: 
Line 302:     def _generate_disk_parameters(self):
I would call it _disk_parameters()
Line 303:         parameters = []
Line 304:         for disk in self._vminfo['disks']:
Line 305:             try:
Line 306:                 parameters.append('--vdsm-image-uuid')


Line 312:                                         % (self._vmid, e))
Line 313:         return parameters
Line 314: 
Line 315:     @contextmanager
Line 316:     def _volumes_handler(self):
Why _handler? Why not call it _volumes()?
Line 317:         self._prepare_volumes()
Line 318:         try:
Line 319:             yield
Line 320:         finally:


Line 400:                            sync=False,
Line 401:                            deathSignal=signal.SIGTERM,
Line 402:                            nice=NICENESS.HIGH,
Line 403:                            ioclass=IOCLASS.IDLE,
Line 404:                            env=self._environments())
This block is indentical in both classes, so better move it to super class, mayb call it _start_virt_v2v()

Then this command will do:

    with self._volumes_handler(), self._password_file():
        yield self._start_virt_v2v()

And the ova command:

    with self._volumes_handler():
        yield self._start_virt_v2v()

The xen commnd will probably do:

    with self._volumes_handler(), self._ssh_agent():
        yield self._start_virt_v2v()
Line 405:             yield proc
Line 406: 
Line 407:     @contextmanager
Line 408:     def _password_file(self):


Line 550:             if isinstance(event, ImportProgress):
Line 551:                 self._status = STATUS.COPYING_DISK
Line 552:                 logging.info("Job %r copying disk %d/%d",
Line 553:                              self._id, event.current_disk,
Line 554:                              event.disk_count)
Unrelated change
Line 555:                 self._disk_progress = 0
Line 556:                 self._current_disk = event.current_disk
Line 557:                 self._disk_count = event.disk_count
Line 558:                 self._description = event.description


Line 560:                 self._disk_progress = event.progress
Line 561:                 if event.progress % 10 == 0:
Line 562:                     logging.info("Job %r copy disk %d progress %d/100",
Line 563:                                  self._id, self._current_disk,
Line 564:                                  event.progress)
Unrelated change
Line 565:             else:
Line 566:                 raise RuntimeError("Job %r got unexpected parser event: %s" %
Line 567:                                    (self._id, event))
Line 568: 


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