Re: [ovs-dev] [PATCH v2 15/15] system-tests: Run conntrack tests with userspace

[Daniele Di Proietto]

On 19/04/2016 17:24, "Joe Stringer"  wrote:

>On 19 April 2016 at 16:53, Joe Stringer  wrote:
>> On 15 April 2016 at 17:02, Daniele Di Proietto  
>> wrote:
>>> The userspace connection tracker doesn't support ALGs, frag reassembly
>>> or NAT yet, so skip those tests.
>>>
>>> Also, connection tracking state input from a local port is not possible
>>> in userspace.
>>>
>>> Finally, the userspace datapath pads all frames with 0, to make them at
>>> least 64 bytes.
>>>
>>> Signed-off-by: Daniele Di Proietto 
>>
>> Having three prongs to your commit message like this usually indicates
>> that there are three separate patches folded inside ;-)
>>
>> FWIW the LOCAL tests should work whether the local stack provides
>> conntrack state input or not - they just exercise slightly different
>> codepaths in the kernel, once the packet leaves OVS, by going out a
>> different netdev type (with a different destination netns). Probably
>> the class of bugs these tests pick up are not applicable for userspace
>> datapath, so it seems fine to skip the conntrack-related ones. I
>> wouldn't mind seeing a basic sanity test that sends packets between
>> the local stack and another namespace, though - which would run for
>> both testsuites, and both with IPv4 and IPv6 traffic.
>
>I was mistaken here. I looked back over and apparently some of the
>tests do rely on this behaviour, and they're passing on the latest
>net-next kernels. They can be skipped for userspace.
>
>> A few comments on the comments below, but LGTM.
>>
>> Acked-by: Joe Stringer 
>>
>>> ---
>>>  tests/system-kmod-macros.at  | 28 +++
>>>  tests/system-traffic.at  | 49 
>>> ++--
>>>  tests/system-userspace-macros.at | 45 +---
>>>  3 files changed, 107 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at
>>> index 8e60929..4cecc23 100644
>>> --- a/tests/system-kmod-macros.at
>>> +++ b/tests/system-kmod-macros.at
>>> @@ -65,3 +65,31 @@ m4_define([CHECK_CONNTRACK],
>>>   on_exit 'ovstest test-netlink-conntrack flush'
>>>  ]
>>>  )
>>> +
>>> +# CHECK_CONNTRACK_ALG()
>>> +#
>>> +# Perform requirements checks for running conntrack ALG tests. The kernel
>>> +# always supports ALG, so no check is needed.
>>> +#
>>> +m4_define([CHECK_CONNTRACK_ALG])
>>
>> Saying the kernel "always" supports X is a little misleading, as these
>> features can all be turned off in the CONFIG.. of course, all the
>> major distros provide kernels with them turned on so it's not such a
>> big deal. Maybe drop the 'always'.

Good point.  I guess it would be nice to come up with some feature detection.
For the moment I'll drop the 'always'

Thanks for the review!

>>
>>> +# CHECK_CONNTRACK_FRAG()
>>> +#
>>> +# Perform requirements checks for running conntrack fragmentations tests.
>>> +# The kernel always supports fragmentation, so no check is needed.
>>> +m4_define([CHECK_CONNTRACK_FRAG])
>>> +
>>> +# CHECK_CONNTRACK_LOCAL_STACK()
>>> +#
>>> +# Perform requirements checks for running conntrack tests with local stack.
>>> +# The kernel always supports reading the connection state of an skb coming
>>> +# from an internal port, without an explicit ct() action, so no check is
>>> +# needed.
>>> +m4_define([CHECK_CONNTRACK_LOCAL_STACK])
>>
>> While the kernel module supports this, the skb is not guaranteed to
>> have conntrack state when coming from an internal port, depending on
>> the kernel version and whether iptables rules are installed in the
>> host network namespace.
>
>Looks like I got a little mixed up - netfilter hooks are not installed
>in all namespaces by default on the latest kernels, but I'd guess that
>they're still set up in the root netns so it should be fairly reliable
>there (at least for the moment).
>
>> 
>>
>>> +# CHECK_CONNTRACK_LOCAL_STACK()
>>> +#
>>> +# Perform requirements checks for running conntrack tests with local stack.
>>> +# While the kernel connection tracker automatically passes all the 
>>> connection
>>> +# tracking state from an internal port to the OpenvSwitch kernel module, 
>>> there
>>> +# is simply no way of doing that with the userspace, so skip the tests.
>>> +m4_define([CHECK_CONNTRACK_LOCAL_STACK],
>>> +[
>>> +AT_SKIP_IF([:])
>>> +])
>>
>> Same comment here, packets may not have conntrack state associated
>> when arriving on internal port.
>>
>>> +# CHECK_CONNTRACK_NAT()
>>> +#
>>> +# Perform requirements checks for running conntrack NAT tests. The 
>>> userspace
>>> +# doesn't support NATs yet, so skip the tests
>>> +#
>>> +m4_define([CHECK_CONNTRACK_NAT],
>>> +[
>>> +AT_SKIP_IF([:])
>>> +])
>>> --
>>> 2.1.4
>>>
>>> ___
>>> dev mailing list
>>> dev@openvswitch.org
>>> http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2 15/15] system-tests: Run conntrack tests with userspace

[Flavio Leitner]

On Fri, Apr 15, 2016 at 05:02:47PM -0700, Daniele Di Proietto wrote:
> The userspace connection tracker doesn't support ALGs, frag reassembly
> or NAT yet, so skip those tests.
> 
> Also, connection tracking state input from a local port is not possible
> in userspace.
> 
> Finally, the userspace datapath pads all frames with 0, to make them at
> least 64 bytes.
> 
> Signed-off-by: Daniele Di Proietto 
> ---
LGTM


Acked-by: Flavio Leitner 


___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2 15/15] system-tests: Run conntrack tests with userspace

[Joe Stringer]

On 19 April 2016 at 16:53, Joe Stringer  wrote:
> On 15 April 2016 at 17:02, Daniele Di Proietto  wrote:
>> The userspace connection tracker doesn't support ALGs, frag reassembly
>> or NAT yet, so skip those tests.
>>
>> Also, connection tracking state input from a local port is not possible
>> in userspace.
>>
>> Finally, the userspace datapath pads all frames with 0, to make them at
>> least 64 bytes.
>>
>> Signed-off-by: Daniele Di Proietto 
>
> Having three prongs to your commit message like this usually indicates
> that there are three separate patches folded inside ;-)
>
> FWIW the LOCAL tests should work whether the local stack provides
> conntrack state input or not - they just exercise slightly different
> codepaths in the kernel, once the packet leaves OVS, by going out a
> different netdev type (with a different destination netns). Probably
> the class of bugs these tests pick up are not applicable for userspace
> datapath, so it seems fine to skip the conntrack-related ones. I
> wouldn't mind seeing a basic sanity test that sends packets between
> the local stack and another namespace, though - which would run for
> both testsuites, and both with IPv4 and IPv6 traffic.

I was mistaken here. I looked back over and apparently some of the
tests do rely on this behaviour, and they're passing on the latest
net-next kernels. They can be skipped for userspace.

> A few comments on the comments below, but LGTM.
>
> Acked-by: Joe Stringer 
>
>> ---
>>  tests/system-kmod-macros.at  | 28 +++
>>  tests/system-traffic.at  | 49 
>> ++--
>>  tests/system-userspace-macros.at | 45 +---
>>  3 files changed, 107 insertions(+), 15 deletions(-)
>>
>> diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at
>> index 8e60929..4cecc23 100644
>> --- a/tests/system-kmod-macros.at
>> +++ b/tests/system-kmod-macros.at
>> @@ -65,3 +65,31 @@ m4_define([CHECK_CONNTRACK],
>>   on_exit 'ovstest test-netlink-conntrack flush'
>>  ]
>>  )
>> +
>> +# CHECK_CONNTRACK_ALG()
>> +#
>> +# Perform requirements checks for running conntrack ALG tests. The kernel
>> +# always supports ALG, so no check is needed.
>> +#
>> +m4_define([CHECK_CONNTRACK_ALG])
>
> Saying the kernel "always" supports X is a little misleading, as these
> features can all be turned off in the CONFIG.. of course, all the
> major distros provide kernels with them turned on so it's not such a
> big deal. Maybe drop the 'always'.
>
>> +# CHECK_CONNTRACK_FRAG()
>> +#
>> +# Perform requirements checks for running conntrack fragmentations tests.
>> +# The kernel always supports fragmentation, so no check is needed.
>> +m4_define([CHECK_CONNTRACK_FRAG])
>> +
>> +# CHECK_CONNTRACK_LOCAL_STACK()
>> +#
>> +# Perform requirements checks for running conntrack tests with local stack.
>> +# The kernel always supports reading the connection state of an skb coming
>> +# from an internal port, without an explicit ct() action, so no check is
>> +# needed.
>> +m4_define([CHECK_CONNTRACK_LOCAL_STACK])
>
> While the kernel module supports this, the skb is not guaranteed to
> have conntrack state when coming from an internal port, depending on
> the kernel version and whether iptables rules are installed in the
> host network namespace.

Looks like I got a little mixed up - netfilter hooks are not installed
in all namespaces by default on the latest kernels, but I'd guess that
they're still set up in the root netns so it should be fairly reliable
there (at least for the moment).

> 
>
>> +# CHECK_CONNTRACK_LOCAL_STACK()
>> +#
>> +# Perform requirements checks for running conntrack tests with local stack.
>> +# While the kernel connection tracker automatically passes all the 
>> connection
>> +# tracking state from an internal port to the OpenvSwitch kernel module, 
>> there
>> +# is simply no way of doing that with the userspace, so skip the tests.
>> +m4_define([CHECK_CONNTRACK_LOCAL_STACK],
>> +[
>> +AT_SKIP_IF([:])
>> +])
>
> Same comment here, packets may not have conntrack state associated
> when arriving on internal port.
>
>> +# CHECK_CONNTRACK_NAT()
>> +#
>> +# Perform requirements checks for running conntrack NAT tests. The userspace
>> +# doesn't support NATs yet, so skip the tests
>> +#
>> +m4_define([CHECK_CONNTRACK_NAT],
>> +[
>> +AT_SKIP_IF([:])
>> +])
>> --
>> 2.1.4
>>
>> ___
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2 15/15] system-tests: Run conntrack tests with userspace

[Joe Stringer]

On 15 April 2016 at 17:02, Daniele Di Proietto  wrote:
> The userspace connection tracker doesn't support ALGs, frag reassembly
> or NAT yet, so skip those tests.
>
> Also, connection tracking state input from a local port is not possible
> in userspace.
>
> Finally, the userspace datapath pads all frames with 0, to make them at
> least 64 bytes.
>
> Signed-off-by: Daniele Di Proietto 

Having three prongs to your commit message like this usually indicates
that there are three separate patches folded inside ;-)

FWIW the LOCAL tests should work whether the local stack provides
conntrack state input or not - they just exercise slightly different
codepaths in the kernel, once the packet leaves OVS, by going out a
different netdev type (with a different destination netns). Probably
the class of bugs these tests pick up are not applicable for userspace
datapath, so it seems fine to skip the conntrack-related ones. I
wouldn't mind seeing a basic sanity test that sends packets between
the local stack and another namespace, though - which would run for
both testsuites, and both with IPv4 and IPv6 traffic.

A few comments on the comments below, but LGTM.

Acked-by: Joe Stringer 

> ---
>  tests/system-kmod-macros.at  | 28 +++
>  tests/system-traffic.at  | 49 
> ++--
>  tests/system-userspace-macros.at | 45 +---
>  3 files changed, 107 insertions(+), 15 deletions(-)
>
> diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at
> index 8e60929..4cecc23 100644
> --- a/tests/system-kmod-macros.at
> +++ b/tests/system-kmod-macros.at
> @@ -65,3 +65,31 @@ m4_define([CHECK_CONNTRACK],
>   on_exit 'ovstest test-netlink-conntrack flush'
>  ]
>  )
> +
> +# CHECK_CONNTRACK_ALG()
> +#
> +# Perform requirements checks for running conntrack ALG tests. The kernel
> +# always supports ALG, so no check is needed.
> +#
> +m4_define([CHECK_CONNTRACK_ALG])

Saying the kernel "always" supports X is a little misleading, as these
features can all be turned off in the CONFIG.. of course, all the
major distros provide kernels with them turned on so it's not such a
big deal. Maybe drop the 'always'.

> +# CHECK_CONNTRACK_FRAG()
> +#
> +# Perform requirements checks for running conntrack fragmentations tests.
> +# The kernel always supports fragmentation, so no check is needed.
> +m4_define([CHECK_CONNTRACK_FRAG])
> +
> +# CHECK_CONNTRACK_LOCAL_STACK()
> +#
> +# Perform requirements checks for running conntrack tests with local stack.
> +# The kernel always supports reading the connection state of an skb coming
> +# from an internal port, without an explicit ct() action, so no check is
> +# needed.
> +m4_define([CHECK_CONNTRACK_LOCAL_STACK])

While the kernel module supports this, the skb is not guaranteed to
have conntrack state when coming from an internal port, depending on
the kernel version and whether iptables rules are installed in the
host network namespace.



> +# CHECK_CONNTRACK_LOCAL_STACK()
> +#
> +# Perform requirements checks for running conntrack tests with local stack.
> +# While the kernel connection tracker automatically passes all the connection
> +# tracking state from an internal port to the OpenvSwitch kernel module, 
> there
> +# is simply no way of doing that with the userspace, so skip the tests.
> +m4_define([CHECK_CONNTRACK_LOCAL_STACK],
> +[
> +AT_SKIP_IF([:])
> +])

Same comment here, packets may not have conntrack state associated
when arriving on internal port.

> +# CHECK_CONNTRACK_NAT()
> +#
> +# Perform requirements checks for running conntrack NAT tests. The userspace
> +# doesn't support NATs yet, so skip the tests
> +#
> +m4_define([CHECK_CONNTRACK_NAT],
> +[
> +AT_SKIP_IF([:])
> +])
> --
> 2.1.4
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH v2 15/15] system-tests: Run conntrack tests with userspace

[Daniele Di Proietto]

The userspace connection tracker doesn't support ALGs, frag reassembly
or NAT yet, so skip those tests.

Also, connection tracking state input from a local port is not possible
in userspace.

Finally, the userspace datapath pads all frames with 0, to make them at
least 64 bytes.

Signed-off-by: Daniele Di Proietto 
---
 tests/system-kmod-macros.at  | 28 +++
 tests/system-traffic.at  | 49 ++--
 tests/system-userspace-macros.at | 45 +---
 3 files changed, 107 insertions(+), 15 deletions(-)

diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at
index 8e60929..4cecc23 100644
--- a/tests/system-kmod-macros.at
+++ b/tests/system-kmod-macros.at
@@ -65,3 +65,31 @@ m4_define([CHECK_CONNTRACK],
  on_exit 'ovstest test-netlink-conntrack flush'
 ]
 )
+
+# CHECK_CONNTRACK_ALG()
+#
+# Perform requirements checks for running conntrack ALG tests. The kernel
+# always supports ALG, so no check is needed.
+#
+m4_define([CHECK_CONNTRACK_ALG])
+
+# CHECK_CONNTRACK_FRAG()
+#
+# Perform requirements checks for running conntrack fragmentations tests.
+# The kernel always supports fragmentation, so no check is needed.
+m4_define([CHECK_CONNTRACK_FRAG])
+
+# CHECK_CONNTRACK_LOCAL_STACK()
+#
+# Perform requirements checks for running conntrack tests with local stack.
+# The kernel always supports reading the connection state of an skb coming
+# from an internal port, without an explicit ct() action, so no check is
+# needed.
+m4_define([CHECK_CONNTRACK_LOCAL_STACK])
+
+# CHECK_CONNTRACK_NAT()
+#
+# Perform requirements checks for running conntrack NAT tests. The kernel
+# always supports NAT, so no check is needed.
+#
+m4_define([CHECK_CONNTRACK_NAT])
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index c8fbe0d..241175b 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -579,7 +579,8 @@ NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 3 -T 1 
--retry-connrefused -v -o wget0
 dnl (again) HTTP requests from p0->p1 should work fine.
 NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 3 -T 1 --retry-connrefused -v -o 
wget0.log])
 
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl
+dnl The userspace connection tracker here has a different internal TCP state 
(CLOSING). Ignore that.
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2) | grep -v 
"state=CLOSING"], [0], [dnl
 
tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=,dport=),reply=(src=10.1.1.2,dst=10.1.1.1,sport=,dport=),zone=1,protoinfo=(state=SYN_SENT)
 
tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=,dport=),reply=(src=10.1.1.2,dst=10.1.1.1,sport=,dport=),zone=2,protoinfo=(state=TIME_WAIT)
 ])
@@ -589,6 +590,7 @@ AT_CLEANUP
 
 AT_SETUP([conntrack - multiple zones, local])
 CHECK_CONNTRACK()
+CHECK_CONNTRACK_LOCAL_STACK()
 OVS_TRAFFIC_VSWITCHD_START()
 
 ADD_NAMESPACES(at_ns0)
@@ -636,6 +638,7 @@ AT_CLEANUP
 
 AT_SETUP([conntrack - multiple namespaces, internal ports])
 CHECK_CONNTRACK()
+CHECK_CONNTRACK_LOCAL_STACK()
 OVS_TRAFFIC_VSWITCHD_START(
[set-fail-mode br0 secure -- ])
 
@@ -676,6 +679,7 @@ AT_CLEANUP
 
 AT_SETUP([conntrack - multi-stage pipeline, local])
 CHECK_CONNTRACK()
+CHECK_CONNTRACK_LOCAL_STACK()
 OVS_TRAFFIC_VSWITCHD_START()
 
 ADD_NAMESPACES(at_ns0)
@@ -969,11 +973,11 @@ dnl UDP packets from ns0->ns1 should solicit "destination 
unreachable" response.
 NS_CHECK_EXEC([at_ns0], [bash -c "echo a | nc $NC_EOF_OPT -u 10.1.1.2 1"])
 
 AT_CHECK([ovs-appctl revalidator/purge], [0])
-AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort | grep -v drop], [0], 
[dnl
- n_packets=1, n_bytes=44, priority=100,udp,in_port=1 
actions=ct(commit,exec(load:0x1->NXM_NX_CT_MARK[[]])),output:2
- n_packets=1, n_bytes=72, 
priority=100,ct_state=+rel+trk,ct_mark=0x1,icmp,in_port=2 actions=output:1
- n_packets=1, n_bytes=72, priority=100,ct_state=-trk,icmp,in_port=2 
actions=ct(table=0)
- n_packets=2, n_bytes=84, priority=10,arp actions=NORMAL
+AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort | grep -v drop | sed 
-e 's/n_bytes=[[0-9]]*/n_bytes=/g'], [0], [dnl
+ n_packets=1, n_bytes=, priority=100,udp,in_port=1 
actions=ct(commit,exec(load:0x1->NXM_NX_CT_MARK[[]])),output:2
+ n_packets=1, n_bytes=, 
priority=100,ct_state=+rel+trk,ct_mark=0x1,icmp,in_port=2 actions=output:1
+ n_packets=1, n_bytes=, priority=100,ct_state=-trk,icmp,in_port=2 
actions=ct(table=0)
+ n_packets=2, n_bytes=, priority=10,arp actions=NORMAL
 NXST_FLOW reply:
 ])
 
@@ -1027,6 +1031,7 @@ AT_CLEANUP
 AT_SETUP([conntrack - FTP])
 AT_SKIP_IF([test $HAVE_PYFTPDLIB = no])
 CHECK_CONNTRACK()
+CHECK_CONNTRACK_ALG()
 OVS_TRAFFIC_VSWITCHD_START()
 
 ADD_NAMESPACES(at_ns0, at_ns1)
@@ -1109,6 +1114,7 @@ AT_CLEANUP
 AT_SETUP([conntrack - IPv6 FTP])
 AT_SKIP_IF([test $HAVE_PYFTPDLIB = no])
 CHECK_CONNTRACK()
+CHECK_CONNTRACK_ALG()
 OVS_TRAFFIC_VSWITCHD_START()
 
 ADD_NAMESPACES(at_ns0, at_ns1)
@@ -1159,6 +1165,7 @@