Hello Zhou Zheng Sheng,
I'd like you to do a code review. Please visit
to review the following change.
Change subject: change of the isRunning function ......................................................................
change of the isRunning function
isRunning() now checks supervdsm pid this not guarantee supervdsmServer really listened and manager framework works right. since ping is call to local manager so it won't take much time. Only ping() can identify whether supervdsm works fine. Change isRunning() to use ping() also eliminate Authentication error usecases: 1.first launch: ping fails because of _svdsm=None, launch supervdsm 2.previous _svdsm: ping fails with Autherror,kill and launch 3.previous svdsm exit:ping fails with socket error,launch 4.call error: raise to caller 5.ping succeed,svdsm killed when callmethod: raise error to caller
Change-Id: Ib25809d4416f26bc95dc72e7b32b8b2a17a71879 Signed-off-by:Royce Lvlvroyce@linux.vnet.ibm.com Signed-off-by:Zhou Zheng Shengzhshzhou@linux.vnet.ibm.com --- M tests/superVdsmTests.py M vdsm/supervdsm.py M vdsm/supervdsmServer.py 3 files changed, 24 insertions(+), 79 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/36/10236/1
diff --git a/tests/superVdsmTests.py b/tests/superVdsmTests.py index 87de66d..57f4de8 100644 --- a/tests/superVdsmTests.py +++ b/tests/superVdsmTests.py @@ -28,7 +28,7 @@ superVdsmCmd = [getNeededPythonPath(), constants.EXT_PYTHON, supervdsm.SUPERVDSM, self._authkey, str(os.getpid()), - self.pidfile, self.timestamp, self.address, + self.pidfile, self.address, str(os.getuid())] misc.execCmd(superVdsmCmd, sync=False, sudo=True) sleep(2) @@ -41,33 +41,24 @@
# temporary values to run temporary svdsm self.pidfd, pidfile = tempfile.mkstemp() - self.timefd, timestamp = tempfile.mkstemp() self.addfd, address = tempfile.mkstemp()
- self._proxy.setIPCPaths(pidfile, timestamp, address) + self._proxy.setIPCPaths(pidfile, address)
def tearDown(self): supervdsm.extraPythonPathList = [] - for fd in (self.pidfd, self.timefd, self.addfd): - os.close(fd) + os.close(self.pidfd) self._proxy.kill() # cleanning old temp files
@MonkeyPatch(supervdsm.SuperVdsmProxy, '_start', monkeyStart) def testIsSuperUp(self): - self._proxy.ping() # this call initiate svdsm - self.assertTrue(self._proxy.isRunning()) + self._proxy.ping() + self.assertTrue(self._proxy.isRunning()) # this call initiate svdsm
@MonkeyPatch(supervdsm.SuperVdsmProxy, '_start', monkeyStart) def testKillSuper(self): self._proxy.ping() self._proxy.kill() - self.assertFalse(self._proxy.isRunning()) + self.assertRaises(AttributeError, self._proxy.isRunning) self._proxy.ping() # Launching vdsm after kill self.assertTrue(self._proxy.isRunning()) - - @MonkeyPatch(supervdsm.SuperVdsmProxy, '_start', monkeyStart) - def testNoPidFile(self): - self._proxy.ping() # svdsm is up - self.assertTrue(self._proxy.isRunning()) - utils.rmFile(self._proxy.timestamp) - self.assertRaises(IOError, self._proxy.isRunning) diff --git a/vdsm/supervdsm.py b/vdsm/supervdsm.py index 532d5ac..a9bbdf6 100644 --- a/vdsm/supervdsm.py +++ b/vdsm/supervdsm.py @@ -20,13 +20,11 @@ #
import os -from multiprocessing import AuthenticationError from multiprocessing.managers import BaseManager import logging import threading import uuid from time import sleep -from errno import ENOENT, ESRCH
import storage.misc as misc from vdsm import constants, utils @@ -45,7 +43,6 @@ raise RuntimeError("SuperVDSM Server not found")
PIDFILE = os.path.join(constants.P_VDSM_RUN, "svdsm.pid") -TIMESTAMP = os.path.join(constants.P_VDSM_RUN, "svdsm.time") ADDRESS = os.path.join(constants.P_VDSM_RUN, "svdsm.sock") SUPERVDSM = __supervdsmServerPath("supervdsmServer.py")
@@ -66,22 +63,16 @@ callMethod = lambda: \ getattr(self._supervdsmProxy._svdsm, self._funcName)(*args, **kwargs) - if not self._supervdsmProxy.isRunning(): - # getting inside only when svdsm is down. its rare case so we - # don't care that isRunning will run twice - with self._supervdsmProxy.proxyLock: - if not self._supervdsmProxy.isRunning(): - self._supervdsmProxy.launch()
- try: - return callMethod() - # handling internal exception that we raise to identify supervdsm - # validation. only this exception can cause kill! - except AuthenticationError: - with self._supervdsmProxy.proxyLock: + with self._supervdsmProxy.proxyLock: + try: + # make sure supervdsmServer works right + self._supervdsmProxy.isRunning() + except: self._supervdsmProxy.kill() self._supervdsmProxy.launch() - return callMethod() + + return callMethod()
class SuperVdsmProxy(object): @@ -92,25 +83,19 @@
def __init__(self): self.proxyLock = threading.Lock() - self._firstLaunch = True + self.setIPCPaths(PIDFILE, ADDRESS)
- # Declaration of public variables that keep files' names that svdsm - # uses. We need to be able to change these variables so that running - # tests doesn't disturb and already running VDSM on the host. - self.setIPCPaths(PIDFILE, TIMESTAMP, ADDRESS) - - def setIPCPaths(self, pidfile, timestamp, address): + def setIPCPaths(self, pidfile, address): self.pidfile = pidfile - self.timestamp = timestamp self.address = address
def open(self, *args, **kwargs): return self._manager.open(*args, **kwargs)
def _cleanOldFiles(self): - self._log.debug("Cleanning svdsm old files: %s, %s, %s", - self.pidfile, self.timestamp, self.address) - for f in (self.pidfile, self.timestamp, self.address): + self._log.debug("Cleanning svdsm old files: %s, %s", + self.pidfile, self.address) + for f in (self.pidfile, self.address): utils.rmFile(f)
def _start(self): @@ -122,7 +107,7 @@ # permissions to read those files. superVdsmCmd = [constants.EXT_PYTHON, SUPERVDSM, self._authkey, str(os.getpid()), - self.pidfile, self.timestamp, self.address, + self.pidfile, self.address, str(os.getuid())]
misc.execCmd(superVdsmCmd, sync=False, sudo=True) @@ -134,42 +119,15 @@ pid = int(f.read().strip()) misc.execCmd([constants.EXT_KILL, "-9", str(pid)], sudo=True) except Exception: - self._log.error("Could not kill old Super Vdsm %s", - exc_info=True) + self._log.warn("Could not kill old Super Vdsm")
self._cleanOldFiles() self._authkey = None self._manager = None self._svdsm = None - self._firstLaunch = True
def isRunning(self): - try: - with open(self.pidfile, "r") as f: - spid = f.read().strip() - with open(self.timestamp, "r") as f: - createdTime = f.read().strip() - except IOError as e: - # pid file and timestamp file must be exist after first launch, - # otherwise excpetion will be raised to svdsm caller - if e.errno == ENOENT and self._firstLaunch: - return False - else: - raise - - try: - pTime = str(misc.getProcCtime(spid)) - except OSError as e: - if e.errno == ESRCH: - # Means pid is not exist, svdsm was killed - return False - else: - raise - - if pTime == createdTime: - return True - else: - return False + return self._svdsm.ping()
def _connect(self): self._manager = _SuperVdsmManager(address=self.address, @@ -185,7 +143,6 @@ self._svdsm = self._manager.instance()
def launch(self): - self._firstLaunch = False self._start() utils.retry(self._connect, Exception, timeout=60)
diff --git a/vdsm/supervdsmServer.py b/vdsm/supervdsmServer.py index 5effd41..8968a82 100755 --- a/vdsm/supervdsmServer.py +++ b/vdsm/supervdsmServer.py @@ -336,15 +336,12 @@ sys.exit(errno.EPERM)
log.debug("Parsing cmd args") - authkey, parentPid, pidfile, timestamp, address, uid = sys.argv[1:] + authkey, parentPid, pidfile, address, uid = sys.argv[1:]
- log.debug("Creating PID and TIMESTAMP files: %s, %s", - pidfile, timestamp) + log.debug("Creating PID:%s", pidfile) spid = os.getpid() with open(pidfile, "w") as f: f.write(str(spid) + "\n") - with open(timestamp, "w") as f: - f.write(str(misc.getProcCtime(spid) + "\n"))
log.debug("Cleaning old socket %s", address) if os.path.exists(address): @@ -369,7 +366,7 @@ servThread.setDaemon(True) servThread.start()
- for f in (address, timestamp, pidfile): + for f in (address, pidfile): chown(f, int(uid), METADATA_GROUP)
log.debug("Started serving super vdsm object")
-- To view, visit http://gerrit.ovirt.org/10236 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: Ib25809d4416f26bc95dc72e7b32b8b2a17a71879 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com