On Mon, Jun 24, 2024 at 04:15:50PM GMT, Ilya Maximets wrote: > 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. >
Ack. > > > >> 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. > If we keep the cookie in host order, "ovs-appctl dump/flows" output will be different depending on the host the test is run, that's why I chose a symmetric value. If the host order is what is typically used in netlink, my original thinking prevails. In that case I'll use a symmetric value to check the datapath flow and a non-symmetric one to test the output of "ovstest psample". > > > >>> +]) > >>> + > >>> +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
