On Wed, Sep 6, 2023 at 9:42 AM Xavier Simonart <[email protected]> wrote:
> Hi Ales > Hi Xavier, thank you for the review. > 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? > Having it as part of the weekly runs is a possibility, I was also thinking that we could run them as part of the one gcc cycle? Either with -jemalloc or without (arguably we wouldn't cover the sanitizer case). The unfortunate thing about weekly runs and such is the propagation of the flag and the whole pipeline is more complex. I'll wait for some other suggestions before I'll put out v2. > > 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 > Oops yes definitely typo, I'll fix that in v2. > + - { 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 >> >> Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
