On 7/8/25 5:50 PM, Adrián Moreno wrote: > On Mon, Jun 30, 2025 at 07:09:57PM +0200, Ilya Maximets wrote: >> Retis is very useful for debugging our system tests or debugging >> kernel issues through our system tests. This change adds a convenient >> way to run any kernel system test with the retis capture on the >> background. E.g.: >> >> make check-kernel OVS_TEST_WITH_RETIS=yes TESTSUITEFLAGS='167 -d' >> >> Retis 1.5 is required, since we're using ifdump profile, and it also >> will mount debugfs for us in case of running in a different namespace. >> It should be available in $PATH. >> >> In addition to just capturing the retis.data, we're also running the >> capture with --print to print all the events as they appear, and >> producing the sorted output in the end. This makes it easier to work >> across systems with different versions of retis and saves time for >> running the sort manually. The raw data is still available for >> advanced processing, if needed. >> >> Not specifying any particular collector, capturing everything that's >> enabled by default. OVS tracking is turned on by default. >> >> Since OVS tracking is used, it's required to start retis after the >> kernel datapath is created, otherwise it will fail to obtain the map >> of upcall PIDs. That's why we need to start it after the bridge is >> created. >> >> Only adding support for kernel-related test suites for now. For >> userspace test suites it may also be useful at some point, but >> currently that requires running without --ovs-track and isn't too >> important. >> >> Startup of the retis capture adds significant amount of time to each >> test, so not running it by default. > > That will hopefully be improved in 1.6 :-). > >> >> Link: https://github.com/retis-org/retis >> Signed-off-by: Ilya Maximets <i.maxim...@ovn.org> >> --- >> Documentation/topics/testing.rst | 22 ++++++++++++++++++++++ >> NEWS | 3 +++ >> tests/ovs-macros.at | 10 ++++++++++ >> tests/system-kmod-macros.at | 3 +++ >> tests/system-offloads-testsuite-macros.at | 3 +++ >> 5 files changed, 41 insertions(+) >> >> diff --git a/Documentation/topics/testing.rst >> b/Documentation/topics/testing.rst >> index b97bf32a9..78f38ecf5 100644 >> --- a/Documentation/topics/testing.rst >> +++ b/Documentation/topics/testing.rst >> @@ -455,6 +455,28 @@ datapath testsuite. >> >> https://git.kernel.org/pub/scm/network/iproute2/iproute2.git >> >> +It is also possible to run `retis`_ capture along with the tests by setting >> +`OVS_TEST_WITH_RETIS` environment variable to 'yes'. This can be useful for >> +debugging the test cases. For example, the following command can be used to >> +run the test 167 under retis:: >> + >> + $ make check-kernel OVS_TEST_WITH_RETIS=yes TESTSUITEFLAGS='167 -d' >> + >> +After the test is completed, the following data will be available in the >> test >> +directory: >> + >> +* `retis.err` - standard error stream of the `retis collect`. >> +* `retis.log` - standard output of the `retis collect`, contains all >> captured >> + events in the order they appeared. >> +* `retis.data` - raw events collected by retis, `retis sort` or other >> commands >> + can be used on this file for further analysis. >> +* `retis.sorted` - text file containing the output of `retis sort` executed >> on >> + the `retis.data`, for convenience. >> + >> +Requires retis version 1.5 or newer. >> + >> +.. _retis: https://github.com/retis-org/retis >> + > > It might be worth mentioning it requires compiling with > --enable-usdt-probes (--ovs-track will fail otherwise).
Good point. Want to submit a patch? :) It may also be good if retis just warned about this and didn't refuse to start. > >> .. _testing-static-analysis: >> >> Static Code Analysis >> diff --git a/NEWS b/NEWS >> index d7231fabc..3439a70d6 100644 >> --- a/NEWS >> +++ b/NEWS >> @@ -24,6 +24,9 @@ Post-v3.5.0 >> - ovs-tcpdump: >> * Update the --mirror-to option, adding support for specifying an >> existing port as a mirror interface. >> + - Added support for running tests from 'check-kernel' and >> 'check-offloads' >> + system test targets under retis by setting OVS_TEST_WITH_RETIS=yes. >> + See the 'Testing' section of the documentation for more details. >> - Updated documentation to remove most documentation of kernel module >> that was previously part of the OVS distribution. It was removed from >> the OVS distribution in the 3.0 release and is no longer present in >> diff --git a/tests/ovs-macros.at b/tests/ovs-macros.at >> index 738cda2e4..132a00541 100644 >> --- a/tests/ovs-macros.at >> +++ b/tests/ovs-macros.at >> @@ -360,6 +360,16 @@ m4_ifndef([AT_FAIL_IF], >> [AT_CHECK([($1) \ >> && exit 99 || exit 0], [0], [ignore], [ignore])])]) >> >> +dnl Start retis to track all the traffic passing through OVS. >> +m4_define([RETIS_CHECK_AND_RUN], >> + [if test "$OVS_TEST_WITH_RETIS" = yes && retis --version > /dev/null; then >> + on_exit 'retis sort --utc retis.data > retis.sorted' > > IIUC, this will run "retis sort" before "retis collect" is done, which > could lead to some issues because retis json output is buffered. It > would be safer to first kill collection. The on_exit hooks are executed in the reverse order. So, the process will first be killed by the hook inside the OVS_DAEMONIZE and then we'll sort. This doesn't guarantee that the process is actually fully stopped, but there is no good way to ensure that, especially on failure. Seems to work fine as-is though. > >> + OVS_DAEMONIZE([retis -p ifdump collect --utc --allow-system-changes \ >> + --ovs-track --out --print 2>retis.err >> 1>retis.log], >> + [retis.pid]) > > Capturing on ifdump without a filter can fill the event file of a lot of > unwanted traffic. > Providing a useful global filter (e.g: OVS_TEST_RETIS_FILTER) would be > difficult for anything but one or two related tests. I don't think we care much. The amount of traffic that is being sent during the test is not that high. > > Adding a filter per test would be nice since each test knows what > traffic is expected but that'd mean ... well ... modifying each test. > > We could do something like "retis collect -p ovs_vport_receive" to only > catch packets going inside OVS. This could go into a profile file that is > easy to modify. WDYT? I'm not sure if it's worth limiting the capture for the test debugging cases. The retis capture is the last resort measure when you do not understand what's happening. Having more events seems better than having a chance to miss something important. They can be filtered out later, if needed. > >> + OVS_WAIT_UNTIL([grep -q 'loaded' retis.err]) >> + fi]) >> + >> dnl Add a rule to always accept the traffic. >> dnl The first argument to this macro should be the command to run: >> dnl iptables or ip6tables >> diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at >> index f7e9ff689..1de0e616b 100644 >> --- a/tests/system-kmod-macros.at >> +++ b/tests/system-kmod-macros.at >> @@ -29,6 +29,9 @@ m4_define([OVS_TRAFFIC_VSWITCHD_START], >> _OVS_VSWITCHD_START([], [$3]) >> dnl Add bridges, ports, etc. >> AT_CHECK([ovs-vsctl -- _ADD_BR([br0]) -- $1 m4_if([$2], [], [], [| >> uuidfilt])], [0], [$2]) >> + >> + dnl Start retis capture if requested. >> + RETIS_CHECK_AND_RUN() >> ]) >> >> # OVS_TRAFFIC_VSWITCHD_STOP([ALLOWLIST], [extra_cmds]) >> diff --git a/tests/system-offloads-testsuite-macros.at >> b/tests/system-offloads-testsuite-macros.at >> index e6d044d21..c80538fd6 100644 >> --- a/tests/system-offloads-testsuite-macros.at >> +++ b/tests/system-offloads-testsuite-macros.at >> @@ -27,6 +27,9 @@ m4_define([OVS_TRAFFIC_VSWITCHD_START], >> _OVS_VSWITCHD_START([], [-- set Open_vSwitch . >> other_config:hw-offload=true $3]) >> dnl Add bridges, ports, etc. >> AT_CHECK([ovs-vsctl -- _ADD_BR([br0]) -- $1 m4_if([$2], [], [], [| >> uuidfilt])], [0], [$2]) >> + >> + dnl Start retis capture if requested. >> + RETIS_CHECK_AND_RUN() >> ]) >> >> # Macro to exclude tests that will fail with TC offload enabled. >> -- >> 2.49.0 >> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
