Martin Sivák has posted comments on this change.
Change subject: vdsm: Reboot capability for VM
......................................................................
Patch Set 22:
(6 comments)
Ok, I went through what we have now and it got so much bigger than what I saw
at the beginning. Based on all the new functionality and comments here I would
personally update the code according to my comments in the code.
And just so you know what was the goal before all the architecture comments:
It was a declarative list (table) of tasks needed for reboot/shutdown. I asked
for it to be declarative and include the timeout values and success condition
(with implementation hidden in the chain) so it would be immediately obvious
for anybody who is reading the code what are the needed steps and how long it
will possibly take. The list should have been evaluated by the CallbackChain
engine (we should have really used composition though).
That said I do not really like the design by committee approach we demonstrated
here. It does not work. If it is not perfect but works and there are no really
strong objections we should commit and refactor in the future. Not delay the
functionality in a quest for 100% perfection that won't happen anyway because
all developers have their own way of analyzing algorithms.
....................................................
File lib/vdsm/utils.py
Line 931: self.log.debug('Calling %s with args=%s and
kwargs=%s',
Line 932: self.func.__name__, self.args,
self.kwargs)
Line 933: self.func(*self.args, **self.kwargs)
Line 934: if self.wait_time:
Line 935: time.sleep(self.wait_time)
- remove the wait_time and ignored_exceptions from here
- move the wait time sleep and try/except for ignored exceptions to the
respective callback functions
- create an attribute/property in the thread that holds the result of the func
(success check) and return in from __call__
Line 936: except self.ignored_exceptions:
Line 937: pass
Line 938: except Exception:
Line 939: self.log.error("%s failed", self.func.__name__,
exc_info=True)
Line 959: self.checkSuccess = checkSuccess or (lambda: False)
Line 960:
Line 961: def run(self):
Line 962: self.log.debug("Starting callback chain.")
Line 963: while self.callbacks and not self.checkSuccess():
Dan: Originally it was the easiest way (the check is always the same)
considering the actual callback runs in a separate thread.
Martin:
- Remove checkSuccess from here and use the new Callback return value (3) to
check on result of the last callback.
Line 964: callback = self.callbacks.popleft()
Line 965: callback()
Line 966:
Line 967: # intentionally long name for the timeout arg to decrease the
probability
....................................................
File vdsm/vm.py
Line 1663: m.appendChildWithArgs('target', type='virtio', port='0')
Line 1664: return m
Line 1665:
Line 1666:
Line 1667: class VmPowerDown(utils.CallbackChain):
- Kill the inheritance and add a callback chain instance as an
parameter/attribute. It is not a child anymore..
Line 1668: def __init__(self, vm, desc):
Line 1669: super(VmPowerDown,
self).__init__(checkSuccess=desc['checkSuccess'])
Line 1670: self.vm = vm
Line 1671: self.desc = desc
Line 1672:
Line 1673: def start(self):
Line 1674: try:
Line 1675: self._setGuestEvent()
Line 1676: super(VmPowerDown, self).start()
- This is the only place you call anything from CallbackChain from inside the
instance. So just change it to self.chain.start()
Line 1677:
Line 1678: return {'status': {'code': doneCode['code'],
Line 1679: 'message': self.desc['returnMsg']}}
Line 1680: except Exception:
Line 1711:
vm._dom.shutdownFlags(libvirt.VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN)
Line 1712:
Line 1713: def acpiReboot():
Line 1714: vm._dom.reboot(libvirt.VIR_DOMAIN_REBOOT_ACPI_POWER_BTN)
Line 1715:
- Add sleeps and try/except for ignoring exceptions here, also make each of the
callback return the result of the respective check success part (it is just a
value anyway).
Line 1716: # End Callbacks
Line 1717:
Line 1718: if reboot:
Line 1719: # flag for successful reboot detection
Line 1736: 'acpi': acpiShutdown,
Line 1737: 'force': vm.destroy,
Line 1738: 'checkSuccess': lambda: vm.lastStatus == 'Down'}
Line 1739:
Line 1740: chain = VmPowerDown(vm, desc)
- Create two instances (CallbackChain and VmPowerDown) and pass the first one
to the other.
- Also use two separate dictionaries for guestEvent and CallbackChain config.
There is no need to pass the functions to vmPowerdown, is it?
Line 1741:
Line 1742: sys_timeout = config.getint('vars', 'sys_shutdown_timeout')
Line 1743: agent_timeout = int(timeout) + sys_timeout
Line 1744:
--
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: 22
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Betak <[email protected]>
Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Giuseppe Vallarelli <[email protected]>
Gerrit-Reviewer: Martin Betak <[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: 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
