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