One more comment inline regarding to the cleanup part On Fri, Jul 26, 2019 at 9:27 AM Darrell Ball <dlu...@gmail.com> wrote:
> Thanks for the patch > > Comments inline > > On Thu, Jul 25, 2019 at 4:31 PM Yi-Hung Wei <yihung....@gmail.com> wrote: > >> This patch adds a system traffic test to verify the zone-based conntrack >> timeout feature. The test uses ovs-vsctl commands to configure >> the customized ICMP and UDP timeout on zone 5 to a shorter period. >> It then injects ICMP and UDP traffic to conntrack, and checks if the >> corresponding conntrack entry expires after the predefined timeout. >> >> Signed-off-by: Yi-Hung Wei <yihung....@gmail.com> >> --- >> tests/system-kmod-macros.at | 9 ++++++ >> tests/system-traffic.at | 65 >> ++++++++++++++++++++++++++++++++++++++++ >> tests/system-userspace-macros.at | 10 +++++++ >> 3 files changed, 84 insertions(+) >> >> diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at >> index 554a61e9bd95..f3c68277be65 100644 >> --- a/tests/system-kmod-macros.at >> +++ b/tests/system-kmod-macros.at >> @@ -100,6 +100,15 @@ m4_define([CHECK_CONNTRACK_FRAG_OVERLAP], >> # >> m4_define([CHECK_CONNTRACK_NAT]) >> >> +# CHECK_CONNTRACK_TIMEOUT() >> +# >> +# Perform requirements checks for running conntrack customized timeout >> tests. >> +# >> +m4_define([CHECK_CONNTRACK_TIMEOUT], >> +[ >> + AT_SKIP_IF([! cat /boot/config-$(uname -r) | grep >> NF_CONNTRACK_TIMEOUT | grep '=y' > /dev/null]) >> +]) >> + >> # CHECK_CT_DPIF_PER_ZONE_LIMIT() >> # >> # Perform requirements checks for running ovs-dpctl >> ct-[set|get|del]-limits per >> diff --git a/tests/system-traffic.at b/tests/system-traffic.at >> index 1a04199dcfe9..6699466dbf4f 100644 >> --- a/tests/system-traffic.at >> +++ b/tests/system-traffic.at >> @@ -3179,6 +3179,71 @@ NXST_FLOW reply: >> OVS_TRAFFIC_VSWITCHD_STOP >> AT_CLEANUP >> >> +AT_SETUP([conntrack - zone-based timeout policy]) >> +CHECK_CONNTRACK() >> +CHECK_CONNTRACK_TIMEOUT() >> +OVS_TRAFFIC_VSWITCHD_START() >> + >> +ADD_NAMESPACES(at_ns0, at_ns1) >> + >> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") >> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") >> + >> +AT_DATA([flows.txt], [dnl >> +priority=1,action=drop >> +priority=10,arp,action=normal >> +priority=100,in_port=1,ip,action=ct(zone=5, table=1) >> +priority=100,in_port=2,ip,action=ct(zone=5, table=1) >> +table=1,in_port=2,ip,ct_state=+trk+est,action=1 >> +table=1,in_port=1,ip,ct_state=+trk+new,action=ct(commit,zone=5),2 >> +table=1,in_port=1,ip,ct_state=+trk+est,action=2 >> +]) >> + >> +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt]) >> + >> +dnl Add customized timeout >> +dnl Note that the default ICMP timeout is 30 seconds. >> +dnl The default timeout for unreplied UDP is 30 seconds, and >> +dnl 180 seconds for replied UDP connection. >> > > Might be good to confirm that the conntrack entries are not removed > quickly before > reduced timeouts are configured > Then flush the entries > Then configure lower timeouts > The send the packets again > Then confirm they are removed quickly > > >> +AT_CHECK([ovs-vsctl add-dp system]) >> +AT_CHECK([ovs-vsctl add-zone-tp system zone=5 udp_first=3 icmp_first=3]) >> + >> +dnl ICMP traffic >> +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.2 | >> FORMAT_PING], [0], [dnl >> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >> +]) >> + >> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], >> [0],[dnl >> >> +icmp,orig=(src=10.1.1.1,dst=10.1.1.2,id=<cleared>,type=8,code=0),reply=(src=10.1.1.2,dst=10.1.1.1,id=<cleared>,type=0,code=0),zone=5 >> +]) >> + >> +dnl Wait until ICMP timeout expire. >> +dnl We intend to wait a bit longer, because conntrack does not recycle >> the entry right after it is expired. >> +sleep 4 >> + >> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], >> [dnl >> +]) >> + >> +dnl Send out an UDP packet from port 1 >> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 >> packet=50540000000a50540000000908004500001c000000000011a4cd0a0101010a0101020001000200080000 >> actions=resubmit(,0)"]) >> + >> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "dst=10\.1\.1\.2,"], >> [0], [dnl >> >> +udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),reply=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),zone=5 >> +]) >> + >> +dnl Wait until UDP timeout expire >> +sleep 4 >> + >> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], >> [dnl >> +]) >> + >> +AT_CHECK([ovs-ofctl del-flows br0]) >> +AT_CHECK([ovs-appctl revalidator/wait]) >> > > Hopefully, the above 2 lines are NOT required to allow 'del-zone' to work > - can you remove them ? > I will test later today to confirm. > The above cleanup +AT_CHECK([ovs-ofctl del-flows br0]) +AT_CHECK([ovs-appctl revalidator/wait]) is not very realistic although without it, we might end up here: dball@ubuntu:~/openvswitch/ovs$ lsmod | grep nf nfnetlink_cttimeout 16384 1 and then not being able to rerun without intervention. This looks a little strange in a 'system' test, since a 'system' test is supposed to be closer to reality. However, more concerning is the possible non-test environment implications. I guess we need to check. > > This patch logically makes sense as part of Patch 11. > > > >> +AT_CHECK([ovs-vsctl del-zone-tp system zone=5]) >> + >> +OVS_TRAFFIC_VSWITCHD_STOP >> +AT_CLEANUP >> + >> AT_BANNER([conntrack - L7]) >> >> AT_SETUP([conntrack - IPv4 HTTP]) >> diff --git a/tests/system-userspace-macros.at b/tests/ >> system-userspace-macros.at >> index 9d5f3bf419d3..ceabad7499d8 100644 >> --- a/tests/system-userspace-macros.at >> +++ b/tests/system-userspace-macros.at >> @@ -98,6 +98,16 @@ m4_define([CHECK_CONNTRACK_FRAG_OVERLAP]) >> # >> m4_define([CHECK_CONNTRACK_NAT]) >> >> +# CHECK_CONNTRACK_TIMEOUT() >> +# >> +# Perform requirements checks for running conntrack customized timeout >> tests. >> +* The userspace datapath does not support this feature yet. >> +# >> +m4_define([CHECK_CONNTRACK_TIMEOUT], >> +[ >> + AT_SKIP_IF([:]) >> +]) >> + >> # CHECK_CT_DPIF_PER_ZONE_LIMIT() >> # >> # Perform requirements checks for running ovs-dpctl >> ct-[set|get|del]-limits per >> -- >> 2.7.4 >> >> _______________________________________________ >> dev mailing list >> d...@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
