On 5/31/22 11:33, Mohammad Heib wrote:
> currently ovn ci only has one test case with the option
> ovn-monitor-all enabled, this patch will add one more
> execution with option ovn-monitor-all=true for each test case that
> are wrapped by OVN_FOR_EACH_NORTHD macro.
>
> This will more or less double the number of test cases.
> It is possible to select a reduce set of test cases using -k "keywords".
> Keyword such as
> dp-groups=yes
> dp-groups=no
> parallelization=yes
> parallelization=no
> ovn-northd
> ovn-northd-ddlog
> ovn_monitor_all=yes
> can be used to select a range of tests, as title is searched as well.
>
> For instance, to run ovn-monitor-all tests, with dp-groups enabled and ddlog
> disabled:
> make check TESTSUITEFLAGS="-k
> dp-groups=yes,ovn_monitor_all=yes,\!ovn-northd-ddlog"
>
> Signed-off-by: Mohammad Heib <mh...@redhat.com>
> ---
Hi Mohammad,
Thanks for working on this, we were really missing tests with
conditional monitoring disabled!
> tests/ovn-macros.at | 19 ++++++++++++++++---
> tests/ovs-macros.at | 4 +++-
> 2 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
> index c6f0f6251..7484b32c3 100644
> --- a/tests/ovn-macros.at
> +++ b/tests/ovn-macros.at
> @@ -323,6 +323,19 @@ ovn_az_attach() {
> -- --may-exist add-br br-int \
> -- set bridge br-int fail-mode=secure
> other-config:disable-in-band=true \
> || return 1
> +
> + # currently this is the optimal place to add the ovn-monitor-all=true
> option,
> + # this can be implemented in a different way by redefining the sim-add
> function
> + # to add the ovn-related external-ids when we add a new simulated node
> via sim-add.
> + #
> + # wait one sec to make sure that the ovn notice and update it
> configuration
> + # according to the new option.
> + #
> + if test X$OVN_MONITOR_ALL = Xyes; then
> + ovs-vsctl set open . external_ids:ovn-monitor-all=true
> + sleep 1
> + fi
> +
I think we can avoid the "sleep 1" if we just do this in the same
transaction as ovn-remote (just above).
> start_daemon ovn-controller --enable-dummy-vif-plug || return 1
> }
>
> @@ -751,19 +764,19 @@ m4_define([OVN_FOR_EACH_NORTHD],
> [m4_foreach([NORTHD_TYPE], [ovn-northd, ovn-northd-ddlog],
> [m4_foreach([NORTHD_USE_DP_GROUPS], [yes, no],
> [m4_foreach([NORTHD_USE_PARALLELIZATION], [yes, no], [$1
> -])])])])
> +]) m4_foreach([OVN_MONITOR_ALL], [yes], [$1])])])])
Maybe I misunderstood the goal but this doesn't seem to generate
all possible combinations, e.g.:
$ make check TESTSUITEFLAGS="-l" | grep 'IP relocation using GARP request'
291: ovn.at:4906 IP relocation using GARP request -- ovn-northd --
dp-groups=yes -- parallelization=yes
292: ovn.at:4906 IP relocation using GARP request -- ovn-northd --
dp-groups=yes -- parallelization=no
293: ovn.at:4906 IP relocation using GARP request -- ovn-northd --
dp-groups=yes -- ovn_monitor_all=yes
294: ovn.at:4906 IP relocation using GARP request -- ovn-northd --
dp-groups=no -- parallelization=yes
295: ovn.at:4906 IP relocation using GARP request -- ovn-northd --
dp-groups=no -- parallelization=no
296: ovn.at:4906 IP relocation using GARP request -- ovn-northd --
dp-groups=no -- ovn_monitor_all=yes
297: ovn.at:4906 IP relocation using GARP request -- ovn-northd-ddlog
-- dp-groups=yes -- parallelization=yes
298: ovn.at:4906 IP relocation using GARP request -- ovn-northd-ddlog
-- dp-groups=yes -- parallelization=no
299: ovn.at:4906 IP relocation using GARP request -- ovn-northd-ddlog
-- dp-groups=yes -- ovn_monitor_all=yes
300: ovn.at:4906 IP relocation using GARP request -- ovn-northd-ddlog
-- dp-groups=no -- parallelization=yes
301: ovn.at:4906 IP relocation using GARP request -- ovn-northd-ddlog
-- dp-groups=no -- parallelization=no
302: ovn.at:4906 IP relocation using GARP request -- ovn-northd-ddlog
-- dp-groups=no -- ovn_monitor_all=yes
It seems to me we're missing:
- dp-groups=yes, parallelization=yes, ovn_monitor_all=yes
I think this should be:
# Defines a versions of a test with all combinations of northd and
# datapath groups.
m4_define([OVN_FOR_EACH_NORTHD],
[m4_foreach([NORTHD_TYPE], [ovn-northd, ovn-northd-ddlog],
[m4_foreach([NORTHD_USE_DP_GROUPS], [yes, no],
[m4_foreach([NORTHD_USE_PARALLELIZATION], [yes, no],
[m4_foreach([OVN_MONITOR_ALL], [yes, no], [$1
])])])])])
Also, related, I sent an RFC patch to remove dp_groups=false all
together. That will reduce the matrix size.
Speaking of which, I think we can follow up with a patch to split the CI
runs and run subsets of the test suite in "parallel". This could be
done by relying on the keyword filtering you suggested suggested in the
commit log. In .github/workflows/test.yml we now use the TESTSUITE env
variable to determine whether we need to run the unit test suite or the
system-test suite (or none). We could add a new env variable to store
the keywords filter, e.g., TESTSUITE_KW, and split the matrix further
(not tested at all):
strategy:
fail-fast: false
monitor_all: [yes, no]
parallelization: [yes, no]
matrix:
include:
- compiler: clang
testsuite: test
testsuite_kw: "${{ matrix.monitor_all}},${{matrix.parallelization}}"
asan: asan
- compiler: clang
testsuite: test
testsuite_kw: "${{ matrix.monitor_all}},${{matrix.parallelization}}"
ubsan: ubsan
>
> # Some tests aren't prepared for dp groups to be enabled.
> m4_define([OVN_FOR_EACH_NORTHD_WITHOUT_DP_GROUPS],
> [m4_foreach([NORTHD_TYPE], [ovn-northd, ovn-northd-ddlog],
> [m4_foreach([NORTHD_USE_DP_GROUPS], [no],
> [m4_foreach([NORTHD_USE_PARALLELIZATION], [yes, no], [$1
> -])])])])
> +])m4_foreach([OVN_MONITOR_ALL], [yes], [$1])])])])
Same comment here about missing combinations.
>
> # Some tests aren't prepared for ddlog to be enabled.
> m4_define([OVN_FOR_EACH_NORTHD_WITHOUT_DDLOG],
> [m4_foreach([NORTHD_TYPE], [ovn-northd],
> [m4_foreach([NORTHD_USE_DP_GROUPS], [yes, no],
> [m4_foreach([NORTHD_USE_PARALLELIZATION], [yes, no], [$1
> -])])])])
> +])m4_foreach([OVN_MONITOR_ALL], [yes], [$1])])])])
Here too.
>
> diff --git a/tests/ovs-macros.at b/tests/ovs-macros.at
> index 0482b7f5b..5d6c3ada2 100644
> --- a/tests/ovs-macros.at
> +++ b/tests/ovs-macros.at
> @@ -10,7 +10,7 @@ dnl set it as a shell variable as well.
> dnl - Skip the test if it's for ovn-northd-ddlog but it didn't get built.
> m4_rename([AT_SETUP], [OVS_AT_SETUP])
> m4_define([AT_SETUP],
> - [OVS_AT_SETUP($@[]m4_ifdef([NORTHD_TYPE], [ --
> NORTHD_TYPE])[]m4_ifdef([NORTHD_USE_DP_GROUPS], [ --
> dp-groups=NORTHD_USE_DP_GROUPS])[]m4_ifdef([NORTHD_USE_PARALLELIZATION], [ --
> parallelization=NORTHD_USE_PARALLELIZATION]))
> + [OVS_AT_SETUP($@[]m4_ifdef([NORTHD_TYPE], [ --
> NORTHD_TYPE])[]m4_ifdef([NORTHD_USE_DP_GROUPS], [ --
> dp-groups=NORTHD_USE_DP_GROUPS])[]m4_ifdef([NORTHD_USE_PARALLELIZATION], [ --
> parallelization=NORTHD_USE_PARALLELIZATION])[]m4_ifdef([OVN_MONITOR_ALL], [
> -- ovn_monitor_all=OVN_MONITOR_ALL]))
> m4_ifdef([NORTHD_TYPE], [[NORTHD_TYPE]=NORTHD_TYPE
> ])dnl
> m4_ifdef([NORTHD_USE_DP_GROUPS], [[NORTHD_USE_DP_GROUPS]=NORTHD_USE_DP_GROUPS
> @@ -19,6 +19,8 @@ m4_ifdef([NORTHD_USE_PARALLELIZATION],
> [[NORTHD_USE_PARALLELIZATION]=NORTHD_USE_
> ])dnl
> m4_ifdef([NORTHD_DUMMY_NUMA], [[NORTHD_DUMMY_NUMA]=NORTHD_DUMMY_NUMA
> ])dnl
> +m4_ifdef([OVN_MONITOR_ALL], [[OVN_MONITOR_ALL]=OVN_MONITOR_ALL
> +])dnl
> m4_if(NORTHD_TYPE, [ovn-northd-ddlog], [dnl
> AT_SKIP_IF([test $TEST_DDLOG = no])
> ])dnl
Thanks,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
