| Message ID | 20230524133953.2363437-1-frode.nordahl@canonical.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] tests: layer3-tunnels: Skip bareudp tests if not supported by kernel. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Wed, May 24, 2023 at 03:39:53PM +0200, Frode Nordahl wrote: > The bareudp tests depend on specific kernel configuration to > succeed. Skip the test if the feature is not enabled in the > running kernel. > > Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com> Tested-by: Simon Horman <simon.horman@corigine.com> Reviewed-by: Simon Horman <simon.horman@corigine.com>
On 5/24/23 15:39, Frode Nordahl wrote: > The bareudp tests depend on specific kernel configuration to > succeed. Skip the test if the feature is not enabled in the > running kernel. > > Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com> > --- > tests/system-kmod-macros.at | 10 ++++++++++ > tests/system-layer3-tunnels.at | 2 ++ > tests/system-userspace-macros.at | 8 ++++++++ > 3 files changed, 20 insertions(+) > > diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at > index fb15a5a7c..55e7821ce 100644 > --- a/tests/system-kmod-macros.at > +++ b/tests/system-kmod-macros.at > @@ -237,3 +237,13 @@ m4_define([CHECK_L3L4_CONNTRACK_REASM]) > # > # The kernel module tests do not use TC offload. > m4_define([CHECK_NO_TC_OFFLOAD]) > + > +# OVS_CHECK_BAREUDP() > +# > +# The feature needs to be enabled in the kernel configuration (CONFIG_BAREUDP) > +# to work. > +m4_define([OVS_CHECK_BAREUDP], > +[ > + AT_SKIP_IF([! ip link add dev bareudp0 type bareudp dstport 6635 ethertype mpls_uc 2>&1 >/dev/null]) > + AT_CHECK([ip link del dev bareudp0]) Maybe call it ovs_bareudp0 or something like that? Just to decrease chances for random collisions. > +]) > diff --git a/tests/system-layer3-tunnels.at b/tests/system-layer3-tunnels.at > index c37852b21..5546bc879 100644 > --- a/tests/system-layer3-tunnels.at > +++ b/tests/system-layer3-tunnels.at > @@ -155,6 +155,7 @@ AT_CLEANUP > > AT_SETUP([layer3 - ping over MPLS Bareudp]) > OVS_CHECK_MIN_KERNEL(5, 7) > +OVS_CHECK_BAREUDP() This new check supersedes the kernel version check. Should we remove it? The original idea was that we can create bareudp tunnels even if the ip utility didn't support them at a time. But, I suppose, enough time passed since then and the ip utility available in distributions caught up, so we can just check it instead. > OVS_TRAFFIC_VSWITCHD_START([_ADD_BR([br1])]) > ADD_NAMESPACES(at_ns0, at_ns1) > > @@ -203,6 +204,7 @@ AT_CLEANUP > > AT_SETUP([layer3 - ping over Bareudp]) > OVS_CHECK_MIN_KERNEL(5, 7) Same here. > +OVS_CHECK_BAREUDP() > OVS_TRAFFIC_VSWITCHD_START([_ADD_BR([br1])]) > ADD_NAMESPACES(at_ns0, at_ns1) > > diff --git a/tests/system-userspace-macros.at b/tests/system-userspace-macros.at > index 482079386..1cb67d6f6 100644 > --- a/tests/system-userspace-macros.at > +++ b/tests/system-userspace-macros.at > @@ -336,3 +336,11 @@ m4_define([CHECK_L3L4_CONNTRACK_REASM], > # > # Userspace tests do not use TC offload. > m4_define([CHECK_NO_TC_OFFLOAD]) > + > +# OVS_CHECK_BAREUDP() > +# > +# The userspace skips all tests that check kernel configuration. This should probably say that userspace datapath doesn't support bareudp tunnels instead. > +m4_define([OVS_CHECK_BAREUDP], > +[ > + AT_SKIP_IF([:]) > +])
On Wed, May 24, 2023 at 09:23:59PM +0200, Ilya Maximets wrote: > On 5/24/23 15:39, Frode Nordahl wrote: > > The bareudp tests depend on specific kernel configuration to > > succeed. Skip the test if the feature is not enabled in the > > running kernel. > > > > Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com> > > --- > > tests/system-kmod-macros.at | 10 ++++++++++ > > tests/system-layer3-tunnels.at | 2 ++ > > tests/system-userspace-macros.at | 8 ++++++++ > > 3 files changed, 20 insertions(+) > > > > diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at > > index fb15a5a7c..55e7821ce 100644 > > --- a/tests/system-kmod-macros.at > > +++ b/tests/system-kmod-macros.at > > @@ -237,3 +237,13 @@ m4_define([CHECK_L3L4_CONNTRACK_REASM]) > > # > > # The kernel module tests do not use TC offload. > > m4_define([CHECK_NO_TC_OFFLOAD]) > > + > > +# OVS_CHECK_BAREUDP() > > +# > > +# The feature needs to be enabled in the kernel configuration (CONFIG_BAREUDP) > > +# to work. > > +m4_define([OVS_CHECK_BAREUDP], > > +[ > > + AT_SKIP_IF([! ip link add dev bareudp0 type bareudp dstport 6635 ethertype mpls_uc 2>&1 >/dev/null]) > > + AT_CHECK([ip link del dev bareudp0]) > > Maybe call it ovs_bareudp0 or something like that? Just to decrease chances > for random collisions. > > > +]) > > diff --git a/tests/system-layer3-tunnels.at b/tests/system-layer3-tunnels.at > > index c37852b21..5546bc879 100644 > > --- a/tests/system-layer3-tunnels.at > > +++ b/tests/system-layer3-tunnels.at > > @@ -155,6 +155,7 @@ AT_CLEANUP > > > > AT_SETUP([layer3 - ping over MPLS Bareudp]) > > OVS_CHECK_MIN_KERNEL(5, 7) > > +OVS_CHECK_BAREUDP() > > This new check supersedes the kernel version check. Should we remove it? > The original idea was that we can create bareudp tunnels even if the ip utility > didn't support them at a time. But, I suppose, enough time passed since then > and the ip utility available in distributions caught up, so we can just check > it instead. I'd say that kernel checks are unreliable in the presence of backports. Whereas tool checks suffer the problem you describe. At this point I'd lean towards a tool-based check. As you say, some time has passed by now. FWIIW, I believe we've always relied on a tool based check for PPS rate limiting checks, which are similar. Perhaps unwisely. But in practice the problems I'm aware of there is with the implementation of the check (hopefully fixed by now) not the tool vs kernel issue. > > OVS_TRAFFIC_VSWITCHD_START([_ADD_BR([br1])]) > > ADD_NAMESPACES(at_ns0, at_ns1) > > > > @@ -203,6 +204,7 @@ AT_CLEANUP > > > > AT_SETUP([layer3 - ping over Bareudp]) > > OVS_CHECK_MIN_KERNEL(5, 7) > > Same here. > > > +OVS_CHECK_BAREUDP() > > OVS_TRAFFIC_VSWITCHD_START([_ADD_BR([br1])]) > > ADD_NAMESPACES(at_ns0, at_ns1) > > > > diff --git a/tests/system-userspace-macros.at b/tests/system-userspace-macros.at > > index 482079386..1cb67d6f6 100644 > > --- a/tests/system-userspace-macros.at > > +++ b/tests/system-userspace-macros.at > > @@ -336,3 +336,11 @@ m4_define([CHECK_L3L4_CONNTRACK_REASM], > > # > > # Userspace tests do not use TC offload. > > m4_define([CHECK_NO_TC_OFFLOAD]) > > + > > +# OVS_CHECK_BAREUDP() > > +# > > +# The userspace skips all tests that check kernel configuration. > > This should probably say that userspace datapath doesn't support bareudp tunnels > instead. > > > +m4_define([OVS_CHECK_BAREUDP], > > +[ > > + AT_SKIP_IF([:]) > > +]) >
On 5/25/23 09:19, Simon Horman wrote: > On Wed, May 24, 2023 at 09:23:59PM +0200, Ilya Maximets wrote: >> On 5/24/23 15:39, Frode Nordahl wrote: >>> The bareudp tests depend on specific kernel configuration to >>> succeed. Skip the test if the feature is not enabled in the >>> running kernel. >>> >>> Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com> >>> --- >>> tests/system-kmod-macros.at | 10 ++++++++++ >>> tests/system-layer3-tunnels.at | 2 ++ >>> tests/system-userspace-macros.at | 8 ++++++++ >>> 3 files changed, 20 insertions(+) >>> >>> diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at >>> index fb15a5a7c..55e7821ce 100644 >>> --- a/tests/system-kmod-macros.at >>> +++ b/tests/system-kmod-macros.at >>> @@ -237,3 +237,13 @@ m4_define([CHECK_L3L4_CONNTRACK_REASM]) >>> # >>> # The kernel module tests do not use TC offload. >>> m4_define([CHECK_NO_TC_OFFLOAD]) >>> + >>> +# OVS_CHECK_BAREUDP() >>> +# >>> +# The feature needs to be enabled in the kernel configuration (CONFIG_BAREUDP) >>> +# to work. >>> +m4_define([OVS_CHECK_BAREUDP], >>> +[ >>> + AT_SKIP_IF([! ip link add dev bareudp0 type bareudp dstport 6635 ethertype mpls_uc 2>&1 >/dev/null]) >>> + AT_CHECK([ip link del dev bareudp0]) >> >> Maybe call it ovs_bareudp0 or something like that? Just to decrease chances >> for random collisions. >> >>> +]) >>> diff --git a/tests/system-layer3-tunnels.at b/tests/system-layer3-tunnels.at >>> index c37852b21..5546bc879 100644 >>> --- a/tests/system-layer3-tunnels.at >>> +++ b/tests/system-layer3-tunnels.at >>> @@ -155,6 +155,7 @@ AT_CLEANUP >>> >>> AT_SETUP([layer3 - ping over MPLS Bareudp]) >>> OVS_CHECK_MIN_KERNEL(5, 7) >>> +OVS_CHECK_BAREUDP() >> >> This new check supersedes the kernel version check. Should we remove it? >> The original idea was that we can create bareudp tunnels even if the ip utility >> didn't support them at a time. But, I suppose, enough time passed since then >> and the ip utility available in distributions caught up, so we can just check >> it instead. > > I'd say that kernel checks are unreliable in the presence of backports. > Whereas tool checks suffer the problem you describe. > At this point I'd lean towards a tool-based check. > As you say, some time has passed by now. Yes, I agree. We may remove the kernel version check though as it is not necessary in the presence of a tool-based check. > > FWIIW, I believe we've always relied on a tool based check for PPS rate > limiting checks, which are similar. Perhaps unwisely. But in practice the > problems I'm aware of there is with the implementation of the check > (hopefully fixed by now) not the tool vs kernel issue. > >>> OVS_TRAFFIC_VSWITCHD_START([_ADD_BR([br1])]) >>> ADD_NAMESPACES(at_ns0, at_ns1) >>> >>> @@ -203,6 +204,7 @@ AT_CLEANUP >>> >>> AT_SETUP([layer3 - ping over Bareudp]) >>> OVS_CHECK_MIN_KERNEL(5, 7) >> >> Same here. >> >>> +OVS_CHECK_BAREUDP() >>> OVS_TRAFFIC_VSWITCHD_START([_ADD_BR([br1])]) >>> ADD_NAMESPACES(at_ns0, at_ns1) >>> >>> diff --git a/tests/system-userspace-macros.at b/tests/system-userspace-macros.at >>> index 482079386..1cb67d6f6 100644 >>> --- a/tests/system-userspace-macros.at >>> +++ b/tests/system-userspace-macros.at >>> @@ -336,3 +336,11 @@ m4_define([CHECK_L3L4_CONNTRACK_REASM], >>> # >>> # Userspace tests do not use TC offload. >>> m4_define([CHECK_NO_TC_OFFLOAD]) >>> + >>> +# OVS_CHECK_BAREUDP() >>> +# >>> +# The userspace skips all tests that check kernel configuration. >> >> This should probably say that userspace datapath doesn't support bareudp tunnels >> instead. >> >>> +m4_define([OVS_CHECK_BAREUDP], >>> +[ >>> + AT_SKIP_IF([:]) >>> +]) >>
diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at index fb15a5a7c..55e7821ce 100644 --- a/tests/system-kmod-macros.at +++ b/tests/system-kmod-macros.at @@ -237,3 +237,13 @@ m4_define([CHECK_L3L4_CONNTRACK_REASM]) # # The kernel module tests do not use TC offload. m4_define([CHECK_NO_TC_OFFLOAD]) + +# OVS_CHECK_BAREUDP() +# +# The feature needs to be enabled in the kernel configuration (CONFIG_BAREUDP) +# to work. +m4_define([OVS_CHECK_BAREUDP], +[ + AT_SKIP_IF([! ip link add dev bareudp0 type bareudp dstport 6635 ethertype mpls_uc 2>&1 >/dev/null]) + AT_CHECK([ip link del dev bareudp0]) +]) diff --git a/tests/system-layer3-tunnels.at b/tests/system-layer3-tunnels.at index c37852b21..5546bc879 100644 --- a/tests/system-layer3-tunnels.at +++ b/tests/system-layer3-tunnels.at @@ -155,6 +155,7 @@ AT_CLEANUP AT_SETUP([layer3 - ping over MPLS Bareudp]) OVS_CHECK_MIN_KERNEL(5, 7) +OVS_CHECK_BAREUDP() OVS_TRAFFIC_VSWITCHD_START([_ADD_BR([br1])]) ADD_NAMESPACES(at_ns0, at_ns1) @@ -203,6 +204,7 @@ AT_CLEANUP AT_SETUP([layer3 - ping over Bareudp]) OVS_CHECK_MIN_KERNEL(5, 7) +OVS_CHECK_BAREUDP() OVS_TRAFFIC_VSWITCHD_START([_ADD_BR([br1])]) ADD_NAMESPACES(at_ns0, at_ns1) diff --git a/tests/system-userspace-macros.at b/tests/system-userspace-macros.at index 482079386..1cb67d6f6 100644 --- a/tests/system-userspace-macros.at +++ b/tests/system-userspace-macros.at @@ -336,3 +336,11 @@ m4_define([CHECK_L3L4_CONNTRACK_REASM], # # Userspace tests do not use TC offload. m4_define([CHECK_NO_TC_OFFLOAD]) + +# OVS_CHECK_BAREUDP() +# +# The userspace skips all tests that check kernel configuration. +m4_define([OVS_CHECK_BAREUDP], +[ + AT_SKIP_IF([:]) +])
The bareudp tests depend on specific kernel configuration to succeed. Skip the test if the feature is not enabled in the running kernel. Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com> --- tests/system-kmod-macros.at | 10 ++++++++++ tests/system-layer3-tunnels.at | 2 ++ tests/system-userspace-macros.at | 8 ++++++++ 3 files changed, 20 insertions(+)