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