Hello Zhou Zheng Sheng,
I'd like you to do a code review. Please visit
http://gerrit.ovirt.org/10236
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 Lv<lvroyce(a)linux.vnet.ibm.com>
Signed-off-by:Zhou Zheng Sheng<zhshzhou(a)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(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com>