Here is an provisional ACK, I trust you to address the comments to my satisfaction :-)
Acked-by: Jarno Rajahalme <jrajaha...@nicira.com> Jarno > On Sep 11, 2015, at 4:15 PM, Joe Stringer <joestrin...@nicira.com> wrote: > > On 11 September 2015 at 14:42, Jarno Rajahalme <jrajaha...@nicira.com> wrote: >>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at >>> index 7dbed68..de6b016 100644 >>> --- a/tests/system-traffic.at >>> +++ b/tests/system-traffic.at >>> @@ -139,3 +139,472 @@ NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 >>> -w 2 10.1.1.100 | FORMAT_PI >>> >>> OVS_TRAFFIC_VSWITCHD_STOP >>> AT_CLEANUP >>> + >>> +AT_SETUP([conntrack - controller]) >>> +CHECK_CONNTRACK() >>> +OVS_TRAFFIC_VSWITCHD_START( >>> + [set-fail-mode br0 standalone -- ]) >>> + >>> +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") >>> + >>> +dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from >>> ns1->ns0. >>> +AT_DATA([flows.txt], [dnl >>> +priority=1,action=drop >>> +priority=10,arp,action=normal >>> +in_port=1,udp,action=ct(commit),controller >>> +in_port=2,ct_state=-trk,udp,action=ct(table=0) >>> +in_port=2,ct_state=+trk+est-new,udp,action=controller >> >> ct_state=+est should be sufficient here? > > True, +est-new is a little redundant. > >>> +dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from >>> ns1->ns0. >>> +AT_DATA([flows.txt], [dnl >>> +priority=1,action=drop >>> +priority=10,arp,action=normal >>> +priority=10,icmp,action=normal >>> +in_port=1,tcp,action=ct(commit),2 >>> +in_port=2,ct_state=-trk,tcp,action=ct(table=0) >>> +in_port=2,ct_state=+trk+est-new,tcp,action=1 >> >> Having explicit priorities here as well would be helpful. The comment above >> about “+est” should apply here as well. > > I can update any/all tests that have priorities to either all provide > specific priorities, or use no priorities. > >>> +]) >>> + >>> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) >>> + >>> +dnl Basic connectivity check. >>> +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.2 >/dev/null]) >>> + >>> +dnl HTTP requests from ns0->ns1 should work fine. >>> +NETNS_DAEMONIZE([at_ns1], [[$PYTHON $srcdir/test-l7.py]], [http0.pid]) >>> +NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 3 -T 1 --retry-connrefused -v -o >>> wget0.log]) >> >> >> wget was trying to resolve the proxy I had configured in the “http_proxy” >> environment variable. The resolution from the netns failed, making the test >> case fail. I had to clear “http_proxy" to make the test work. > > Hmm, I don't have a good solution for this. Personally I always push > code to a remote test VM that I use specifically for the purpose of > running these tests, so any configuration I might have in my local > desktop environment wouldn't cause issues such as this. (This is > equivalent to how the Vagrant tests run) Maybe a comment in some doc describing how to run check-kernel? > >>> + >>> +AT_CHECK([conntrack -L 2>&1 | FORMAT_CT(10.1.1.2)], [0], [dnl >>> +TIME_WAIT src=10.1.1.1 dst=10.1.1.2 sport=<cleared> dport=<cleared> >>> src=10.1.1.2 dst=10.1.1.1 sport=<cleared> dport=<cleared> [[ASSURED]] >>> mark=0 use=1 >> >> Do you know how/if [ASSURED] maps to ct_state flags? > > I can look into it. My understanding is that it's equivalent to > "established", although it may be more aligned with our concept of > "committed". > >>> +AT_SETUP([conntrack - commit, recirc]) >>> +CHECK_CONNTRACK() >>> +OVS_TRAFFIC_VSWITCHD_START( >>> + [set-fail-mode br0 standalone -- ]) >>> + >>> +ADD_NAMESPACES(at_ns0, at_ns1, at_ns2, at_ns3) >>> + >>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") >>> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") >>> +ADD_VETH(p2, at_ns2, br0, "10.1.1.3/24") >>> +ADD_VETH(p3, at_ns3, br0, "10.1.1.4/24") >>> + >>> +dnl Allow any traffic from ns0->ns1, ns2->ns3. >>> +AT_DATA([flows.txt], [dnl >>> +priority=1,action=drop >>> +priority=10,arp,action=normal >>> +priority=10,icmp,action=normal >>> +in_port=1,tcp,ct_state=-trk,action=ct(commit,table=0) >> >> Maybe some of the test cases should use non-zero table? > > Agreed, there's several directions that we can increase the coverage > in the testsuite. I'll add this to the list. > >>> +ADD_NAMESPACES(at_ns0, at_ns1, at_ns2, at_ns3) >>> + >>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") >>> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") >>> +ADD_VETH(p2, at_ns2, br0, "10.1.1.3/24") >>> +ADD_VETH(p3, at_ns3, br0, "10.1.1.4/24") >>> + >>> +dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from >>> ns1->ns0. >> >> Update comment? > > For ns2/ns3 as well? Sure. > >>> +AT_DATA([flows.txt], [dnl >>> +priority=1,action=drop >>> +priority=10,arp,action=normal >>> +priority=10,icmp,action=normal >>> +in_port=1,tcp,action=ct(),2 >>> +in_port=2,ct_state=-trk,tcp,action=ct(table=0) >>> +in_port=2,ct_state=+trk+new,tcp,action=1 >> >> Here “+new” should be sufficient, in general, “+trk” is not needed if any of >> the other bits are matched for being set, right? > > For several of these I have made the rule more explicit than strictly > necessary, so that if there is an unexpected combination of bits, then > those flows will hit the "drop" rule rather than silently matching the > accept rule. > >>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") >>> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") >>> +ADD_VETH(p2, at_ns2, br0, "10.1.1.3/24") >>> +ADD_VETH(p3, at_ns3, br0, "10.1.1.4/24") >>> + >>> +dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from >>> ns1->ns0. >> >> Does this comment need an update? > > Yes, I'll update it. I can see the fallacy of copy-paste test crafting > here: The comments are initially added to improve readability, but > when they're copied several times it's easy to miss the fact that > they're out of date. It wouldn't be a problem if there were no > comments, but then it would be more difficult to grok what's going on. > >>> +dnl Allow any traffic from ns0->ns1, allow established in reverse. >>> +AT_DATA([flows-br0.txt], [dnl >>> +priority=1,action=drop >>> +priority=10,arp,action=normal >>> +priority=10,icmp,action=normal >>> +in_port=2,tcp,ct_state=-trk,action=ct(commit,zone=1),1 >>> +in_port=1,tcp,ct_state=-trk,action=ct(table=0,zone=1) >>> +in_port=1,tcp,ct_state=+trk+est,ct_zone=1,action=2 >>> +]) >>> + >>> +dnl Allow any traffic from ns0->ns1, allow established in reverse. >> >> Should this comment be different from the one above?? > > I think it's saying the same thing, it just wasn't copy/pasted from > the earlier one. I'll review each of these comments. > >>> +AT_SETUP([conntrack - ICMP related 2]) >>> +CHECK_CONNTRACK() >>> +OVS_TRAFFIC_VSWITCHD_START( >>> + [set-fail-mode br0 standalone -- ]) >>> + >>> +ADD_NAMESPACES(at_ns0, at_ns1) >>> + >>> +ADD_VETH(p0, at_ns0, br0, "172.16.0.1/24") >>> +ADD_VETH(p1, at_ns1, br0, "172.16.0.2/24") >>> + >>> +dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from >>> ns1->ns0. >>> +AT_DATA([flows.txt], [dnl >>> +priority=1,action=drop >>> +priority=10,arp,action=normal >>> +in_port=1,ct_state=-trk,udp,action=ct(commit,table=0) >>> +in_port=1,ct_state=+trk,actions=controller >>> +in_port=2,ct_state=-trk,action=ct(table=0) >>> +in_port=2,ct_state=+trk-inv-new,action=controller >> >> Could there be a match on +rel here? > > Yes, we could expand the flows to be more explicit about the exact > packets seen (looks like there's a "new" and a "related, reply" > packets) > > Thanks for the thorough review, > >>> diff --git a/tests/test-odp.c b/tests/test-odp.c >>> index 3e7939e..cb245d6 100644 >>> --- a/tests/test-odp.c >>> +++ b/tests/test-odp.c >>> @@ -57,7 +57,10 @@ parse_keys(bool wc_keys) >>> struct odp_flow_key_parms odp_parms = { >>> .flow = &flow, >>> .support = { >>> + .max_mpls_depth = SIZE_MAX, >> >> Why this? > > I think this was intended to be in a separate patch. From memory, if > any of the .support fields are false, then we don't test ODP parsing > of that feature. > > Cheers, > Joe _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev