Change in vdsm[master]: change startup process for vdsm and supervdsm

ybronhei at redhat.com ybronhei at redhat.com
Wed Nov 14 08:19:41 UTC 2012


Yaniv Bronhaim has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................


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

(7 inline comments)

This patch is too big and changes not only svdsm implementation but also vdsm startup process and the way it starts and stops svdsm. 

You didn't include unit tests for this patch so it's hard to know if it works in different cases.

I prefer that you will split this patch and rebase it over upstream code before accepting it, but if you want to keep it that way (what I think is wrong..), try to rebase it and add uts for that and I'll try to verify if it works properly.

thanks.

....................................................
File vdsm/supervdsm.py
Line 53:         try:
Line 54:             return callMethod()
Line 55:         except(IOError, socket.error, AuthenticationError):
Line 56:             os.kill(os.getpid(), signal.SIGTERM)
Line 57:             raise
we don't need to kill svdsm if we get IOError or socket.error, it could be raised from svdsm internal implementation, like when delNetwork raised IOError.. it doesn't mean that something is wrong with svdsm.. we should just raise the error.
Although, AuthenticationError must lead to svdsm restart.
Line 58: 
Line 59: 
Line 60: class SuperVdsmProxy(object):
Line 61:     """


Line 67:         try:
Line 68:             misc.retry(self._connect, Exception, timeout=60)
Line 69:         except Exception:
Line 70:             self._log.debug("error raised when connecting to super vdsm")
Line 71:             os.unlink(ADDRESS)
unlink can cause exception and the next kill line will never run.. use utils.rmFile
Line 72:             os.kill(os.getpid(), signal.SIGTERM)
Line 73:         self._log.debug("Connected to Super Vdsm")
Line 74: 
Line 75:     def open(self, *args, **kwargs):


....................................................
File vdsm/supervdsmServer.py
Line 100:         return _getLsBlk(*args, **kwargs)
Line 101: 
Line 102:     @logDecorator
Line 103:     def stopSuperVdsm(self):
Line 104:         os.unlink(ADDRESS)
you should use utils.rmFile here. but why do we need the option to stop svdsm from outside?..
Line 105:         os.kill(os.getpid(), signal.SIGTERM)
Line 106: 
Line 107:     @logDecorator
Line 108:     def readMultipathConf(self):


Line 335: 
Line 336:     log = logging.getLogger("SuperVdsm.Server")
Line 337:     try:
Line 338:         # kill old supervdsm
Line 339:         _killSupervdsm(log)
supservdsm.py should control the initialization, termination and calling for requests to svdsm, supervdsmServer supposed to handle all svdsm api and startup. (and if svdsm should die when vdsm goes done, the suicide process).

This implementation mix this concepts and turn supervdsmServer.py to be the manager. so why do you keep supervdsm.py?
Line 340: 
Line 341:         log.debug("Creating PID file")
Line 342: 
Line 343:         with open(PIDFILE, "w") as f:


....................................................
File vdsm/vdsm
Line 81:     log = logging.getLogger('vds')
Line 82:     try:
Line 83:         logging.root.handlers.append(logging.StreamHandler())
Line 84:         log.handlers.append(logging.StreamHandler())
Line 85:         chown(LOGFILE, VDSM_USER, VDSM_GROUP)
We don't want to perform this chown every startup.. permission to log are not supposed to change.
This's why we start vdsm with user vdsm and not as root, we don't want to run vdsm as root because we don't know what other programmers added to vdsm startup. 
It is safer to change the user from root before invoking your process.
Line 86:     except:
Line 87:         log.error("Exception raised", exc_info=True)
Line 88:     return log
Line 89: 


Line 142: 
Line 143:     # When key is None, connections are restricted to child processes
Line 144:     manager = _SuperVdsmManager(ADDRESS, None)
Line 145:     vdsmPid = os.getpid()
Line 146:     svdsmProc = Process(target=restartSvdsm, args=(manager, vdsmPid, ))
why did you change to Process instead of multiprocessing manager?
Line 147:     try:
Line 148:         svdsmProc.start()
Line 149:         waitThread = threading.Thread(target=__waitSvdsm,
Line 150:                                       args=[svdsmProc])


Line 145:     vdsmPid = os.getpid()
Line 146:     svdsmProc = Process(target=restartSvdsm, args=(manager, vdsmPid, ))
Line 147:     try:
Line 148:         svdsmProc.start()
Line 149:         waitThread = threading.Thread(target=__waitSvdsm,
I think it's important and we can add it to supervdsm.py (not here.. just to separate responsibilities, this py should only initialize  vdsm), but I think we shouldn't stop vdsm if svdsm stops.. but i need another opinion about it ..
Line 150:                                       args=[svdsmProc])
Line 151:         waitThread.setDaemon(True)
Line 152:         waitThread.start()
Line 153:         startVdsm(logger)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv <lvroyce at linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl at us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Royce Lv <lvroyce at linux.vnet.ibm.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Shu Ming <shuming at linux.vnet.ibm.com>
Gerrit-Reviewer: Wenyi Gao <wenyi at linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj 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