On 19/04/2016 17:24, "Joe Stringer" <j...@ovn.org> wrote:
>On 19 April 2016 at 16:53, Joe Stringer <j...@ovn.org> wrote:
>> On 15 April 2016 at 17:02, Daniele Di Proietto <diproiet...@vmware.com>
>> 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 <diproiet...@vmware.com>
>>
>> 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 <j...@ovn.org>
>>
>>> ---
>>> 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).
>
>> <snip>
>>
>>> +# 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
