Hi Ales Thanks for the fix and help decreasing the duration of the testsuite.
I have one comment (we are unexpectedly skipping some clang system tests) - see embedded below. I was also wondering: Some parts of northd would not be covered anymore by testing. I agree that those non-covered parts are small and doubling the test time for only those part has a high cost. Should we run "sometime" w/ parallelization disabled ? Hence we would not add more non-covered parts in the tests. This "sometimes" could be somehow "random" (but debugging would be more complex) or parts of weekly runs ? WDYT? Thanks Xavier On Tue, Sep 5, 2023 at 12:52 PM Ales Musil <[email protected]> wrote: > Single test needs to run 4 times to check all > the permutations, this is taking a lot of CI time > and job space. > > Remove the parallel permutation and leave parallelization > enabled for all tests, as this use case is more complex. > This allows us to greatly reduce the number of test cases > (by half) and we can also remove 6 jobs from the CI pipeline. > > The time reduction is very noticeable going down from ~30 min > to ~20 min. > > Signed-off-by: Ales Musil <[email protected]> > --- > .github/workflows/test.yml | 30 ++++++++++++------------------ > tests/ovn-macros.at | 14 +++++--------- > tests/ovs-macros.at | 6 +----- > 3 files changed, 18 insertions(+), 32 deletions(-) > > diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml > index fe2a14c40..b7f1c7bca 100644 > --- a/.github/workflows/test.yml > +++ b/.github/workflows/test.yml > @@ -102,29 +102,23 @@ jobs: > cfg: > - { compiler: gcc, opts: --disable-ssl } > - { compiler: clang, opts: --disable-ssl } > - - { compiler: gcc, testsuite: test, test_range: "-500" } > - - { compiler: gcc, testsuite: test, test_range: "501-1000" } > - - { compiler: gcc, testsuite: test, test_range: "1001-" } > + - { compiler: gcc, testsuite: test, test_range: "-300" } > + - { compiler: gcc, testsuite: test, test_range: "301-600" } > + - { compiler: gcc, testsuite: test, test_range: "601-" } > - { compiler: clang, testsuite: test, sanitizers: sanitizers, > test_range: "-300" } > - - { compiler: clang, testsuite: test, sanitizers: sanitizers, > test_range: "301-600" } > - - { compiler: clang, testsuite: test, sanitizers: sanitizers, > test_range: "601-900" } > - - { compiler: clang, testsuite: test, sanitizers: sanitizers, > test_range: "901-1200" } > - - { compiler: clang, testsuite: test, sanitizers: sanitizers, > test_range: "1201-" } > - - { compiler: gcc, testsuite: test, libs: -ljemalloc, test_range: > "-500" } > - - { compiler: gcc, testsuite: test, libs: -ljemalloc, test_range: > "501-1000" } > - - { compiler: gcc, testsuite: test, libs: -ljemalloc, test_range: > "1001-" } > + - { compiler: clang, testsuite: test, sanitizers: sanitizers, > test_range: "301-400" } > I guess it is a typo: we are now skipping tests 401 to 600 > + - { compiler: clang, testsuite: test, sanitizers: sanitizers, > test_range: "601-" } > + - { compiler: gcc, testsuite: test, libs: -ljemalloc, test_range: > "-300" } > + - { compiler: gcc, testsuite: test, libs: -ljemalloc, test_range: > "301-600" } > + - { compiler: gcc, testsuite: test, libs: -ljemalloc, test_range: > "601-" } > - { compiler: gcc, testsuite: system-test-dpdk, dpdk: dpdk, > test_range: "-100" } > - - { compiler: gcc, testsuite: system-test-dpdk, dpdk: dpdk, > test_range: "101-200" } > - - { compiler: gcc, testsuite: system-test-dpdk, dpdk: dpdk, > test_range: "201-" } > + - { compiler: gcc, testsuite: system-test-dpdk, dpdk: dpdk, > test_range: "101-" } > - { compiler: gcc, testsuite: system-test-userspace, test_range: > "-100" } > - - { compiler: gcc, testsuite: system-test-userspace, test_range: > "101-200" } > - - { compiler: gcc, testsuite: system-test-userspace, test_range: > "201-" } > + - { compiler: gcc, testsuite: system-test-userspace, test_range: > "101-" } > - { compiler: gcc, testsuite: system-test, test_range: "-100" } > - - { compiler: gcc, testsuite: system-test, test_range: "101-200" } > - - { compiler: gcc, testsuite: system-test, test_range: "201-" } > + - { compiler: gcc, testsuite: system-test, test_range: "101-" } > - { compiler: clang, testsuite: system-test, sanitizers: > sanitizers, test_range: "-100" } > - - { compiler: clang, testsuite: system-test, sanitizers: > sanitizers, test_range: "101-200" } > - - { compiler: clang, testsuite: system-test, sanitizers: > sanitizers, test_range: "201-" } > + - { compiler: clang, testsuite: system-test, sanitizers: > sanitizers, test_range: "101-" } > - { arch: x86, compiler: gcc, opts: --disable-ssl } > > steps: > diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at > index 13d5dc3d4..a100e2fa9 100644 > --- a/tests/ovn-macros.at > +++ b/tests/ovn-macros.at > @@ -170,9 +170,7 @@ ovn_start_northd() { > ovn-northd-ddlog) northd_args="$northd_args > --ddlog-record=${AZ:+$AZ/}northd$suffix/replay.dat -v" ;; > esac > > - if test X$NORTHD_USE_PARALLELIZATION = Xyes; then > - northd_args="$northd_args --n-threads=4" > - fi > + northd_args="$northd_args --n-threads=4" > > local name=${d_prefix}northd${suffix} > echo "${prefix}starting $name" > @@ -890,17 +888,15 @@ m4_define([OVN_POPULATE_ARP], > [AT_CHECK(ovn_populate_arp__, [0], [ignore])]) > # parallelization on/off and conditional monitoring on/off. > m4_define([OVN_FOR_EACH_NORTHD], > [m4_foreach([NORTHD_TYPE], [ovn-northd], > - [m4_foreach([NORTHD_USE_PARALLELIZATION], [yes, no], > - [m4_foreach([OVN_MONITOR_ALL], [yes, no], [$1 > -])])])]) > + [m4_foreach([OVN_MONITOR_ALL], [yes, no], [$1 > +])])]) > > # Defines versions of the test with all combinations of northd and > # parallelization on/off. To be used when the ovn-controller > configuration > # is not relevant. > m4_define([OVN_FOR_EACH_NORTHD_NO_HV], > - [m4_foreach([NORTHD_TYPE], [ovn-northd], > - [m4_foreach([NORTHD_USE_PARALLELIZATION], [yes, no], [$1 > -])])]) > + [m4_foreach([NORTHD_TYPE], [ovn-northd], [$1 > +])]) > > > # OVN_NBCTL(NBCTL_COMMAND) adds NBCTL_COMMAND to list of commands to be > run by RUN_OVN_NBCTL(). > diff --git a/tests/ovs-macros.at b/tests/ovs-macros.at > index 5b8da2048..f88b0ed48 100644 > --- a/tests/ovs-macros.at > +++ b/tests/ovs-macros.at > @@ -9,13 +9,9 @@ dnl - If NORTHD_TYPE is defined, then append it to the > test name and > dnl set it as a shell variable as well. > m4_rename([AT_SETUP], [OVS_AT_SETUP]) > m4_define([AT_SETUP], > - [OVS_AT_SETUP($@[]m4_ifdef([NORTHD_TYPE], [ -- > NORTHD_TYPE])[]m4_ifdef([NORTHD_USE_PARALLELIZATION], [ -- > parallelization=NORTHD_USE_PARALLELIZATION])[]m4_ifdef([OVN_MONITOR_ALL], [ > -- ovn_monitor_all=OVN_MONITOR_ALL])) > + [OVS_AT_SETUP($@[]m4_ifdef([NORTHD_TYPE], [ -- > NORTHD_TYPE])[]m4_ifdef([OVN_MONITOR_ALL], [ -- > ovn_monitor_all=OVN_MONITOR_ALL])) > m4_ifdef([NORTHD_TYPE], [[NORTHD_TYPE]=NORTHD_TYPE > ])dnl > -m4_ifdef([NORTHD_USE_PARALLELIZATION], > [[NORTHD_USE_PARALLELIZATION]=NORTHD_USE_PARALLELIZATION > -])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 > ovs_init > -- > 2.41.0 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
