On Wed, Sep 6, 2023 at 8:05 PM Han Zhou <[email protected]> wrote: > > > > 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.
That sounds like a good idea. I would actually even go with a few tests from ovn-northd.at, we don't need the controller part for that. I'll adjust the patch and post v2 with ~10 tests from ovn-northd.at having the parallelization permutations enabled. > > 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 Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA [email protected] IM: amusil _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
