Change in vdsm[master]: vdsm: Utilize CallbackChain in vm.shutdown

mbetak at redhat.com mbetak at redhat.com
Wed Mar 5 16:02:37 UTC 2014


Martin Betak has posted comments on this change.

Change subject: vdsm: Utilize CallbackChain in vm.shutdown
......................................................................


Patch Set 3:

(6 comments)

http://gerrit.ovirt.org/#/c/23457/3//COMMIT_MSG
Commit Message:

Line 5: CommitDate: 2014-01-30 16:16:58 +0100
Line 6: 
Line 7: vdsm: Utilize CallbackChain in vm.shutdown
Line 8: 
Line 9: Refactor the existing shutdown/reboot code to utilize the
> That's not just a refactoring: you change the logic. Until now, shutdown (o
Done
Line 10: utils.CallbackChain helper.
Line 11: 
Line 12: Change-Id: I8cc4f4140913df9827cca296e880b37f47a21b76


http://gerrit.ovirt.org/#/c/23457/3/vdsm/vm.py
File vdsm/vm.py:

Line 1841: class MigrationError(Exception):
Line 1842:     pass
Line 1843: 
Line 1844: 
Line 1845: class VmPowerDown(object):
> I am still not convinced that aggregating these method in class is useful. 
In the next patchset I present a proposal that gets rid of the dicts and more cleanly separates shutdown and reboot code. I'm interested in your opinion.
Line 1846:     def __init__(self, vm, delay, message, reboot, timeout):
Line 1847:         self.vm = vm
Line 1848:         self.chain = utils.CallbackChain()
Line 1849:         self.delay = max(0, int(delay))


Line 1845: class VmPowerDown(object):
Line 1846:     def __init__(self, vm, delay, message, reboot, timeout):
Line 1847:         self.vm = vm
Line 1848:         self.chain = utils.CallbackChain()
Line 1849:         self.delay = max(0, int(delay))
> type coercion should happen much higher in the stack. This class should ass
Done
Line 1850:         self.timeout = max(0, int(timeout))
Line 1851: 
Line 1852:         if reboot:
Line 1853:             self.desc = {'guestEvent': 'RebootInProgress',


Line 1851: 
Line 1852:         if reboot:
Line 1853:             self.desc = {'guestEvent': 'RebootInProgress',
Line 1854:                          'returnMsg': 'Machine rebooting'}
Line 1855:             callbacks = {'guestAgent': self.guestAgentReboot,
> What is the benefit of having the "callbacks" dictionary? I think that simp
Done
Line 1856:                          'acpi': self.acpiReboot}
Line 1857:         else:
Line 1858:             self.desc = {'guestEvent': 'Powering down',
Line 1859:                          'returnMsg': 'Machine shutting down'}


Line 1891:                     'message': 'VM without ACPI or active SolidICE tools. '
Line 1892:                                'Try Forced Shutdown.'}}
Line 1893: 
Line 1894:     def _waitForPowerDownEvent(self):
Line 1895:         return self.vm._powerDownEvent.wait(self.timeout)
> I a big fan of small steps.  But I'd like to understand where they are head
Well my main vision was to make the shutdown code more modular and more easily extensible to add the full-featured reboot support (with timeout parameter and forceful callbacks for shutdown/reboot). Further refactoring of vm.py would in my opinion require coordination across virt group and VDSM maintainers.
Line 1896: 
Line 1897:     # Callbacks for Callback Chain
Line 1898: 
Line 1899:     def guestAgentShutdown(self, delay, message):


Line 1897:     # Callbacks for Callback Chain
Line 1898: 
Line 1899:     def guestAgentShutdown(self, delay, message):
Line 1900:         self.vm.guestAgent.desktopShutdown(delay, message, False)
Line 1901:         time.sleep(self.delay)  # delay for user to close his application
> Why do we force this delay-long sleep? why not add it to the wait timeout?
Done
Line 1902:         return self._waitForPowerDownEvent()
Line 1903: 
Line 1904:     def guestAgentReboot(self, delay, message):
Line 1905:         self.vm.guestAgent.desktopShutdown(delay, message, True)


-- 
To view, visit http://gerrit.ovirt.org/23457
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8cc4f4140913df9827cca296e880b37f47a21b76
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Betak <mbetak at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Martin Betak <mbetak at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list