| Message ID | 20230504065921.28651-1-amusil@redhat.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | [ovs-dev] system-tests: Try to load modules only if they weren't loaded before | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On 5/4/23 08:59, Ales Musil wrote: > When the system is busy with multiple test running concurrently > the modprobe might fail with "Device or resource busy", which > will fail the whole test. Instead of always trying to load the module > check if it's already loaded. Also replace the AT_CHECK with OVS_WAIT_UNTIL > which should reduce the chance of failing on initial contention. Hi, Ales. I'm not sure if this change will not introduce more problems than it's trying to solve. The module reloading is there for a reason. It's the only way to clean up some of the in-kernel resources and start a new test from a clean slate. Without reloading we may get all kinds of unpredictable side effects carried over from the previous tests. Some of the internal in-kernel resources are not even isolated to network namespaces. Especially on older kernels. So, I would advise against running them in parallel, even if you can run each test in a separate namespace somehow (which is not how it is done today). Best regards, Ilya Maximets. > > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > tests/system-common-macros.at | 13 ------------ > tests/system-kmod-macros.at | 39 +++++++++++++++++++++++++++-------- > 2 files changed, 30 insertions(+), 22 deletions(-) > > diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at > index e6f204cc1..c6ba7fb4f 100644 > --- a/tests/system-common-macros.at > +++ b/tests/system-common-macros.at > @@ -330,19 +330,6 @@ m4_define([OVS_CHECK_CT_CLEAR], > m4_define([OVS_CHECK_CT_ZERO_SNAT], > [AT_SKIP_IF([! grep -q "Datapath supports ct_zero_snat" ovs-vswitchd.log])]) > > -# LOAD_MODULE([name]) > -# > -# Tries to load specified kernel module and removes it after > -# if it wasn't loaded before this call. > -# > -m4_define([LOAD_MODULE], > - [if ! lsmod | grep -q $1; then > - on_exit 'modprobe -q -r $1' > - fi > - AT_CHECK([modprobe $1]) > - ] > -) > - > # OVN_TEST_IPV6_PREFIX_DELEGATION() > m4_define([OVN_TEST_IPV6_PREFIX_DELEGATION], > [ > diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at > index 787a59c97..df1b2147f 100644 > --- a/tests/system-kmod-macros.at > +++ b/tests/system-kmod-macros.at > @@ -1,3 +1,30 @@ > +# LOAD_MODULE([name]) > +# > +# Tries to load specified kernel module and removes it after > +# if it wasn't loaded before this call. > +# > +m4_define([LOAD_MODULE], > + [if ! lsmod | grep -q $1; then > + OVS_WAIT_UNTIL([modprobe $1]) > + on_exit 'modprobe -q -r $1' > + fi > + ] > +) > + > +# TRY_LOAD_MODULE([name]) > +# > +# Tries to load specified kernel module, it won't fail > +# if modprobe didn't succeed, and removes it after > +# if it wasn't loaded before this call. > +# > +m4_define([TRY_LOAD_MODULE], > + [if ! lsmod | grep -q $1; then > + modprobe $1 || echo "Module $1 not loaded." > + on_exit 'modprobe -q -r $1' > + fi > + ] > +) > + > # _ADD_BR([name]) > # > # Expands into the proper ovs-vsctl commands to create a bridge with the > @@ -16,12 +43,9 @@ m4_define([_ADD_BR], [[add-br $1 -- set Bridge $1 protocols=OpenFlow10,OpenFlow1 > # Best-effort loading of all available vport modules is performed. > # > m4_define([OVS_TRAFFIC_VSWITCHD_START], > - [AT_CHECK([modprobe openvswitch]) > - on_exit 'modprobe -r openvswitch' > + [LOAD_MODULE([openvswitch]) > m4_foreach([mod], [[vport_geneve], [vport_gre], [vport_lisp], [vport_stt], [vport_vxlan]], > - [modprobe -q mod || echo "Module mod not loaded." > - on_exit 'modprobe -q -r mod' > - ]) > + [TRY_LOAD_MODULE([mod])]) > on_exit 'ovs-dpctl del-dp ovs-system' > on_exit 'ovs-appctl dpctl/flush-conntrack' > _OVS_VSWITCHD_START([]) > @@ -60,10 +84,7 @@ m4_define([CONFIGURE_VETH_OFFLOADS], > # > m4_define([CHECK_CONNTRACK], > m4_foreach([mod], [[nf_conntrack_ipv4], [nf_conntrack_ipv6], [nf_nat_ftp], > - [nf_nat_tftp]], > - [modprobe mod || echo "Module mod not loaded." > - on_exit 'modprobe -r mod' > - ]) > + [nf_nat_tftp]], [TRY_LOAD_MODULE([mod])]) > sysctl -w net.netfilter.nf_conntrack_helper=0 > on_exit 'ovstest test-netlink-conntrack flush' > )
diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at index e6f204cc1..c6ba7fb4f 100644 --- a/tests/system-common-macros.at +++ b/tests/system-common-macros.at @@ -330,19 +330,6 @@ m4_define([OVS_CHECK_CT_CLEAR], m4_define([OVS_CHECK_CT_ZERO_SNAT], [AT_SKIP_IF([! grep -q "Datapath supports ct_zero_snat" ovs-vswitchd.log])]) -# LOAD_MODULE([name]) -# -# Tries to load specified kernel module and removes it after -# if it wasn't loaded before this call. -# -m4_define([LOAD_MODULE], - [if ! lsmod | grep -q $1; then - on_exit 'modprobe -q -r $1' - fi - AT_CHECK([modprobe $1]) - ] -) - # OVN_TEST_IPV6_PREFIX_DELEGATION() m4_define([OVN_TEST_IPV6_PREFIX_DELEGATION], [ diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at index 787a59c97..df1b2147f 100644 --- a/tests/system-kmod-macros.at +++ b/tests/system-kmod-macros.at @@ -1,3 +1,30 @@ +# LOAD_MODULE([name]) +# +# Tries to load specified kernel module and removes it after +# if it wasn't loaded before this call. +# +m4_define([LOAD_MODULE], + [if ! lsmod | grep -q $1; then + OVS_WAIT_UNTIL([modprobe $1]) + on_exit 'modprobe -q -r $1' + fi + ] +) + +# TRY_LOAD_MODULE([name]) +# +# Tries to load specified kernel module, it won't fail +# if modprobe didn't succeed, and removes it after +# if it wasn't loaded before this call. +# +m4_define([TRY_LOAD_MODULE], + [if ! lsmod | grep -q $1; then + modprobe $1 || echo "Module $1 not loaded." + on_exit 'modprobe -q -r $1' + fi + ] +) + # _ADD_BR([name]) # # Expands into the proper ovs-vsctl commands to create a bridge with the @@ -16,12 +43,9 @@ m4_define([_ADD_BR], [[add-br $1 -- set Bridge $1 protocols=OpenFlow10,OpenFlow1 # Best-effort loading of all available vport modules is performed. # m4_define([OVS_TRAFFIC_VSWITCHD_START], - [AT_CHECK([modprobe openvswitch]) - on_exit 'modprobe -r openvswitch' + [LOAD_MODULE([openvswitch]) m4_foreach([mod], [[vport_geneve], [vport_gre], [vport_lisp], [vport_stt], [vport_vxlan]], - [modprobe -q mod || echo "Module mod not loaded." - on_exit 'modprobe -q -r mod' - ]) + [TRY_LOAD_MODULE([mod])]) on_exit 'ovs-dpctl del-dp ovs-system' on_exit 'ovs-appctl dpctl/flush-conntrack' _OVS_VSWITCHD_START([]) @@ -60,10 +84,7 @@ m4_define([CONFIGURE_VETH_OFFLOADS], # m4_define([CHECK_CONNTRACK], m4_foreach([mod], [[nf_conntrack_ipv4], [nf_conntrack_ipv6], [nf_nat_ftp], - [nf_nat_tftp]], - [modprobe mod || echo "Module mod not loaded." - on_exit 'modprobe -r mod' - ]) + [nf_nat_tftp]], [TRY_LOAD_MODULE([mod])]) sysctl -w net.netfilter.nf_conntrack_helper=0 on_exit 'ovstest test-netlink-conntrack flush' )
When the system is busy with multiple test running concurrently the modprobe might fail with "Device or resource busy", which will fail the whole test. Instead of always trying to load the module check if it's already loaded. Also replace the AT_CHECK with OVS_WAIT_UNTIL which should reduce the chance of failing on initial contention. Signed-off-by: Ales Musil <amusil@redhat.com> --- tests/system-common-macros.at | 13 ------------ tests/system-kmod-macros.at | 39 +++++++++++++++++++++++++++-------- 2 files changed, 30 insertions(+), 22 deletions(-)