Change in vdsm[master]: Add ut to supervdsmServer zombie reaper

Royce Lv has uploaded a new change for review.

Change subject: Add ut to supervdsmServer zombie reaper
......................................................................

Add ut to supervdsmServer zombie reaper

Change-Id: I7fb8f3dcd575266febe11967e3f8df7d3588acb8
Signed-off-by: Royce Lv
---
M tests/superVdsmTests.py
M vdsm/supervdsmServer.py
2 files changed, 12 insertions(+), 7 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/54/9354/1

diff --git a/tests/superVdsmTests.py b/tests/superVdsmTests.py
index 20b1591..cb52ff1 100644
--- a/tests/superVdsmTests.py
+++ b/tests/superVdsmTests.py
@@ -4,6 +4,7 @@
 import tempfile
 from vdsm import utils
 import os
+from time import sleep

class TestSuperVdsm(TestCaseBase):
@@ -16,8 +17,11 @@
         self.pidfd, pidfile = tempfile.mkstemp()
         self.timefd, timestamp = tempfile.mkstemp()
         self.addfd, address = tempfile.mkstemp()
+        self.pidfile = pidfile

self._proxy.setIPCPaths(pidfile, timestamp, address)
+        self._proxy.ping()
+        self.assertTrue(self._proxy.isRunning())

def tearDown(self):
         for fd in (self.pidfd, self.timefd, self.addfd):
@@ -25,19 +29,19 @@
         self._proxy.kill()  # cleanning old temp files

def testIsSuperUp(self):
-        self._proxy.ping()  # this call initiate svdsm
-
-        self.assertTrue(self._proxy.isRunning())
+        pass

def testKillSuper(self):
-        self._proxy.ping()
         self._proxy.kill()
         self.assertFalse(self._proxy.isRunning())
         self._proxy.ping()  # Launching vdsm after kill
         self.assertTrue(self._proxy.isRunning())

def testNoPidFile(self):
-        self._proxy.ping()  # svdsm is up
-        self.assertTrue(self._proxy.isRunning())
         utils.rmFile(self._proxy.timestamp)
         self.assertFalse(self._proxy.isRunning())
+
+    def testZombieReaper(self):
+        reapedPid = self._proxy.ping()
+        sleep(10)  # supervdsmServer checks SIGCHLD every 5 secs
+        self.assertRaises(OSError, os.waitpid, reapedPid, os.WNOHANG)
diff --git a/vdsm/supervdsmServer.py b/vdsm/supervdsmServer.py
index 1287fef..56dd5fd 100755
--- a/vdsm/supervdsmServer.py
+++ b/vdsm/supervdsmServer.py
@@ -92,7 +92,8 @@
     @logDecorator
     def ping(self, *args, **kwargs):
         # This method exists for testing purposes
-        return True
+        res = self._runAs(DISKIMAGE_USER, (DISKIMAGE_GROUP,), os.getpid)
+        return res

@logDecorator
     def getDevicePartedInfo(self, *args, **kwargs):

--
To view, visit http://gerrit.ovirt.org/9354
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I7fb8f3dcd575266febe11967e3f8df7d3588acb8
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Add ut to supervdsmServer zombie reaper
......................................................................

Patch Set 1:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/59/ (1/2)

--
To view, visit http://gerrit.ovirt.org/9354
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fb8f3dcd575266febe11967e3f8df7d3588acb8
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Add ut to supervdsmServer zombie reaper
......................................................................

Patch Set 1:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/82/ (2/2)

--
To view, visit http://gerrit.ovirt.org/9354
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fb8f3dcd575266febe11967e3f8df7d3588acb8
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: oVirt Jenkins CI Server

Yaniv Bronhaim has posted comments on this change.

Change subject: Add ut to supervdsmServer zombie reaper
......................................................................

Patch Set 1: I would prefer that you didn't submit this

(3 inline comments)

Thanks! Some small comments.. Still working on http://gerrit.ovirt.org/#/c/9278/ to actually run the tests.

....................................................
File tests/superVdsmTests.py
Line 16:         # temporary values to run temporary svdsm
Line 17:         self.pidfd, pidfile = tempfile.mkstemp()
Line 18:         self.timefd, timestamp = tempfile.mkstemp()
Line 19:         self.addfd, address = tempfile.mkstemp()
Line 20:         self.pidfile = pidfile
why do you need to keep the file name ?
Line 21: 
Line 22:         self._proxy.setIPCPaths(pidfile, timestamp, address)
Line 23:         self._proxy.ping()
Line 24:         self.assertTrue(self._proxy.isRunning())

Line 19:         self.addfd, address = tempfile.mkstemp()
Line 20:         self.pidfile = pidfile
Line 21: 
Line 22:         self._proxy.setIPCPaths(pidfile, timestamp, address)
Line 23:         self._proxy.ping()
You've changed my tests flow here, the point was to test if calling to svdsm ping function will start and initialize super vdsm process. With your change we won't know if something is wrong with super vdsm initialization, because during the setUp we already start it. This is the reason for testIsSuperUp that you omit... But if you prefer it that way, I accept the change, just remove all testIsSuperUp function and write a comment next to ping() call that it will cause svdsm execution (because you can't really understand it from reading the code)
Line 24:         self.assertTrue(self._proxy.isRunning())
Line 25: 
Line 26:     def tearDown(self):
Line 27:         for fd in (self.pidfd, self.timefd, self.addfd):

....................................................
File vdsm/supervdsmServer.py
Line 89: 
Line 90: class _SuperVdsm(object):
Line 91: 
Line 92:     @logDecorator
Line 93:     def ping(self, *args, **kwargs):
I think you should add another function to check this, because when I wrote ping I met that all it does is to verify that a message got to svdsm and the return value returned. here you call to _runAs that can throw exception or whatever that can happened and it changes the meaning of ping...
Line 94:         # This method exists for testing purposes
Line 95:         res = self._runAs(DISKIMAGE_USER, (DISKIMAGE_GROUP,), os.getpid)
Line 96:         return res
Line 97:

--
To view, visit http://gerrit.ovirt.org/9354
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fb8f3dcd575266febe11967e3f8df7d3588acb8
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

Royce Lv has posted comments on this change.

Change subject: Add ut to supervdsmServer zombie reaper
......................................................................

Patch Set 1: (3 inline comments)

....................................................
File tests/superVdsmTests.py
Line 16:         # temporary values to run temporary svdsm
Line 17:         self.pidfd, pidfile = tempfile.mkstemp()
Line 18:         self.timefd, timestamp = tempfile.mkstemp()
Line 19:         self.addfd, address = tempfile.mkstemp()
Line 20:         self.pidfile = pidfile
It's a stupid mistake forgot to clean:(
Line 21: 
Line 22:         self._proxy.setIPCPaths(pidfile, timestamp, address)
Line 23:         self._proxy.ping()
Line 24:         self.assertTrue(self._proxy.isRunning())

Line 19:         self.addfd, address = tempfile.mkstemp()
Line 20:         self.pidfile = pidfile
Line 21: 
Line 22:         self._proxy.setIPCPaths(pidfile, timestamp, address)
Line 23:         self._proxy.ping()
I moved them here because every test needs to check whether svdsm works right.But at a second thought I think you're right, maybe we may have test to not start supervdsm and see what vdsm would act in future.
Line 24:         self.assertTrue(self._proxy.isRunning())
Line 25: 
Line 26:     def tearDown(self):
Line 27:         for fd in (self.pidfd, self.timefd, self.addfd):

....................................................
File vdsm/supervdsmServer.py
Line 89: 
Line 90: class _SuperVdsm(object):
Line 91: 
Line 92:     @logDecorator
Line 93:     def ping(self, *args, **kwargs):
I guess maintainers won't like two test functions in supervdsm so I stole your function. I'll figure out another way.
Line 94:         # This method exists for testing purposes
Line 95:         res = self._runAs(DISKIMAGE_USER, (DISKIMAGE_GROUP,), os.getpid)
Line 96:         return res
Line 97:

--
To view, visit http://gerrit.ovirt.org/9354
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fb8f3dcd575266febe11967e3f8df7d3588acb8
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

Yaniv Bronhaim has posted comments on this change.

Change subject: Add ut to supervdsmServer zombie reaper
......................................................................

Patch Set 1:

Please rebase it over http://gerrit.ovirt.org/#/c/9278/ and verify that the tests work over fedora and rhel.

Thanks.

--
To view, visit http://gerrit.ovirt.org/9354
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fb8f3dcd575266febe11967e3f8df7d3588acb8
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

Royce Lv has posted comments on this change.

Change subject: Add ut to supervdsmServer zombie reaper
......................................................................

Patch Set 2: Verified

--
To view, visit http://gerrit.ovirt.org/9354
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fb8f3dcd575266febe11967e3f8df7d3588acb8
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

Yaniv Bronhaim has posted comments on this change.

Change subject: Add ut to supervdsmServer zombie reaper
......................................................................

Patch Set 2: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/9354
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fb8f3dcd575266febe11967e3f8df7d3588acb8
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Add ut to supervdsmServer zombie reaper
......................................................................

Patch Set 2: I would prefer that you didn't submit this

(2 inline comments)

....................................................
File tests/superVdsmTests.py
Line 78:         self.assertRaises(IOError, self._proxy.isRunning)
Line 79: 
Line 80:     @MonkeyPatch(supervdsm.SuperVdsmProxy, '_start', monkeyStart)
Line 81:     def testZombieReaper(self):
Line 82:         REPO_PATH = '/rhev/data-center'
this should be taken from configure.ac - we realllly want to drop the /rhev string, and it is hard as it is.
Line 83:         self._proxy.ping()
Line 84:         self.assertTrue(self._proxy.isRunning())
Line 85:         self._proxy.validateAccess(
Line 86:                 QEMU_PROCESS_USER,

Line 89: 
Line 90:         with os.fdopen(self.pidfd, "r") as f:
Line 91:             svdsmPid = f.read().strip()
Line 92: 
Line 93:         reapPid = utils.execCmd(
We (or should I say "Saggi") try to make constants.py cleaner, as well as cleaning how we execute external applications.

Instead of adding /usr/bin/ps dependency, would you use storage.misc.pidStat() ? Moving it out of "storage" would make sense, too.
Line 94:                          [EXT_PS, '--ppid', svdsmPid, '-o', 'pid='])[1]
Line 95:         if reapPid:
Line 96:             sleep(6)  # supervdsmServer checks SIGCHLD every 5 secs

--
To view, visit http://gerrit.ovirt.org/9354
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fb8f3dcd575266febe11967e3f8df7d3588acb8
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

Yaniv Bronhaim has posted comments on this change.

Change subject: Add ut to supervdsmServer zombie reaper
......................................................................

Patch Set 2: No score

(1 inline comment)

....................................................
File tests/superVdsmTests.py
Line 89: 
Line 90:         with os.fdopen(self.pidfd, "r") as f:
Line 91:             svdsmPid = f.read().strip()
Line 92: 
Line 93:         reapPid = utils.execCmd(
already done - http://gerrit.ovirt.org/10932
Line 94:                          [EXT_PS, '--ppid', svdsmPid, '-o', 'pid='])[1]
Line 95:         if reapPid:
Line 96:             sleep(6)  # supervdsmServer checks SIGCHLD every 5 secs

--
To view, visit http://gerrit.ovirt.org/9354
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fb8f3dcd575266febe11967e3f8df7d3588acb8
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Add ut to supervdsmServer zombie reaper
......................................................................

Patch Set 3:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/752/ (1/2)

--
To view, visit http://gerrit.ovirt.org/9354
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fb8f3dcd575266febe11967e3f8df7d3588acb8
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Add ut to supervdsmServer zombie reaper
......................................................................

Patch Set 3:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/717/ (2/2)

--
To view, visit http://gerrit.ovirt.org/9354
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fb8f3dcd575266febe11967e3f8df7d3588acb8
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Add ut to supervdsmServer zombie reaper
......................................................................

Patch Set 3: Fails

Build Failed

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/717/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/752/ : FAILURE

--
To view, visit http://gerrit.ovirt.org/9354
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fb8f3dcd575266febe11967e3f8df7d3588acb8
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

Itamar Heim has posted comments on this change.

Change subject: Add ut to supervdsmServer zombie reaper
......................................................................

Patch Set 3:

ping?

Yaniv Bronhaim has posted comments on this change.

Change subject: Add ut to supervdsmServer zombie reaper
......................................................................

Patch Set 3:

not relevant, can be abandoned. thanks

Itamar Heim has abandoned this change.

Change subject: Add ut to supervdsmServer zombie reaper
......................................................................

Abandoned