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(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server