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