Hi Dumitru,
thank you for reviewing this patch
On 6/8/22 12:25, Dumitru Ceara wrote:
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<[email protected]>
---
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).
updated in v3
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.:
i actually tried to save some unit execution time via not executing
parallelization with ovn_monitor_all
just to save execution time but i agree it's a bit confusing so i
applied your suggestion to v3
<https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/>.
$ 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
yes sure i will send a follow up patch to apply your suggestions to the
ci runs.
# 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
Thanks,
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
