Conditional to Ben’s approval of the nested actions within the OF ct action, of
course :-)
Jarno
> On Sep 11, 2015, at 4:37 PM, Jarno Rajahalme <jrajaha...@nicira.com> wrote:
>
> 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
