On Fri, 5 Jan 2024 at 03:03, Ilya Maximets <[email protected]> wrote: > On 1/4/24 11:57, Simon Horman wrote: >> One question from my side is, given that this is currently broken in many >> kernels in use today, how we should integrate this. For one thing, >> applying this patch causes the CI to fail. >> >> https://github.com/ovsrobot/ovs/actions/runs/7405341045 >> >> It might be nice if we could detect known to be broken kernels. >> But I'm not sure, there is an easy way to do that, other than >> running the test itself. >> >> Do you have any thoughts on this? > > One option could be to exclude the kernels below 6.7 with: > > OVS_CHECK_KERNEL(6, 7) > > Unfortunately the original issue seems to be backported to some > distribution kernels, but all the kernels 6.7+ should be fine. > > However, I'm not convinced the test failed in CI because of this. > They failed due to difference on packet and by counters. > In case of offload tests they also have matching packet counters, > but different byte counters. I know there were cases where TC > counts bytes differently. So, the counters seem to be not a > very reliable source of information. Is there any other way > we can detect the issue without comparing exact values of the > packet/byte counters?
Thanks all for the quick feedback! I've now submitted a V2 patch which addresses both issues raised: 1. Skip running the test on kernel versions older than 6.7 2. Remove byte counters to fix the offloads system tests On Fri, 5 Jan 2024 at 04:40, Aaron Conole <[email protected]> wrote: > We should consider putting this test into the kernel tree instead of > housing it in the OVS tree. We're really using ovs here as a glorified > flow generator, but maybe we can just take the flows that it spits out > and use them in tools/testing/selftests/net/openvswitch/openvswitch.sh > with ovs-dpctl.py and update the test_nat_connect_v4 with this check. > That would also have the advantage of keeping the tests with the kernel, > since it is a datapath specific bug. Agree, this should be tested in the kernel to catch any potential future regressions before they are merged. I've attached a quick patch I wrote today which adds a basic kernel self-test using the same ruleset as the ovs test I created. Was interested in getting some early feedback here on this list first, and if positive, I can clean this patch up and submit to the kernel next week. By the way, I'm happy if you guys decide not to merge my ovs patch and you just want to test this in the kernel, however I do wonder if there's some additional benefit to having the test in both the kernel and ovs. This would allow package builders to run the ovs tests to sanity check if the kernel they are building on has working NAT support or not. Especially given Ilya's point that ebddb1404900 has been backported to many different distro kernels. --- .../selftests/net/openvswitch/openvswitch.sh | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh index f8499d4c87f3..a1ba0c64f263 100755 --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh @@ -17,6 +17,7 @@ tests=" ct_connect_v4 ip4-ct-xon: Basic ipv4 tcp connection using ct connect_v4 ip4-xon: Basic ipv4 ping between two NS nat_connect_v4 ip4-nat-xon: Basic ipv4 tcp connection via NAT + nat_related_v4 ip4-nat-related-xon: ICMP related via SNAT netlink_checks ovsnl: validate netlink attrs and settings upcall_interfaces ovs: test the upcall interfaces drop_reason drop: test drop reasons are emitted" @@ -473,6 +474,54 @@ test_nat_connect_v4 () { return 0 } +# nat_related_v4 test +test_nat_related_v4 () { + which nc >/dev/null 2>/dev/null || return $ksft_skip + + sbx_add "test_nat_related_v4" || return $? + + ovs_add_dp "test_nat_related_v4" natrelated4 || return 1 + info "create namespaces" + for ns in client server; do + ovs_add_netns_and_veths "test_nat_related_v4" "natrelated4" "$ns" \ + "${ns:0:1}0" "${ns:0:1}1" || return 1 + done + + ip netns exec client ip addr add 172.31.110.10/24 dev c1 + ip netns exec client ip link set c1 up + ip netns exec server ip addr add 172.31.110.20/24 dev s1 + ip netns exec server ip link set s1 up + + ip netns exec server ip route add 192.168.0.20/32 via 172.31.110.10 + + ovs_add_flow "test_nat_related_v4" natrelated4 \ + 'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1 + ovs_add_flow "test_nat_related_v4" natrelated4 \ + 'in_port(2),eth(),eth_type(0x0806),arp()' '1' || return 1 + ovs_add_flow "test_nat_related_v4" natrelated4 \ + "ct_state(-trk),in_port(1),eth(),eth_type(0x0800),ipv4(dst=172.31.110.20)" \ + "ct(commit,nat(src=192.168.0.20)),recirc(0x1)" + ovs_add_flow "test_nat_related_v4" natrelated4 \ + "ct_state(-trk),in_port(2),eth(),eth_type(0x0800),ipv4()" \ + "ct(commit,nat),recirc(0x2)" + + ovs_add_flow "test_nat_related_v4" natrelated4 \ + "recirc_id(0x1),ct_state(+trk-inv),in_port(1),eth(),eth_type(0x0800),ipv4()" "2" + ovs_add_flow "test_nat_related_v4" natrelated4 \ + "recirc_id(0x2),ct_state(+rel+trk),in_port(2),eth(),eth_type(0x0800),ipv4(src=172.31.110.20,dst=172.31.110.10,proto=1),icmp()" "1" + ovs_add_flow "test_nat_related_v4" natrelated4 \ + "recirc_id(0x2),ct_state(+rel+trk),in_port(2),eth(),eth_type(0x0800),ipv4(dst=192.168.0.20,proto=1),icmp()" "drop" + + # Solicit destination unreachable response from server + ovs_sbx "test_nat_connect_v4" ip netns exec client bash -c "echo a | nc -u -w 1 172.31.110.20 10000" + + # Check to make sure no packets were dropped (matched rule with incorrect IP) + python3 "$ovs_base/ovs-dpctl.py" dump-flows natrelated4 | grep "drop" | grep "packets:0" || return 1 + + info "done..." + return 0 +} + # netlink_validation # - Create a dp # - check no warning with "old version" simulation -- 2.34.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
