Dan Kenigsberg has posted comments on this change.
Change subject: vdsm: Reboot capability for VM
......................................................................
Patch Set 22: Code-Review-1
(7 comments)
....................................................
File lib/vdsm/utils.py
Line 25: .. attribute:: utils.symbolerror
Line 26:
Line 27: Contains a reverse dictionary pointing from error string to its
error code.
Line 28: """
Line 29: from collections import namedtuple, deque
Do we really need a deque? Do we expect to append while popping?
Line 30: from SimpleXMLRPCServer import SimpleXMLRPCServer
Line 31: from SimpleXMLRPCServer import SimpleXMLRPCRequestHandler
Line 32: from StringIO import StringIO
Line 33: from weakref import proxy
Line 919: os.killpg(0, 9)
Line 920: sys.exit(-3)
Line 921:
Line 922:
Line 923: class Callback(namedtuple('Callback_', ('func', 'timeout',
'wait_time',
this chaining mechanism deserves a patch of its own, where infra folks can
better review it. would you split it, and provide proper unit test to it?
Line 924: 'args', 'kwargs',
Line 925: 'ignored_exceptions', ))):
Line 926: log = logging.getLogger("utils.Callback")
Line 927:
Line 938: except Exception:
Line 939: self.log.error("%s failed", self.func.__name__,
exc_info=True)
Line 940:
Line 941: thread = threading.Thread(target=threadFunc)
Line 942: thread.start()
non-daemon threads gave us plenty of suffering - I think that vdsm should not
block its exist until the whole chain finishes.
Line 943: thread.join(self.timeout)
Line 944:
Line 945:
Line 946: class CallbackChain(threading.Thread):
Line 952: or it runs out of alternatives.
Line 953: """
Line 954: log = logging.getLogger("utils.CallbackChain")
Line 955:
Line 956: def __init__(self, callbacks=None, checkSuccess=None):
How about avoiding the transient stage of non-callable checkSuccess by having
checkSuccess=lambda: False
here? Similarly,
callbacks=()
seems nicer to me.
Line 957: super(CallbackChain, self).__init__()
Line 958: self.callbacks = deque(callbacks or [])
Line 959: self.checkSuccess = checkSuccess or (lambda: False)
Line 960:
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():
Would you describe the benefit of requiring a checkSuccess function per
callbackChain, instead of stating that each callback should return True on
success and False on failure?
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/guestIF.py
Line 298: self._forward('log-off')
Line 299: except:
Line 300: self.log.error("desktopLogoff failed", exc_info=True)
Line 301:
Line 302: def desktopShutdown(self, timeout, msg, reboot):
shouldn't we better fail the operation if the guest agent does not support
reboot? silently shutting down instead of rebooting may not be what the end
user expects.
Line 303: try:
Line 304: self.log.debug("desktopShutdown called")
Line 305: self._forward('shutdown', {'timeout': timeout, 'message':
msg,
Line 306: 'reboot': reboot})
....................................................
File vdsm/vm.py
Line 1664: return m
Line 1665:
Line 1666:
Line 1667: class VmPowerDown(utils.CallbackChain):
Line 1668: def __init__(self, vm, desc):
When using inheritance, it is expected to keep "a VmPowerDown is a
utils.CallbackChain" valid, which means that you can only add args to your
initiator (or any other method).
Other than that - how about using a well-defined API for __init__, instead of
this generic "bag of args" that is desc?
Line 1669: super(VmPowerDown,
self).__init__(checkSuccess=desc['checkSuccess'])
Line 1670: self.vm = vm
Line 1671: self.desc = desc
Line 1672:
--
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
