New subject: Change in vdsm[master]: refactor supervdsm proxy call logic

19 Dec 2012

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 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

    

Change in vdsm[master]: change of the isRunning function