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 <[email protected]>
Gerrit-Reviewer: Adam Litke <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Royce Lv <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: Shu Ming <[email protected]>
Gerrit-Reviewer: Wenyi Gao <[email protected]>
Gerrit-Reviewer: Xu He Jie <[email protected]>
Gerrit-Reviewer: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: Zhou Zheng Sheng <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
