Dan Kenigsberg has posted comments on this change.
Change subject: vdsm: Reboot capability for VM
......................................................................
Patch Set 37:
(6 comments)
I find it hard to digest the VmPowerDown class. To me it seems that it could be
replaced by a couple of helper function defined in the shutdown method, with no
additional state beyond the Vm and the chain.
Maybe it would be easier for me to follow if you move the current shutdown
implementation to the callback chain framework, and then add the the reboot
support on another patch.
....................................................
File vdsm/vm.py
Line 1729: self.timeout = max(0, timeout)
Line 1730: self.force = force
Line 1731:
Line 1732: def start(self):
Line 1733: try:
could you narrow this huge catch-all try-block? Which exceptions are you afraid
of?
Line 1734: self._setGuestEvent()
Line 1735: # are there any available methods for shutdown/reboot?
Line 1736: if self.chain.callbacks:
Line 1737: self.chain.start()
Line 1756: self.vm._guestEventTime = time.time()
Line 1757: self.vm._guestEvent = self.desc['guestEvent']
Line 1758:
Line 1759: @classmethod
Line 1760: def create(cls, vm, delay, message, timeout, reboot, force):
why do we need this class method beyond the simple __init__?
Line 1761: graceful_timeout = int(timeout)
Line 1762: agent_user_delay = int(delay)
Line 1763:
Line 1764: # flag for successful power-down event detection
Line 1764: # flag for successful power-down event detection
Line 1765: # this flag is common for both shutdown and reboot workflows
Line 1766: # because we want to exit the CallbackChain in case either
Line 1767: # of them happens
Line 1768: vm._powerDownEvent.clear()
I find it awkward that the VmPowerDown object needs to know about internals of
the Vm object. Can we take move this code to the syncronous part of
Vm.shutdown().
Line 1769: if reboot:
Line 1770: desc = {'action': 'Reboot',
Line 1771: 'guestEvent': 'RebootInProgress',
Line 1772: 'returnMsg': 'Machine rebooting'}
Line 1803: return powerDown
Line 1804:
Line 1805: def _waitForEvent(self, event):
Line 1806: event.wait(self.timeout)
Line 1807: return event.is_set()
isn't this the returned value of wait()? why call again?
Line 1808:
Line 1809: # Callbacks for Callback Chain
Line 1810:
Line 1811: def guestAgentShutdown(self, delay, message):
Line 1809: # Callbacks for Callback Chain
Line 1810:
Line 1811: def guestAgentShutdown(self, delay, message):
Line 1812: self.vm.guestAgent.desktopShutdown(delay, message, False)
Line 1813: time.sleep(self.delay)
Why do we need this explicit delay? A comment will help.
Line 1814: return self._waitForEvent(self.vm._powerDownEvent)
Line 1815:
Line 1816: def guestAgentReboot(self, delay, message):
Line 1817: self.vm.guestAgent.desktopShutdown(delay, message, True)
Line 1838: def forceReboot(self):
Line 1839: self.vm.reset()
Line 1840: return self._waitForEvent(self.vm._powerDownEvent)
Line 1841:
Line 1842: # End Callbacks
is this comment helpful? (not to me)
Line 1843:
Line 1844:
Line 1845: class Vm(object):
Line 1846: """
--
To view, visit http://gerrit.ovirt.org/15829
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I12737e363a80679ffb1db55f14eaee158312d7da
Gerrit-PatchSet: 37
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Betak <[email protected]>
Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Better Saggi <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Giuseppe Vallarelli <[email protected]>
Gerrit-Reviewer: Greg Padgett <[email protected]>
Gerrit-Reviewer: Martin Betak <[email protected]>
Gerrit-Reviewer: Martin Polednik <[email protected]>
Gerrit-Reviewer: Martin Sivák <[email protected]>
Gerrit-Reviewer: Michal Skrivanek <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Peter V. Saveliev <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: Vinzenz Feenstra <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
