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