On 6/24/24 15:14, Adrián Moreno wrote: > On Mon, Jun 24, 2024 at 02:03:00PM GMT, Ilya Maximets wrote: >> On 6/5/24 22:23, Adrian Moreno wrote: >>> Test simultaneous IPFIX and local sampling including slow-path. >>> >>> Signed-off-by: Adrian Moreno <amore...@redhat.com> >>> --- >>> tests/system-common-macros.at | 4 ++ >>> tests/system-traffic.at | 105 ++++++++++++++++++++++++++++++++++ >>> 2 files changed, 109 insertions(+) >>> >> >> Not a full review, but see a few comments below. >> >>> diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at >>> index 2a68cd664..22b8885e4 100644 >>> --- a/tests/system-common-macros.at >>> +++ b/tests/system-common-macros.at >>> @@ -378,3 +378,7 @@ m4_define([OVS_CHECK_GITHUB_ACTION], >>> # OVS_CHECK_DROP_ACTION() >>> m4_define([OVS_CHECK_DROP_ACTION], >>> [AT_SKIP_IF([! grep -q "Datapath supports explicit drop action" >>> ovs-vswitchd.log])]) >>> + >>> +# OVS_CHECK_EMIT_SAMPLE() >>> +m4_define([OVS_CHECK_EMIT_SAMPLE], >>> + [AT_SKIP_IF([! grep -q "Datapath supports emit_sample" >>> ovs-vswitchd.log])]) >>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at >>> index bd7647cbe..babc56b56 100644 >>> --- a/tests/system-traffic.at >>> +++ b/tests/system-traffic.at >>> @@ -8977,3 +8977,108 @@ OVS_WAIT_UNTIL([cat p2.pcap | grep -E "0x0050: >>> *0000 *0000 *5002 *2000 *b85e *00 >>> >>> OVS_TRAFFIC_VSWITCHD_STOP >>> AT_CLEANUP >>> + >>> +AT_SETUP([emit_sample]) >> >> Other tests have a test group name at the beginning. >> You're also adding the test into NSH section. Add a new section, e.g., >> 'sampling' for this with AT_BANNER. >> > > Thanks. Will do. > >>> +OVS_TRAFFIC_VSWITCHD_START() >>> +OVS_CHECK_EMIT_SAMPLE() >>> + >>> +ADD_NAMESPACES(at_ns0, at_ns1) >>> +NS_CHECK_EXEC([at_ns0], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], >>> [ignore]) >>> +NS_CHECK_EXEC([at_ns1], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], >>> [ignore]) >> >> This looks strange, don't disable IPv6. Appropriate datapath actions >> should not forward IPv6 traffic, if that is a desired behavior. >> > > I found tests cleaner if automatic ipv6 traffic is not sent by ports. > But sure, I can deal with that with OpenFlow.
Would be good to have two separate tests. One for IPv4 and one for IPv6. > >> Though, I guess, it would be nice to test capturing both IPv4 and IPv6. >> >>> + >>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "e4:11:22:33:44:55") >>> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", "e4:11:22:33:44:66") >>> + >>> +NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.2 e4:11:22:33:44:66]) >>> +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.1 e4:11:22:33:44:55]) >>> + >>> + >>> +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg >>> ofproto_dpif_upcall:dbg]) >>> + >>> +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \ >>> + -- --id=@ipfix create IPFIX targets=\"127.0.0.1:4739\" >>> \ >>> + -- create Flow_Sample_Collector_Set id=1 bridge=@br0 >>> ipfix=@ipfix, local_sample_group=10 \ >>> + -- create Flow_Sample_Collector_Set id=2 bridge=@br0 >>> ipfix=@ipfix, local_sample_group=12], >>> + [0], [ignore]) >> >> Why are we adding IPFIX? This should work without IPFIX. >> > > It is intentional. I want to test both features work simulteaneously. Tests should be simple. At least, we should have at least one very simple test, so if it fails we know that there is an issue in the basic logic. It's valuable to test a combination of the two, but it should be a separate test. So, 3 tests so far: IPv4, IPv6, psample+ipfix. Maybe the 4th separate test for the slow path. > >> Having both together can be a separate test. >> >>> + >>> +AT_DATA([flows.txt], [dnl >>> +in_port=ovs-p0,ip >>> actions=sample(probability=65535,collector_set_id=1,obs_domain_id=1431655765,obs_point_id=1717986918),output(port=ovs-p1,max_len=100) >>> +in_port=ovs-p1,ip >>> actions=sample(probability=65535,collector_set_id=2,obs_domain_id=2290649224,obs_point_id=2576980377),output(port=ovs-p0,max_len=100) >> >> It should be possible to wrap these lines a little with dnl. >> >> Also, please make IDs non-symmetric, so we can't check that the >> byte order is correct. >> > > Originally, I thought just concatenating the values in native endianness > as a kind of replacement for the user_action_cookie (hence the symmetric > values). But I'm having second thoughts. I plan to change it in the next > version and store it in "network" order. I will change the test to > verify it accordingly. I'm not sure I get that, but just in case: OpenFlow messages themselves should be encoded with a network byte order. When printed to the user, they should be printed in a host byte order. Netlink messages typically have values in the host byte order, but there are some weird netlink messages that expect network byte order. In any case, the psample test utility should print out values in the host byte order. > >>> +]) >>> + >>> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) >>> + >>> +OVS_DAEMONIZE([ovstest test-psample > psample.out], [psample1.pid]) >> >> Should we wait for the test app to start listening? i.e. there might be >> a race between the listener and the ping below. >> > > I guess so. > >>> + >>> +NS_CHECK_EXEC([at_ns0], [ping -q -c 1 10.1.1.2 | FORMAT_PING], [0], [dnl >>> +1 packets transmitted, 1 received, 0% packet loss, time 0ms >>> +]) >> >> We may need to add -W to the ping. >> > > Ack. > >>> + >>> +m4_define([SAMPLE1], [group_id=0xa >>> obs_domain=0x55555555,obs_point=0x66666666 >>> .*icmp.*nw_src=10.1.1.1,nw_dst=10.1.1.2]) >>> +m4_define([SAMPLE2], [group_id=0xc >>> obs_domain=0x88888888,obs_point=0x99999999 >>> .*icmp.*nw_src=10.1.1.2,nw_dst=10.1.1.1]) >> >> May use m4_join to wrap these lines. >> >>> +AT_CHECK([grep -E 'SAMPLE1' psample.out >/dev/null]) >>> +AT_CHECK([grep -E 'SAMPLE2' psample.out >/dev/null]) >> >> 'grep -q' to avoid the output redirection. We may need to wait for them >> also, >> i.e. OVS_WAIT_UNTIL. >> > > Ack. > >>> + >>> +AT_CHECK([ovs-ofctl dump-ipfix-flow br0 | sed 's/tx pkts=[[0-9]]*/tx >>> pkts=24/' | sed 's/tx errs=[[0-9]]*/tx errs=0/'], [0], >> >> Wrap the line with '\'. >> > > Ack. > >> [dnl >>> +NXST_IPFIX_FLOW reply (xid=0x2): 2 ids >>> + id 1: flows=1, current flows=0, sampled pkts=1, ipv4 ok=1, ipv6 ok=0, >>> tx pkts=24 >>> + pkts errs=0, ipv4 errs=0, ipv6 errs=0, tx errs=0 >>> + id 2: flows=1, current flows=0, sampled pkts=1, ipv4 ok=1, ipv6 ok=0, >>> tx pkts=24 >>> + pkts errs=0, ipv4 errs=0, ipv6 errs=0, tx errs=0 >>> +]) >>> + >>> +AT_CHECK([ovs-appctl lsample/show br0], [0], [dnl >>> +local sample statistics for bridge "br0": >>> +- Collector Set ID: 1 >>> + Local Sample Group: 10 >>> + Total number of bytes: 98 >>> + Total number of packets: 1 >>> + >>> +- Collector Set ID: 2 >>> + Local Sample Group: 12 >>> + Total number of bytes: 98 >>> + Total number of packets: 1 >>> +]) >>> + >>> +# Disable trunc feature to force traffic to go through slow path. >> >> dnl >> >>> +AT_CHECK([ovs-appctl dpif/set-dp-features br0 trunc false]) >>> + >>> +AT_CHECK([ovs-appctl ofproto/trace br0 >>> 'in_port=ovs-p0,dl_src=e4:11:22:33:44:55,dl_dst=e4:11:22:33:44:66,dl_type=0x0800,nw_src=10.1.1.1,nw_dst=10.1.1.12'], >>> [0], [stdout]) >> >> Can be wrapped with m4_join. >> >>> +AT_CHECK([tail -3 stdout], [0], [dnl >>> +Datapath actions: >>> emit_sample(group=10,cookie=5555555566666666),userspace(pid=4294967295,flow_sample(probability=65535,collector_set_id=1,obs_domain_id=1431655765,obs_point_id=1717986918,output_port=4294967295)),trunc(100),3 >>> +This flow is handled by the userspace slow path because it: >>> + - Uses action(s) not supported by datapath. >>> +]) >> >> Wrap. >> >>> + >>> +OVS_DAEMONIZE([ovstest test-psample > psample_slow.out], >>> [psample_slow.pid]) >> >> Wait? >> >>> + >>> +NS_CHECK_EXEC([at_ns0], [ping -q -c 1 10.1.1.2 | FORMAT_PING], [0], [dnl >>> +1 packets transmitted, 1 received, 0% packet loss, time 0ms >>> +]) >> >> -W >> >>> + >>> +AT_CHECK([grep -E 'SAMPLE1' psample_slow.out >/dev/null]) >>> +AT_CHECK([grep -E 'SAMPLE2' psample_slow.out >/dev/null]) >> >> -q, wait >> >>> + >>> +AT_CHECK([ovs-ofctl dump-ipfix-flow br0 | sed 's/tx pkts=[[0-9]]*/tx >>> pkts=24/' | sed 's/tx errs=[[0-9]]*/tx errs=0/'], [0], [dnl >> >> wrap. >> >>> +NXST_IPFIX_FLOW reply (xid=0x2): 2 ids >>> + id 1: flows=2, current flows=0, sampled pkts=2, ipv4 ok=2, ipv6 ok=0, >>> tx pkts=24 >>> + pkts errs=0, ipv4 errs=0, ipv6 errs=0, tx errs=0 >>> + id 2: flows=2, current flows=0, sampled pkts=2, ipv4 ok=2, ipv6 ok=0, >>> tx pkts=24 >>> + pkts errs=0, ipv4 errs=0, ipv6 errs=0, tx errs=0 >>> +]) >>> + >>> +AT_CHECK([ovs-appctl lsample/show br0], [0], [dnl >>> +local sample statistics for bridge "br0": >>> +- Collector Set ID: 1 >>> + Local Sample Group: 10 >>> + Total number of bytes: 196 >>> + Total number of packets: 2 >>> + >>> +- Collector Set ID: 2 >>> + Local Sample Group: 12 >>> + Total number of bytes: 196 >>> + Total number of packets: 2 >>> +]) >>> + >>> +OVS_TRAFFIC_VSWITCHD_STOP(["/sending to collector failed/d"]) >> >> Why does this warning appear? >> > > Because IPFIX target is localhost and port is closed, so pkt send > returns an error. OK. So, the test without IPFIX should not have that, the test with IPFIX will have it. But, please, add a comment on why we're removing this warning. > >>> +AT_CLEANUP >> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
