| Message ID | 516043441bd13bc1e6ba7f507a04362e04c06da5.1633520807.git.cdleonard@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | selftests: Improve nettest and net/fcnal-test.sh | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Series has a cover letter |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 57 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | No static functions without inline keyword in header files |
On 10/6/21 5:47 AM, Leonard Crestez wrote: > Reduce default client timeout from 5 seconds to 500 miliseconds. > Can be overridden from environment by exporting NETTEST_CLIENT_TIMEOUT=5 > > Some tests need ICMP timeouts so pass an explicit -t5 for those. > > Signed-off-by: Leonard Crestez <cdleonard@gmail.com> > --- > tools/testing/selftests/net/fcnal-test.sh | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > The problem with blindly reducing the timeouts is running the script on a loaded server. Some tests are expected to timeout while for tests a timeout is a failure.
On 06.10.2021 18:01, David Ahern wrote: > On 10/6/21 5:47 AM, Leonard Crestez wrote: >> Reduce default client timeout from 5 seconds to 500 miliseconds. >> Can be overridden from environment by exporting NETTEST_CLIENT_TIMEOUT=5 >> >> Some tests need ICMP timeouts so pass an explicit -t5 for those. >> >> Signed-off-by: Leonard Crestez <cdleonard@gmail.com> >> --- >> tools/testing/selftests/net/fcnal-test.sh | 17 +++++++++++------ >> 1 file changed, 11 insertions(+), 6 deletions(-) >> > > The problem with blindly reducing the timeouts is running the script on > a loaded server. Some tests are expected to timeout while for tests a > timeout is a failure. Keeping the default value "5" would be fine as long as it is possible to override externally and get fast results on a mostly-idle machine. Placing a default value in the environment which is overriden by certain tests achieves that. In theory it would also be possible for fcnal-test.sh to parse as "--timeout" option and pass it into every single test but that solution would cause much more code churn. Having default values in environment variables that can still be overridden by command-line arguments is a common pattern in many tools. It also avoids having to pass-through every flag through every intermediate wrapper. -- Regards, Leonard
On 10/6/21 3:26 PM, Leonard Crestez wrote: > > > On 06.10.2021 18:01, David Ahern wrote: >> On 10/6/21 5:47 AM, Leonard Crestez wrote: >>> Reduce default client timeout from 5 seconds to 500 miliseconds. >>> Can be overridden from environment by exporting NETTEST_CLIENT_TIMEOUT=5 >>> >>> Some tests need ICMP timeouts so pass an explicit -t5 for those. >>> >>> Signed-off-by: Leonard Crestez <cdleonard@gmail.com> >>> --- >>> tools/testing/selftests/net/fcnal-test.sh | 17 +++++++++++------ >>> 1 file changed, 11 insertions(+), 6 deletions(-) >>> >> >> The problem with blindly reducing the timeouts is running the script on >> a loaded server. Some tests are expected to timeout while for tests a >> timeout is a failure. > > Keeping the default value "5" would be fine as long as it is possible to > override externally and get fast results on a mostly-idle machine. 5 is the default for nettest.c; the test script passes in -t1 for all tests. > > Placing a default value in the environment which is overriden by certain > tests achieves that. > > In theory it would also be possible for fcnal-test.sh to parse as > "--timeout" option and pass it into every single test but that solution > would cause much more code churn. > > Having default values in environment variables that can still be > overridden by command-line arguments is a common pattern in many tools. > It also avoids having to pass-through every flag through every > intermediate wrapper. I do not agree with env variables here.
On 07.10.2021 04:17, David Ahern wrote: > On 10/6/21 3:26 PM, Leonard Crestez wrote: >> On 06.10.2021 18:01, David Ahern wrote: >>> On 10/6/21 5:47 AM, Leonard Crestez wrote: >>>> Reduce default client timeout from 5 seconds to 500 miliseconds. >>>> Can be overridden from environment by exporting NETTEST_CLIENT_TIMEOUT=5 >>>> >>>> Some tests need ICMP timeouts so pass an explicit -t5 for those. >>>> >>>> Signed-off-by: Leonard Crestez <cdleonard@gmail.com> >>>> --- >>>> tools/testing/selftests/net/fcnal-test.sh | 17 +++++++++++------ >>>> 1 file changed, 11 insertions(+), 6 deletions(-) >>>> >>> >>> The problem with blindly reducing the timeouts is running the script on >>> a loaded server. Some tests are expected to timeout while for tests a >>> timeout is a failure. >> >> Keeping the default value "5" would be fine as long as it is possible to >> override externally and get fast results on a mostly-idle machine. > > 5 is the default for nettest.c; the test script passes in -t1 for all tests. An explicit -t is only passed for some of the tests $ grep -c nettest.*-r tools/testing/selftests/net/fcnal-test.sh 243 $ grep -c nettest.*-t tools/testing/selftests/net/fcnal-test.sh 15 >> Placing a default value in the environment which is overriden by certain >> tests achieves that. >> >> In theory it would also be possible for fcnal-test.sh to parse as >> "--timeout" option and pass it into every single test but that solution >> would cause much more code churn. >> >> Having default values in environment variables that can still be >> overridden by command-line arguments is a common pattern in many tools. >> It also avoids having to pass-through every flag through every >> intermediate wrapper. > > I do not agree with env variables here. Would you agree with adding an option to fcnal-test.sh which decreases timeouts passed to nettest client calls?
On 10/7/21 2:52 PM, Leonard Crestez wrote: > Would you agree with adding an option to fcnal-test.sh which decreases > timeouts passed to nettest client calls? sure
diff --git a/tools/testing/selftests/net/fcnal-test.sh b/tools/testing/selftests/net/fcnal-test.sh index e73aeb3884c5..cf5dd96bb9db 100755 --- a/tools/testing/selftests/net/fcnal-test.sh +++ b/tools/testing/selftests/net/fcnal-test.sh @@ -40,10 +40,15 @@ # Kselftest framework requirement - SKIP code is 4. ksft_skip=4 VERBOSE=0 +# Use a reduced client timeout by default +# Can be overridden by user +NETTEST_CLIENT_TIMEOUT=${NETTEST_CLIENT_TIMEOUT:-0.5} +export NETTEST_CLIENT_TIMEOUT + NSA_DEV=eth1 NSA_DEV2=eth2 NSB_DEV=eth1 NSC_DEV=eth2 VRF=red @@ -1076,11 +1081,11 @@ ipv4_tcp_novrf() for a in ${NSA_LO_IP} 127.0.0.1 do log_start show_hint "Should fail 'No route to host' since addresses on loopback are out of device scope" run_cmd nettest -s -k - run_cmd nettest -r ${a} -d ${NSA_DEV} + run_cmd nettest -r ${a} -d ${NSA_DEV} -t5 log_test_addr ${a} $? 1 "Global server, device client, local connection" done a=${NSA_IP} log_start @@ -1379,23 +1384,23 @@ ipv4_udp_novrf() for a in ${NSA_LO_IP} 127.0.0.1 do log_start show_hint "Should fail since addresses on loopback are out of device scope" run_cmd nettest -D -s -k - run_cmd nettest -D -r ${a} -d ${NSA_DEV} + run_cmd nettest -D -r ${a} -d ${NSA_DEV} -t5 log_test_addr ${a} $? 2 "Global server, device client, local connection" log_start show_hint "Should fail since addresses on loopback are out of device scope" run_cmd nettest -D -s -k - run_cmd nettest -D -r ${a} -d ${NSA_DEV} -C + run_cmd nettest -D -r ${a} -d ${NSA_DEV} -C -t5 log_test_addr ${a} $? 1 "Global server, device send via cmsg, local connection" log_start show_hint "Should fail since addresses on loopback are out of device scope" run_cmd nettest -D -s -k - run_cmd nettest -D -r ${a} -d ${NSA_DEV} -S + run_cmd nettest -D -r ${a} -d ${NSA_DEV} -S -t5 log_test_addr ${a} $? 1 "Global server, device client via IP_UNICAST_IF, local connection" done a=${NSA_IP} log_start @@ -3443,11 +3448,11 @@ netfilter_icmp() for a in ${NSA_IP} ${VRF_IP} do log_start run_cmd nettest ${arg} -s -k - run_cmd_nsb nettest ${arg} -r ${a} + run_cmd_nsb nettest ${arg} -r ${a} -t5 log_test_addr ${a} $? 1 "Global ${stype} server, Rx reject icmp-port-unreach" done } ipv4_netfilter() @@ -3498,11 +3503,11 @@ netfilter_icmp6() for a in ${NSA_IP6} ${VRF_IP6} do log_start run_cmd nettest -6 -s ${arg} -k - run_cmd_nsb nettest -6 ${arg} -r ${a} + run_cmd_nsb nettest -6 ${arg} -r ${a} -t5 log_test_addr ${a} $? 1 "Global ${stype} server, Rx reject icmp-port-unreach" done } ipv6_netfilter()
Reduce default client timeout from 5 seconds to 500 miliseconds. Can be overridden from environment by exporting NETTEST_CLIENT_TIMEOUT=5 Some tests need ICMP timeouts so pass an explicit -t5 for those. Signed-off-by: Leonard Crestez <cdleonard@gmail.com> --- tools/testing/selftests/net/fcnal-test.sh | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)