Change in vdsm[master]: [WIP]normalize startup: seperate supervdsm framework err and...

ybronhei at redhat.com ybronhei at redhat.com
Thu Jan 10 10:13:55 UTC 2013


Yaniv Bronhaim has posted comments on this change.

Change subject: [WIP]normalize startup: seperate supervdsm framework err and function err
......................................................................


Patch Set 3: I would prefer that you didn't submit this

(4 inline comments)

how is it better then current implementation? the split between framework error and call error was handled also in previous version.

not sure if it adds something, but if you and one more will think that this implementation is better or more clear and understandable than the current version i will accept it

....................................................
File vdsm/supervdsm.py
Line 72:             with self._supervdsmProxy.proxyLock:
Line 73:                 if not self._supervdsmProxy.isFunctional():
Line 74:                     self._supervdsmProxy.kill()
Line 75:                     self._supervdsmProxy.launch()
Line 76:             raise
shouldn't you call the method again after launch?
Line 77:         if ex:
Line 78:             raise ex
Line 79:         else:
Line 80:             return ret


Line 127: 
Line 128:     def kill(self):
Line 129:         try:
Line 130:             if self._isSvdsmAlive():
Line 131:                 with open(self.pidfile, "r") as f:
you already read it, you can get it from _isSvdsmAlive as a return value
Line 132:                     pid = int(f.read().strip())
Line 133:                 misc.execCmd([constants.EXT_KILL, "-9", str(pid)], sudo=True)
Line 134:         except Exception:
Line 135:             self._log.warn("Could not kill old Super Vdsm %s",


Line 146:                 spid = f.read().strip()
Line 147:             with open(self.timestamp, "r") as f:
Line 148:                 createdTime = f.read().strip()
Line 149:         except IOError as e:
Line 150:             # pid file and timestamp file must be exist after first launch,
you omit the first launch variable.. and i still think it useful. this is my way to distinguish if someone deleted our svdsm files manually, or the files weren't exist before we got here (possible only in first call).
Line 151:             # otherwise excpetion will be raised to svdsm caller
Line 152:             if e.errno == ENOENT:
Line 153:                 return False
Line 154:             else:


....................................................
File vdsm/supervdsmServer.py
Line 80:         try:
Line 81:             ret = func(*args, **kwargs)
Line 82:         except Exception:
Line 83:             callbackLogger.error("Error in %s", func.__name__, exc_info=True)
Line 84:             ex = Exception
it doesn't keep the exception object.. it should be:

except Exception as e:
 ex = e
Line 85:         return (ret, ex)
Line 86: 
Line 87:     return wrapper
Line 88: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib25809d4416f26bc95dc72e7b32b8b2a17a71879
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv <lvroyce at linux.vnet.ibm.com>
Gerrit-Reviewer: Royce Lv <lvroyce at linux.vnet.ibm.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list