Change in vdsm[master]: Run pinger in separate process

[Nir Soffer]

Nir Soffer has uploaded a new change for review.

Change subject: Run pinger in separate process
......................................................................

Run pinger in separate process

Running pinger in a separate process should be less fragile, as the
pinger thread cannot be delayed because of locking issues. Hopefully
this will eliminate the conectivity failures we see on CI.

Change-Id: I92719ef4a2673ed874fa47a9acbc1e7028bb7197
Signed-off-by: Nir Soffer 
---
A tests/functional/ping.py
M tests/functional/utils.py
2 files changed, 14 insertions(+), 12 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/93/22293/1

diff --git a/tests/functional/ping.py b/tests/functional/ping.py
new file mode 100644
index 0000000..70f9195
--- /dev/null
+++ b/tests/functional/ping.py
@@ -0,0 +1,8 @@
+import time
+from vdsm import vdscli
+
+client = vdscli.connect()
+
+while True:
+    client.ping()
+    time.sleep(1)
diff --git a/tests/functional/utils.py b/tests/functional/utils.py
index 79327f6..e884a79 100644
--- a/tests/functional/utils.py
+++ b/tests/functional/utils.py
@@ -19,13 +19,14 @@
 from collections import namedtuple
 from contextlib import contextmanager
 from functools import wraps
-import time
-import threading
+import os

from vdsm.config import config
+from vdsm import constants
 from vdsm import ipwrapper
 from vdsm import netinfo
 from vdsm import vdscli
+from vdsm import utils
 from vdsm.netconfpersistence import RunningConfig
 import supervdsm

@@ -174,19 +175,12 @@
     @contextmanager
     def pinger(self):
         """Keeps pinging vdsm for operations that need it"""
-        def ping():
-            while not done:
-                self.vdscli.ping()
-                time.sleep(1)
+        path = os.path.join(os.path.dirname(__file__), 'ping.py')
+        pinger = utils.execCmd([constants.EXT_PYTHON, path], sync=False)
         try:
-            done = False
-            pinger_thread = threading.Thread(target=ping)
-            pinger_thread.start()
             yield
-        except Exception:
-            raise
         finally:
-            done = True
+            pinger.kill()

def networkQos(self, networkName):
         network = self.netinfo.networks[networkName]

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Run pinger in separate process
......................................................................

Patch Set 1: Verified-1

Build Failed

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

http://jenkins.ovirt.org/job/vdsm_network_functional_tests/1004/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5244/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6134/ : SUCCESS

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Run pinger in separate process
......................................................................

Patch Set 2: Verified-1

Build Failed

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

http://jenkins.ovirt.org/job/vdsm_network_functional_tests/1005/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5245/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6135/ : SUCCESS

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Run pinger in separate process
......................................................................

Patch Set 2:

Build Failed

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

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5245/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6135/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_network_functional_tests/1006/ : FAILURE

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Run pinger in separate process
......................................................................

Patch Set 2:

(2 comments)

....................................................
File tests/functional/ping.py
Line 1: import time
Line 2: from vdsm import vdscli
Line 3: 
Line 4: client = vdscli.connect()
actually, this script is the client, and vdscli.connect() is the (proxy for the)  server.
Line 5: 
Line 6: try:
Line 7:     while True:
Line 8:         client.ping()

....................................................
File tests/functional/utils.py
Line 175:     @contextmanager
Line 176:     def pinger(self):
Line 177:         """Keeps pinging vdsm for operations that need it"""
Line 178:         path = os.path.join(os.path.dirname(__file__), 'ping.py')
Line 179:         pinger = utils.execCmd([constants.EXT_PYTHON, path], sync=False)
multiprocessing has its issues, but I suppose that using it could have simplified this patch (/me is not sure about it).
Line 180:         try:
Line 181:             yield
Line 182:         finally:
Line 183:             pinger.kill()

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Run pinger in separate process
......................................................................

Patch Set 2:

Build Failed

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

http://jenkins.ovirt.org/job/vdsm_network_functional_tests/1007/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5246/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6136/ : SUCCESS

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: Run pinger in separate process
......................................................................

Patch Set 2:

(2 comments)

....................................................
File tests/functional/ping.py
Line 1: import time
Line 2: from vdsm import vdscli
Line 3: 
Line 4: client = vdscli.connect()
Well

server = vdscli.connect()

Is confusing - am I serving something?

How about:

con = vdscli.connect()

connect returns a connection - common pattern.
Line 5: 
Line 6: try:
Line 7:     while True:
Line 8:         client.ping()

....................................................
File tests/functional/utils.py
Line 175:     @contextmanager
Line 176:     def pinger(self):
Line 177:         """Keeps pinging vdsm for operations that need it"""
Line 178:         path = os.path.join(os.path.dirname(__file__), 'ping.py')
Line 179:         pinger = utils.execCmd([constants.EXT_PYTHON, path], sync=False)
May be simpler code in the tests, much more complex code under the hood, for no reason. I think we should use the simplest solution for the tests. If we need to simplify, we can add a function for running such test scripts.
Line 180:         try:
Line 181:             yield
Line 182:         finally:
Line 183:             pinger.kill()

Antoni Segura Puimedon has posted comments on this change.

Change subject: Run pinger in separate process
......................................................................

Patch Set 2:

(1 comment)

....................................................
File tests/functional/ping.py
Line 1: import time
Line 2: from vdsm import vdscli
Line 3: 
Line 4: client = vdscli.connect()
I agree with con. I always use conn myself ;-)
Line 5: 
Line 6: try:
Line 7:     while True:
Line 8:         client.ping()

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Run pinger in separate process
......................................................................

Patch Set 2:

Build Failed

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

http://jenkins.ovirt.org/job/vdsm_network_functional_tests/1008/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5247/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6137/ : SUCCESS

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: Run pinger in separate process
......................................................................

Patch Set 2:

This should be less fragile but with this patch we get lot of strange failures in the network functional tests. Need to understand why we get those failures.

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: Run pinger in separate process
......................................................................

Patch Set 2: Verified-1

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Run pinger in separate process
......................................................................

Patch Set 2:

Build Failed

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

http://jenkins.ovirt.org/job/vdsm_network_functional_tests/1032/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5299/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6189/ : FAILURE

[Nir Soffer]

Nir Soffer has abandoned this change.

Change subject: Run pinger in separate process
......................................................................

Abandoned

Somehow this is even less reliable than the threading version. We should understand the real issue behind this.