From: Ondrej Lichtner olichtne@redhat.com
Before applying the following patch, please review and provide feedback if it makes sense...
Ondrej Lichtner (1): NetTestCommand: fix signal handling for bg commands
lnst/Common/NetTestCommand.py | 13 +--- lnst/Common/TestsCommon.py | 14 ++-- regression-tests/tests/25/lnst-ctl.conf | 2 - regression-tests/tests/25/recipe2.xml | 20 +++--- regression-tests/tests/25/recipe3.xml | 53 --------------- regression-tests/tests/25/run.sh | 9 +-- regression-tests/tests/25/test_modules/IcmpPing.py | 75 ---------------------- 7 files changed, 21 insertions(+), 165 deletions(-) delete mode 100644 regression-tests/tests/25/lnst-ctl.conf delete mode 100644 regression-tests/tests/25/recipe3.xml delete mode 100644 regression-tests/tests/25/test_modules/IcmpPing.py
From: Ondrej Lichtner olichtne@redhat.com
This commit fixes signal handling (mainly SIGINT) for background commands. The default action is to ignore the signals SIGHUP, SIGINT and SIGTERM. This is needed for "run command" commands to work, and if the developer of a test module wants to change the behaviour he cna do so in the run method of his module.
This commit also changes the method wait_on_interrupt provided for test modules. Previously it was bound to usage coupled with the set_handle_intr signal handler. Now the method is standalone changes your SIGINT signal handler only for the duration of the method. This makes the method set_handle_intr obsolete so it was removed.
Finally the regression test #25 was modified to reflect these changes.
Signed-off-by: Ondrej Lichtner olichtne@redhat.com --- lnst/Common/NetTestCommand.py | 13 +--- lnst/Common/TestsCommon.py | 14 ++-- regression-tests/tests/25/lnst-ctl.conf | 2 - regression-tests/tests/25/recipe2.xml | 20 +++--- regression-tests/tests/25/recipe3.xml | 53 --------------- regression-tests/tests/25/run.sh | 9 +-- regression-tests/tests/25/test_modules/IcmpPing.py | 75 ---------------------- 7 files changed, 21 insertions(+), 165 deletions(-) delete mode 100644 regression-tests/tests/25/lnst-ctl.conf delete mode 100644 regression-tests/tests/25/recipe3.xml delete mode 100644 regression-tests/tests/25/test_modules/IcmpPing.py
diff --git a/lnst/Common/NetTestCommand.py b/lnst/Common/NetTestCommand.py index 0f7c2a3..c1a796f 100644 --- a/lnst/Common/NetTestCommand.py +++ b/lnst/Common/NetTestCommand.py @@ -137,15 +137,11 @@ class NetTestCommand: "res_header": self._cmd_cls._format_cmd_res_header(), "msg": "Running in background."}
- def _sig_handler(self, signum, frame): - raise KeyboardInterrupt() - def _run(self): os.setpgrp() - signal.signal(signal.SIGHUP, self._sig_handler) - signal.signal(signal.SIGINT, self._sig_handler) - signal.signal(signal.SIGTERM, self._sig_handler) - self._cmd_cls.set_handle_intr() + signal.signal(signal.SIGHUP, signal.SIG_IGN) + signal.signal(signal.SIGINT, signal.SIG_IGN) + signal.signal(signal.SIGTERM, signal.SIG_IGN)
self._connection_pipe = self._write_pipe
@@ -362,9 +358,6 @@ class NetTestCommandGeneric(object):
return "%-9s" % (self._command["type"] + netns)
- def set_handle_intr(self): - pass - def set_resource_table(self, res_table): self._resource_table = res_table
diff --git a/lnst/Common/TestsCommon.py b/lnst/Common/TestsCommon.py index 2504ecf..b471d8d 100644 --- a/lnst/Common/TestsCommon.py +++ b/lnst/Common/TestsCommon.py @@ -59,25 +59,19 @@ class TestGeneric(NetTestCommandGeneric): self._testLogger = logging.getLogger("root.testLogger") NetTestCommandGeneric.__init__(self, command)
- def set_handle_intr(self): - """ - Call this if you need this class to handle SIGINT. For backgroud - process purposes. - """ - signal.signal(signal.SIGINT, self._signal_intr_handler) - - def _signal_intr_handler(self, signum, frame): - raise KeyboardInterrupt() - def wait_on_interrupt(self): ''' Should be used by test implementation for waiting on SIGINT ''' try: + handler = signal.getsignal(signal.SIGINT) + signal.signal(signal.SIGINT, signal.default_int_handler) while True: time.sleep(1) except KeyboardInterrupt: pass + finally: + signal.signal(signal.SIGINT, handler)
def _get_val(self, value, opt_type, default): if opt_type == "addr": diff --git a/regression-tests/tests/25/lnst-ctl.conf b/regression-tests/tests/25/lnst-ctl.conf deleted file mode 100644 index a4d654d..0000000 --- a/regression-tests/tests/25/lnst-ctl.conf +++ /dev/null @@ -1,2 +0,0 @@ -[environment] -test_module_dirs = ./test_modules diff --git a/regression-tests/tests/25/recipe2.xml b/regression-tests/tests/25/recipe2.xml index fd8b3f6..30452aa 100644 --- a/regression-tests/tests/25/recipe2.xml +++ b/regression-tests/tests/25/recipe2.xml @@ -33,17 +33,21 @@ </host> </network> <task> - <run command="ping -c 4 {ip(tm2,phy1)}" host="tm1" netns="in"/> - <run host="tm1" module="IcmpPing" bg_id="on_bg" netns="in"> + <run module="PktCounter" host="tm1" netns="in" bg_id="ctr"> <options> - <option name="addr" value="{ip(tm2,phy1)}"/> - <option name="interval" value="1"/> + <option name="input_netdev_name" value="{devname(tm1,in)}"/> </options> </run> - <ctl_wait seconds="2"/> - <run command="pstree -p" host="tm1" netns="in"/> - <ctl_wait seconds="2"/> - <intr host="tm1" bg_id="on_bg"/> + <run module="PktgenTx" host="tm2"> + <options> + <option name="netdev_name" value="{devname(tm2,phy1)}"/> + <option name="pktgen_option" value="dst {ip(tm1, in)}"/> + <option name="pktgen_option" value="dst_mac {hwaddr(tm1,in)}"/> + <option name="pktgen_option" value="count 1000"/> + </options> + </run> + <ctl_wait seconds="5"/> + <intr host="tm1" bg_id="ctr"/> </task> </lnstrecipe>
diff --git a/regression-tests/tests/25/recipe3.xml b/regression-tests/tests/25/recipe3.xml deleted file mode 100644 index 30452aa..0000000 --- a/regression-tests/tests/25/recipe3.xml +++ /dev/null @@ -1,53 +0,0 @@ -<lnstrecipe> - <network> - <host id="tm1"> - <interfaces> - <eth id="phy1" label="net1"/> - <veth_pair> - <veth id="in" netns="in"> - <addresses> - <address value="192.168.0.3/24"/> - </addresses> - </veth> - <veth id="out"/> - </veth_pair> - <bridge id="br"> - <slaves> - <slave id="out"/> - <slave id="phy1"/> - </slaves> - <addresses> - <address value="192.168.0.1/24"/> - </addresses> - </bridge> - </interfaces> - </host> - <host id="tm2"> - <interfaces> - <eth id="phy1" label="net1"> - <addresses> - <address value="192.168.0.10/24"/> - </addresses> - </eth> - </interfaces> - </host> - </network> - <task> - <run module="PktCounter" host="tm1" netns="in" bg_id="ctr"> - <options> - <option name="input_netdev_name" value="{devname(tm1,in)}"/> - </options> - </run> - <run module="PktgenTx" host="tm2"> - <options> - <option name="netdev_name" value="{devname(tm2,phy1)}"/> - <option name="pktgen_option" value="dst {ip(tm1, in)}"/> - <option name="pktgen_option" value="dst_mac {hwaddr(tm1,in)}"/> - <option name="pktgen_option" value="count 1000"/> - </options> - </run> - <ctl_wait seconds="5"/> - <intr host="tm1" bg_id="ctr"/> - </task> -</lnstrecipe> - diff --git a/regression-tests/tests/25/run.sh b/regression-tests/tests/25/run.sh index 34c1dc6..8e1daa2 100755 --- a/regression-tests/tests/25/run.sh +++ b/regression-tests/tests/25/run.sh @@ -4,22 +4,17 @@
init_test
-lnst-ctl -c lnst-ctl.conf -d run recipe1.xml | tee test.log +lnst-ctl -d run recipe1.xml | tee test.log rv1=${PIPESTATUS[0]} log1=`cat test.log`
-lnst-ctl -c lnst-ctl.conf -d run recipe2.xml | tee test.log +lnst-ctl -d run recipe2.xml | tee test.log rv2=${PIPESTATUS[0]} log2=`cat test.log`
-lnst-ctl -d run recipe3.xml | tee test.log -rv3=${PIPESTATUS[0]} -log3=`cat test.log` - print_separator assert_status "pass" "$rv1" assert_status "pass" "$rv2" -assert_status "pass" "$rv3"
rm -f test.log
diff --git a/regression-tests/tests/25/test_modules/IcmpPing.py b/regression-tests/tests/25/test_modules/IcmpPing.py deleted file mode 100644 index 0469687..0000000 --- a/regression-tests/tests/25/test_modules/IcmpPing.py +++ /dev/null @@ -1,75 +0,0 @@ -""" -This module defines icmp ping test - -Copyright 2011 Red Hat, Inc. -Licensed under the GNU General Public License, version 2 as -published by the Free Software Foundation; see COPYING for details. -""" - -__author__ = """ -jpirko@redhat.com (Jiri Pirko) -""" - -import logging -import re -from lnst.Common.TestsCommon import TestGeneric -from lnst.Common.ExecCmd import exec_cmd - -class IcmpPing(TestGeneric): - def _compose_cmd(self): - addr = self.get_mopt("addr", opt_type="addr") - cmd = "ping %s" % addr - count = self.get_opt("count") - if count: - cmd += " -c %s" % count - interval = self.get_opt("interval") - if interval: - cmd += " -i %s" % interval - iface = self.get_opt("iface") - if iface: - cmd += " -I %s" % iface - size = self.get_opt("size") - if size: - cmd += " -s %s" % size - return cmd - - def run(self): - cmd = self._compose_cmd() - limit_rate = self.get_opt("limit_rate", default=80) - - #run twice to test that we interrupt the entire bg command not just the - #currently running exec_cmd - data_stdout = exec_cmd(cmd, die_on_err=False)[0] - data_stdout = exec_cmd(cmd, die_on_err=False)[0] - - stat_pttr1 = r'(\d+) packets transmitted, (\d+) received' - stat_pttr2 = r'rtt min/avg/max/mdev = (\d+.\d+)/(\d+.\d+)/(\d+.\d+)/(\d+.\d+) ms' - - match = re.search(stat_pttr1, data_stdout) - if not match: - res_data = {"msg": "expected pattern not found"} - return self.set_fail(res_data) - - trans_pkts, recv_pkts = match.groups() - rate = int(round((float(recv_pkts) / float(trans_pkts)) * 100)) - logging.debug("Transmitted "%s", received "%s", " - "rate "%d%%", limit_rate "%d%%"" - % (trans_pkts, recv_pkts, rate, limit_rate)) - - res_data = {"rate": rate, - "limit_rate": limit_rate} - - match = re.search(stat_pttr2, data_stdout) - if match: - tmin, tavg, tmax, tmdev = [float(x) for x in match.groups()] - logging.debug("rtt min "%.3f", avg "%.3f", max "%.3f", " - "mdev "%.3f"" % (tmin, tavg, tmax, tmdev)) - - res_data["rtt_min"] = tmin - res_data["rtt_max"] = tmax - - if rate < limit_rate: - res_data["msg"] = "rate is lower than limit" - return self.set_fail(res_data) - - return self.set_pass(res_data)
Mon, Feb 23, 2015 at 01:39:29PM CET, olichtne@redhat.com wrote:
From: Ondrej Lichtner olichtne@redhat.com This commit also changes the method wait_on_interrupt provided for test modules. Previously it was bound to usage coupled with the
I don't think that it __WAS__ bound previously, the method should be still the counterpart to set_handle_intr(). I still don't see the reason why you want to remove it. It's a valid mean to do synchronization from within the task.
The valid use case for this is when you want to compare data before and after another <run> takes place, that's what PktCounter is doing.
<example>
MyModule(TestsCommon): def run(): # I'm interested in synchronization within task self.set_handle_intr()
# save state before sync point state1 = state.dump()
# now wait for 'intr' in lnst recipe # this blocks self.wait_on_interrupt()
# save state after sync point state2 = state.dump()
# process the monitor output if state.compare(state1, state2): self.set_fail() else: self.set_pass()
</example>
set_handle_intr signal handler. Now the method is standalone changes your SIGINT signal handler only for the duration of the method. This makes the method set_handle_intr obsolete so it was removed.
Well with the patch your example would simply look like this:
<example>
MyModule(TestsCommon): def run(): # save state before sync point state1 = state.dump()
# now wait for 'intr' in lnst recipe # this blocks self.wait_on_interrupt()
# save state after sync point state2 = state.dump()
# process the monitor output if state.compare(state1, state2): self.set_fail() else: self.set_pass()
</example>
Basically you just skip the set_handle_intr() call so the wait_on_interrupt call is both a declaration that you're interested in synchronization within task and the action of waiting as well.
As to why I want to do this, I have 2 reasons: 1. it allows you to define your own signal handling for normal situations and when you want to 'wait_on_interrupt' it will return your setup after the call is finished. 2. you type less -> when you want to wait on interrupt, you just call wait_on_interrupt instead of setting the handler then waiting and then possibly resetting the handlers again.
Let me know what you think.
On Mon, Feb 23, 2015 at 02:04:32PM +0100, Jan Tluka wrote:
Mon, Feb 23, 2015 at 01:39:29PM CET, olichtne@redhat.com wrote:
From: Ondrej Lichtner olichtne@redhat.com This commit also changes the method wait_on_interrupt provided for test modules. Previously it was bound to usage coupled with the
I don't think that it __WAS__ bound previously, the method should be still the counterpart to set_handle_intr(). I still don't see the reason why you want to remove it. It's a valid mean to do synchronization from within the task.
The valid use case for this is when you want to compare data before and after another <run> takes place, that's what PktCounter is doing.
<example>
MyModule(TestsCommon): def run(): # I'm interested in synchronization within task self.set_handle_intr()
# save state before sync point state1 = state.dump() # now wait for 'intr' in lnst recipe # this blocks self.wait_on_interrupt() # save state after sync point state2 = state.dump() # process the monitor output if state.compare(state1, state2): self.set_fail() else: self.set_pass()</example>
set_handle_intr signal handler. Now the method is standalone changes your SIGINT signal handler only for the duration of the method. This makes the method set_handle_intr obsolete so it was removed.
LNST-developers mailing list LNST-developers@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/lnst-developers
Ack. All your points are valid. Also the code is much clearer. Not sure why I refused to accept this previously, very likely that I wanted to keep that part of the code unchanged since it was working ok. However your change makes it better.
-Jan
Mon, Mar 02, 2015 at 09:08:51AM CET, olichtne@redhat.com wrote:
Well with the patch your example would simply look like this:
<example>
MyModule(TestsCommon): def run(): # save state before sync point state1 = state.dump()
# now wait for 'intr' in lnst recipe # this blocks self.wait_on_interrupt() # save state after sync point state2 = state.dump() # process the monitor output if state.compare(state1, state2): self.set_fail() else: self.set_pass()</example>
Basically you just skip the set_handle_intr() call so the wait_on_interrupt call is both a declaration that you're interested in synchronization within task and the action of waiting as well.
As to why I want to do this, I have 2 reasons:
- it allows you to define your own signal handling for normal
situations and when you want to 'wait_on_interrupt' it will return your setup after the call is finished. 2. you type less -> when you want to wait on interrupt, you just call wait_on_interrupt instead of setting the handler then waiting and then possibly resetting the handlers again.
Let me know what you think.
On Mon, Feb 23, 2015 at 02:04:32PM +0100, Jan Tluka wrote:
Mon, Feb 23, 2015 at 01:39:29PM CET, olichtne@redhat.com wrote:
From: Ondrej Lichtner olichtne@redhat.com This commit also changes the method wait_on_interrupt provided for test modules. Previously it was bound to usage coupled with the
I don't think that it __WAS__ bound previously, the method should be still the counterpart to set_handle_intr(). I still don't see the reason why you want to remove it. It's a valid mean to do synchronization from within the task.
The valid use case for this is when you want to compare data before and after another <run> takes place, that's what PktCounter is doing.
<example>
MyModule(TestsCommon): def run(): # I'm interested in synchronization within task self.set_handle_intr()
# save state before sync point state1 = state.dump() # now wait for 'intr' in lnst recipe # this blocks self.wait_on_interrupt() # save state after sync point state2 = state.dump() # process the monitor output if state.compare(state1, state2): self.set_fail() else: self.set_pass()</example>
set_handle_intr signal handler. Now the method is standalone changes your SIGINT signal handler only for the duration of the method. This makes the method set_handle_intr obsolete so it was removed.
LNST-developers mailing list LNST-developers@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/lnst-developers
LNST-developers mailing list LNST-developers@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/lnst-developers
lnst-developers@lists.fedorahosted.org
-
Jan Tluka -
Ondrej Lichtner