Dan Kenigsberg has posted comments on this change. Change subject: supervdsmServer is down after failed operation (#846307) ......................................................................
Patch Set 2: I would prefer that you didn't submit this (3 inline comments) Please add a unit test that shows that supervdsm handles IOError-raising functions properly. .................................................... Commit Message Line 5: CommitDate: 2012-09-10 14:32:31 +0300 Line 6: Line 7: supervdsmServer is down after failed operation (#846307) Line 8: Line 9: https://bugzilla.redhat.com/show_bug.cgi?id=846307 please add the Bug-Id: tag in front. No need to mention the BZ# in the title, too. Line 10: Line 11: After running operation that throws exception that we don't catch in Line 12: supervdsmServer, we catch it in ProxyCaller::__call__ method. Line 13: In this except code we reset supervdsmServer and call the same method .................................................... File vdsm/supervdsm.py Line 64: getattr(self._supervdsmProxy._svdsm, self._funcName)(*args, Line 65: **kwargs) Line 66: try: Line 67: return callMethod() Line 68: except (IOError, socket.error, AuthenticationError): the root problem is not touched. If the called method raises an IOError, it is *not* a good reason for supervdsm to die. The process should be restarted only when the supervdsm framework has a bug, not on bugs in its payload. Line 69: self._supervdsmProxy._restartSupervdsm() Line 70: Line 71: Line 72: class SuperVdsmProxy(object): Line 65: **kwargs) Line 66: try: Line 67: return callMethod() Line 68: except (IOError, socket.error, AuthenticationError): Line 69: self._supervdsmProxy._restartSupervdsm() silently returning None is a very bad idea in case of an error. Raising an exception would be more reasonable. Line 70: Line 71: Line 72: class SuperVdsmProxy(object): Line 73: """ -- To view, visit http://gerrit.ovirt.org/7901 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idad4a622b82259b777851d1b0c1b37ec8da2b01e Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: Barak Azulay <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Douglas Schilling Landgraf <[email protected]> Gerrit-Reviewer: Eduardo <[email protected]> Gerrit-Reviewer: Igor Lvovsky <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: Yeela Kaplan <[email protected]> _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
