| Message ID | 20240723182439.1434795-3-thinker.li@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | monitor network traffic for flaky test cases | expand |
On Tue, 2024-07-23 at 11:24 -0700, Kui-Feng Lee wrote: > Enable traffic monitoring for the test case > tc_redirect/tc_redirect_dtime. > > Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com> > --- > tools/testing/selftests/bpf/prog_tests/tc_redirect.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c > b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c > index 327d51f59142..1be6ea8d6c64 100644 > --- a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c > +++ b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c > @@ -900,6 +900,7 @@ static void test_udp_dtime(struct test_tc_dtime > *skel, int family, bool bpf_fwd) > static void test_tc_redirect_dtime(struct netns_setup_result > *setup_result) > { > struct test_tc_dtime *skel; > + struct tmonitor_ctx *tmon = NULL; nit: No need to set "tmon" to NULL here. > struct nstoken *nstoken; > int hold_tstamp_fd, err; > > @@ -934,6 +935,9 @@ static void test_tc_redirect_dtime(struct > netns_setup_result *setup_result) > if (!ASSERT_OK(err, "disable forwarding")) > goto done; > > + tmon = traffic_monitor_start(NS_DST); > + ASSERT_NEQ(tmon, NULL, "traffic_monitor_start"); > + > test_tcp_clear_dtime(skel); > > test_tcp_dtime(skel, AF_INET, true); > @@ -958,6 +962,7 @@ static void test_tc_redirect_dtime(struct > netns_setup_result *setup_result) > test_udp_dtime(skel, AF_INET6, false); > > done: > + traffic_monitor_stop(tmon); > test_tc_dtime__destroy(skel); > close(hold_tstamp_fd); > }
On 07/23, Kui-Feng Lee wrote:
> Enable traffic monitoring for the test case tc_redirect/tc_redirect_dtime.
Alternatively, we might extend test_progs to have some new generic
arg to enable trafficmon for a given set of tests (and then pass this
flag in the CI):
./test_progs --traffic_monitor=t1,t2,t3...
Might be useful in case we need to debug some other test in the future.
On 7/24/24 01:36, Geliang Tang wrote: > On Tue, 2024-07-23 at 11:24 -0700, Kui-Feng Lee wrote: >> Enable traffic monitoring for the test case >> tc_redirect/tc_redirect_dtime. >> >> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com> >> --- >> tools/testing/selftests/bpf/prog_tests/tc_redirect.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c >> b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c >> index 327d51f59142..1be6ea8d6c64 100644 >> --- a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c >> +++ b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c >> @@ -900,6 +900,7 @@ static void test_udp_dtime(struct test_tc_dtime >> *skel, int family, bool bpf_fwd) >> static void test_tc_redirect_dtime(struct netns_setup_result >> *setup_result) >> { >> struct test_tc_dtime *skel; >> + struct tmonitor_ctx *tmon = NULL; > > nit: No need to set "tmon" to NULL here. Earlier "goto done" statements would need tmon to be initialized. Does that make sense? > >> struct nstoken *nstoken; >> int hold_tstamp_fd, err; >> >> @@ -934,6 +935,9 @@ static void test_tc_redirect_dtime(struct >> netns_setup_result *setup_result) >> if (!ASSERT_OK(err, "disable forwarding")) >> goto done; >> >> + tmon = traffic_monitor_start(NS_DST); >> + ASSERT_NEQ(tmon, NULL, "traffic_monitor_start"); >> + >> test_tcp_clear_dtime(skel); >> >> test_tcp_dtime(skel, AF_INET, true); >> @@ -958,6 +962,7 @@ static void test_tc_redirect_dtime(struct >> netns_setup_result *setup_result) >> test_udp_dtime(skel, AF_INET6, false); >> >> done: >> + traffic_monitor_stop(tmon); >> test_tc_dtime__destroy(skel); >> close(hold_tstamp_fd); >> } >
On 7/24/24 08:26, Stanislav Fomichev wrote: > On 07/23, Kui-Feng Lee wrote: >> Enable traffic monitoring for the test case tc_redirect/tc_redirect_dtime. > > Alternatively, we might extend test_progs to have some new generic > arg to enable trafficmon for a given set of tests (and then pass this > flag in the CI): > > ./test_progs --traffic_monitor=t1,t2,t3... > > Might be useful in case we need to debug some other test in the future. We run a few test cases with network namespaces. So we need to specify namespaces to monitor. And, these namespaces are not created yet when a test starts. To adapt this approach, these test cases should be changed to use a generic way that create network namespaces when a test starts. Or, we just monitor default network namespace. For test cases with network namespaces, they need to call these functions. WDYT?
On 07/24, Kui-Feng Lee wrote: > > > On 7/24/24 08:26, Stanislav Fomichev wrote: > > On 07/23, Kui-Feng Lee wrote: > > > Enable traffic monitoring for the test case tc_redirect/tc_redirect_dtime. > > > > Alternatively, we might extend test_progs to have some new generic > > arg to enable trafficmon for a given set of tests (and then pass this > > flag in the CI): > > > > ./test_progs --traffic_monitor=t1,t2,t3... > > > > Might be useful in case we need to debug some other test in the future. > > We run a few test cases with network namespaces. So we need to > specify namespaces to monitor. And, these namespaces are not created > yet when a test starts. To adapt this approach, these test cases should > be changed to use a generic way that create network namespaces when > a test starts. > > Or, we just monitor default network namespace. For test cases with > network namespaces, they need to call these functions. > > WDYT? Ah, true, in this case ignore me :-)
diff --git a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c index 327d51f59142..1be6ea8d6c64 100644 --- a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c +++ b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c @@ -900,6 +900,7 @@ static void test_udp_dtime(struct test_tc_dtime *skel, int family, bool bpf_fwd) static void test_tc_redirect_dtime(struct netns_setup_result *setup_result) { struct test_tc_dtime *skel; + struct tmonitor_ctx *tmon = NULL; struct nstoken *nstoken; int hold_tstamp_fd, err; @@ -934,6 +935,9 @@ static void test_tc_redirect_dtime(struct netns_setup_result *setup_result) if (!ASSERT_OK(err, "disable forwarding")) goto done; + tmon = traffic_monitor_start(NS_DST); + ASSERT_NEQ(tmon, NULL, "traffic_monitor_start"); + test_tcp_clear_dtime(skel); test_tcp_dtime(skel, AF_INET, true); @@ -958,6 +962,7 @@ static void test_tc_redirect_dtime(struct netns_setup_result *setup_result) test_udp_dtime(skel, AF_INET6, false); done: + traffic_monitor_stop(tmon); test_tc_dtime__destroy(skel); close(hold_tstamp_fd); }
Enable traffic monitoring for the test case tc_redirect/tc_redirect_dtime. Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com> --- tools/testing/selftests/bpf/prog_tests/tc_redirect.c | 5 +++++ 1 file changed, 5 insertions(+)