Enable traffic monitoring to debug flaky selftests #168
Conversation
|
@ameryhung If I understand correctly, the traffic monitoring logs are printed to files. In github UI only stdout is going to be visible, the logs stored in files will be lost by default. So on CI you may want to either dump the logs to stdout, or collect them into a separate artifact. |
|
I will change to upload the log file as well. Originally, I thought "-m" also prints to stdout. However, it does not include the payload. Having it should be more helpful for debugging. |
|
You can ignore GCC BPF tests failure. They are disabled at kernel-patches/bpf, I'll disable them here too shortly. |
|
I just noticed there is an older similar PR already: #130 There is a piece of yaml uploading logs as artifacts there. Re stdout vs artifacts, consider the verbosity of the monitoring log. If it's lots of lines per test, then I'd go with the artifacts. |
|
I think uploading tmon logs as artifacts makes sense. I will update the PR. Thanks! |
run-vmtest/action.yml
Outdated
| - uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: tmon-logs-${{ inputs.arch }}-${{ inputs.toolchain_full }}${{ inputs.kernel-test }} | ||
| if-no-files-found: error | ||
| path: "/tmp/tmon_pcap/*" |
There was a problem hiding this comment.
Managing artifacts is really a job-level task IMO. And we should avoid new action inputs if possible.
I think in this case, it makes sense to add an always() (link) step in kernel-test workflow after run-vmtest call step. toolchain_full string is directly available there, since the build artifacts are identified by it. Also, with always() condition, logs will be uploaded on failure, which is what we want.
Later we may want to add other log files to the step (but don't worry about that in this PR).
ameryhung
force-pushed
the
enable_bpf_selftest_traffic_monitoring
branch
from
8ec43a2 to
aa63759
Compare
run-vmtest/run-bpf-selftests.sh
Outdated
| @@ -43,7 +43,7 @@ test_progs_helper() { | |||
| # "&& true" does not change the return code (it is not executed | |||
| # if the Python script fails), but it prevents exiting on a | |||
| # failure due to the "set -e". | |||
| ./${selftest} ${args} --json-summary "${json_file}" && true | |||
| ./${selftest} ${args} -m '*' --json-summary "${json_file}" && true | |||
There was a problem hiding this comment.
nit: adding -m to args was ok too
There was a problem hiding this comment.
Do you mean what I had before (i.e., "args+=" -m ''")? That didn't work because the single quote inside the double quote was not evaluated and removed so test_progs would see '' instead of *. I can replace the args with arrays.
local args=("$2")
args+=("-m")
args+=("*")
./test_progs "${args[@]}"
There was a problem hiding this comment.
That didn't work because the single quote inside the double quote was not evaluated and removed so test_progs would see '' instead of *.
Hmm... that should work.
Consider this:
args+=" ${w:+-w$w}"
args+=" -m '*'"
echo ./foo "$args" bar$ ./x.sh
./foo -m '*' bar
$ w=123 ./x.sh
./foo -w123 -m '*' barAnyways, this is a nit thing, don't spend too much time on it.
ameryhung
force-pushed
the
enable_bpf_selftest_traffic_monitoring
branch
from
aa63759 to
773e073
Compare
|
IMHO uploading the artifact is not wrong and is generally useful. But it's probably a bit more useful to get the output from stdout/stderr. We already have infra in KPD that slurps failed selftest output and emails it out to contributors. Would be nice to leverage that. If it's just a matter of improving the selftest infra in kernel tree then maybe @mlau2 can help. |
ameryhung
force-pushed
the
enable_bpf_selftest_traffic_monitoring
branch
from
773e073 to
a37ec9e
Compare
@danobi dumping everything to stdout/stderr is ok up to a certain point. An extreme bad example is selftests/sched_ext which prints megabytes of log to stdout. This makes github ui to freeze, and is generally not convenient to work with. The users of these logs should consider its verbosity, format and situations when it is actually needed. I assumed traffic monitoring might be pretty verbose, so uploading as separate artifacts made sense to me. I just downloaded an artifact from a successfull test run, and it appears that the "logs" are actually some binary data (@ameryhung I don't really understand what it is, so if that's not right lmk): If this is expected, then there is just no point in printing it to stdout: it's unreadable. |
|
@theihor The logs indeed are binary data and can be inspected using tcpdump. E.g., tcpdump -X -r <packet_log> will show packets with their payloads |
@ameryhung thank you, makes sense. Well it's 1.1Mb of the logs in total right now, and tcpdump-ing them is gonna be lots of lines. So let's use the artifacts. We only need to make sure that relevant people are aware that this data is available. |
Convert args to array so that later we can pass "-m *" to test_progs through args. Currently, with args being string, it is not possible. Doing args+="-m *" will result in the wildcard being expanded. Doing args+="-m '*'" will result in single quoted wildcard being passed to test_progs. Signed-off-by: Amery Hung <[email protected]>
Traffic monitoring was add to the selftests/bpf[0] to debug some flaky test cases. Enable this in CI by building selftests with libpcap, installing libpcap in the test environment and finally turn it on when running the test. [0] https://lore.kernel.org/bpf/[email protected]/ Signed-off-by: Amery Hung <[email protected]>
After enabling traffic monitor in bpf selftests, upload the log files as artifacts. Signed-off-by: Amery Hung <[email protected]>
ameryhung
force-pushed
the
enable_bpf_selftest_traffic_monitoring
branch
from
a37ec9e to
505d233
Compare
|
I just know I have a mlau2 account :) In reply to @danobi about stdout/stderr, it does log to stdout/stderr (can't remember which one). I believe it was unnoticed because the test has passed. There is no details output by default for successful test. Using "test_progs -v" should show it. Like: #> ./test_progs -v -m '*' -t tc_redirect/tc_redirect_dtime |
Fair enough. In those cases the artifacts will be useful. And as far as I know, it's free on GHA. |
Traffic monitoring was add to the selftests/bpf[0] to debug some flaky test cases. Enable this in CI by building selftests with libpcap, installing libpcap in the test environment and finally turn it on when running the test.
[0] https://lore.kernel.org/bpf/[email protected]/