Change in vdsm[master]: signals: Handle signals to non-main threads

dkuznets at redhat.com dkuznets at redhat.com
Mon Jan 19 13:25:10 UTC 2015


Dima Kuznetsov has posted comments on this change.

Change subject: signals: Handle signals to non-main threads
......................................................................


Patch Set 24:

(7 comments)

http://gerrit.ovirt.org/#/c/29392/24/lib/vdsm/infra/sigutils/tests.py
File lib/vdsm/infra/sigutils/tests.py:

Line 33: 
Line 34: def assert_read(stream, expected):
Line 35:     while True:
Line 36:         try:
Line 37:             tools.assert_equals(stream.read(len(expected)), expected)
> If we don't use vdsm  testing framework, we don't need to use nose.assert_ 
Saggi wanted to use nose facilities (since we're running the tests with nose in .infra package)
Line 38:         except IOError as e:
Line 39:             if e.errno != errno.EINTR:
Line 40:                 raise
Line 41:         else:


Line 41:         else:
Line 42:             break
Line 43: 
Line 44: 
Line 45: def popen_helper(*args):
> This name does not mean anything. Should be something like child_test().
Agree
Line 46:     return subprocess.Popen(
Line 47:         [os.path.abspath(CHILD_SCRIPT)] + list(args),
Line 48:         stdout=subprocess.PIPE,
Line 49:         cwd=os.path.dirname(__file__)


Line 50:     )
Line 51: 
Line 52: 
Line 53: def test_signal_received():
Line 54:     popen = popen_helper('test_signal_received')
> You never terminate and wait on this sub process. This should be:
Agree
Line 55:     assert_read(popen.stdout, 'ready\n')
Line 56:     popen.send_signal(signal.SIGUSR1)
Line 57:     assert_read(popen.stdout, 'signal sigusr1\n')
Line 58:     assert_read(popen.stdout, 'done\n')


http://gerrit.ovirt.org/#/c/29392/24/lib/vdsm/infra/sigutils/tests_child.py
File lib/vdsm/infra/sigutils/tests_child.py:

Line 27: sys.path.insert(0, '../../..')
Line 28: from vdsm.infra import sigutils
Line 29: 
Line 30: 
Line 31: def test_signal_received():
> Please do not name methods which are not tests with test_ prefix.
Done
Line 32:     sigutils.wait_for_signal()
Line 33: 
Line 34: 
Line 35: def test_signal_timeout(timeout):


Line 57:     '''
Line 58: 
Line 59:     def thread_target():
Line 60:         subprocess.Popen(['true'])
Line 61:         time.sleep(0.1)  # sleep so SIGCHLD is delivered here.
> 0.1 may be too short for overloaded jenkins slave.
will bump to 1
Line 62:     threading.Thread(target=thread_target).start()
Line 63:     sigutils.wait_for_signal()
Line 64: 
Line 65: 


Line 67:     try:
Line 68:         sigutils.register()
Line 69:     except RuntimeError:
Line 70:         sys.stdout.write('exception\n')
Line 71: 
> The not registered test from test.py should be here.
Done
Line 72: 
Line 73: if __name__ == '__main__':
Line 74:     # Set up signal handlers
Line 75:     signal.signal(signal.SIGUSR1,


Line 80:     sigutils.register()
Line 81: 
Line 82:     # Set timer to kill the process in case we're stuck.
Line 83:     signal.signal(signal.SIGALRM, lambda *_: sys.exit(1))
Line 84:     signal.setitimer(signal.ITIMER_REAL, 10)
> Why do we need ITIMER_REAL? isn't SIGALRM enough?
This call creates an alarm
Line 85: 
Line 86:     sys.stdout.write('ready\n')
Line 87: 
Line 88:     globals()[sys.argv[1]](*sys.argv[2:])


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5dbcd00cec22ef12f2b6253b016dcbd0aa889583
Gerrit-PatchSet: 24
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dima Kuznetsov <dkuznets at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Dima Kuznetsov <dkuznets at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list