On Wed, Sep 6, 2023 at 12:55 AM Ales Musil <[email protected]> wrote: > > 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. >
Weekly run might be helpful, but I think it would be better to have some coverage on every run. I think running with and without parallelization for gcc is a good option (and run only parallelized with sanitizer). Alternatively, we could select some of the basic tests in ovn.at to include both parallelized and unparallelized. Thanks, Han > > > > > 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
