Change in vdsm[master]: v2v: revactory ImportVM with commnad classes
nsoffer at redhat.com
nsoffer at redhat.com
Sun Dec 6 22:56:54 UTC 2015
Nir Soffer has posted comments on this change.
Change subject: v2v: revactory ImportVM with commnad classes
......................................................................
Patch Set 2:
(10 comments)
Much nicer like this.
https://gerrit.ovirt.org/#/c/49951/2//COMMIT_MSG
Commit Message:
Line 3: AuthorDate: 2015-12-06 15:44:22 +0200
Line 4: Commit: Shahar Havivi <shaharh at redhat.com>
Line 5: CommitDate: 2015-12-06 16:41:52 +0200
Line 6:
Line 7: v2v: revactory ImportVM with commnad classes
refactoring
or more specific: extract specific classes for libvirt and ova
Line 8:
Line 9: We are handling a lot of inputs for ImportVM (libvirt VM, ova) and
Line 10: adding Xen and Kvm which make ImportVM bloated with a lot of conditions.
Line 11:
https://gerrit.ovirt.org/#/c/49951/2/tests/v2vTests.py
File tests/v2vTests.py:
Line 364
Line 365
Line 366
Line 367
Line 368
Do we still need this?
https://gerrit.ovirt.org/#/c/49951/2/vdsm/v2v.py
File vdsm/v2v.py:
Line 360: super(self.__class__, self).__init__(uri, vminfo, job_id, irs)
Line 361: self._username = username
Line 362: self._password = password
Line 363:
Line 364: self._disk_parameters = []
Move up to V2VCommand
Line 365: self._passwd_file = os.path.join(_V2V_DIR, "%s.tmp" % job_id)
Line 366:
Line 367: def command(self):
Line 368: cmd = [_VIRT_V2V.cmd,
Line 385: return cmd
Line 386:
Line 387: @contextmanager
Line 388: def execute(self):
Line 389: self._generate_disk_parameters()
This is common for both, so maybe implement in V2VCommand?
Line 390: self._prepare_volumes()
Line 391: fd = os.open(self._passwd_file, os.O_WRONLY | os.O_CREAT, 0o600)
Line 392: try:
Line 393: os.write(fd, self._password)
Line 386:
Line 387: @contextmanager
Line 388: def execute(self):
Line 389: self._generate_disk_parameters()
Line 390: self._prepare_volumes()
preparing and tearing down volumes is common for all imports, so better keep it in the import vm class.
Line 391: fd = os.open(self._passwd_file, os.O_WRONLY | os.O_CREAT, 0o600)
Line 392: try:
Line 393: os.write(fd, self._password)
Line 394: finally:
Line 387: @contextmanager
Line 388: def execute(self):
Line 389: self._generate_disk_parameters()
Line 390: self._prepare_volumes()
Line 391: fd = os.open(self._passwd_file, os.O_WRONLY | os.O_CREAT, 0o600)
It is better to keep the password context manager separate. You can nest multiple context managers:
with password_file(...), other_context():
yield
Line 392: try:
Line 393: os.write(fd, self._password)
Line 394: finally:
Line 395: os.close(fd)
Line 395: os.close(fd)
Line 396: try:
Line 397: yield
Line 398: finally:
Line 399: self._teardown_volumes()
This is unsafe now, if it raises, we don't clear the password file.
Line 400: try:
Line 401: os.remove(self._passwd_file)
Line 402: except Exception:
Line 403: logging.exception("Job %r error removing passwd file: %s",
Line 426: cmd.extend(self._generate_disk_parameters())
Line 427: return cmd
Line 428:
Line 429: @contextmanager
Line 430: def execute(self):
This is can make sense if you actually execute the command here, and return a process object that import vm can watch.
Line 431: self._generate_disk_parameters()
Line 432: self._prepare_volumes()
Line 433: try:
Line 434: yield
Line 451: self._description = ''
Line 452: self._disk_progress = 0
Line 453: self._disk_count = 1
Line 454: self._current_disk = 1
Line 455: self._aborted = False
This looks so much better now!
Line 456:
Line 457: def start(self):
Line 458: self._thread = concurrent.thread(self._run_command)
Line 459: self._thread.start()
Line 507: self._proc = execCmd(self._command.command(), sync=False,
Line 508: deathSignal=signal.SIGTERM,
Line 509: nice=NICENESS.HIGH,
Line 510: ioclass=IOCLASS.IDLE,
Line 511: env=self._command.environments())
This can work if execCmd is done in self._command.execute():
with self._command.execute() as proc:
# watch it
# wait for it
# check returncode
Line 512:
Line 513: self._proc.blocking = True
Line 514: self._watch_process_output()
Line 515: self._wait_for_process()
--
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: 2
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: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list