| Message ID | 20221102143513.274198-1-amusil@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ci: Update jobs to use numbers instead of test flags | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Bleep bloop. Greetings Ales Musil, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line is 86 characters long (recommended limit is 79)
#46 FILE: .ci/linux-build.sh:54:
if ! sudo make -j4 check-kernel TESTSUITEFLAGS="$TEST_RANGE" RECHECK=yes; then
WARNING: Line is 90 characters long (recommended limit is 79)
#104 FILE: .github/workflows/test.yml:42:
- { compiler: clang, testsuite: test, sanitizers: sanitizers, test_range: "-300" }
WARNING: Line is 93 characters long (recommended limit is 79)
#105 FILE: .github/workflows/test.yml:43:
- { compiler: clang, testsuite: test, sanitizers: sanitizers, test_range: "301-600" }
WARNING: Line is 93 characters long (recommended limit is 79)
#106 FILE: .github/workflows/test.yml:44:
- { compiler: clang, testsuite: test, sanitizers: sanitizers, test_range: "601-900" }
WARNING: Line is 94 characters long (recommended limit is 79)
#107 FILE: .github/workflows/test.yml:45:
- { compiler: clang, testsuite: test, sanitizers: sanitizers, test_range: "901-1200" }
WARNING: Line is 91 characters long (recommended limit is 79)
#108 FILE: .github/workflows/test.yml:46:
- { compiler: clang, testsuite: test, sanitizers: sanitizers, test_range: "1201-" }
WARNING: Line is 82 characters long (recommended limit is 79)
#109 FILE: .github/workflows/test.yml:47:
- { compiler: gcc, testsuite: test, libs: -ljemalloc, test_range: "-500" }
WARNING: Line is 86 characters long (recommended limit is 79)
#110 FILE: .github/workflows/test.yml:48:
- { compiler: gcc, testsuite: test, libs: -ljemalloc, test_range: "501-1000" }
WARNING: Line is 83 characters long (recommended limit is 79)
#111 FILE: .github/workflows/test.yml:49:
- { compiler: gcc, testsuite: test, libs: -ljemalloc, test_range: "1001-" }
WARNING: Line is 84 characters long (recommended limit is 79)
#112 FILE: .github/workflows/test.yml:50:
- { compiler: clang, testsuite: test, libs: -ljemalloc, test_range: "-500" }
WARNING: Line is 88 characters long (recommended limit is 79)
#113 FILE: .github/workflows/test.yml:51:
- { compiler: clang, testsuite: test, libs: -ljemalloc, test_range: "501-1000" }
WARNING: Line is 85 characters long (recommended limit is 79)
#114 FILE: .github/workflows/test.yml:52:
- { compiler: clang, testsuite: test, libs: -ljemalloc, test_range: "1001-" }
Lines checked: 124, Warnings: 12, Errors: 0
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
On 11/2/22 15:35, Ales Musil wrote: > To prevent some jobs not running after CI scripts updates > use numbers instead of the test flags. This still allows us > to use parallelization, but without worrying about skipping > some tests by mistake. > > For "test" suites use 3 parts, with 1500 tests in mind. > That should give as additional space for future tests. > Currently, there is ~1200 tests. For sanitizers use 5 parts > instead, as they are slower in general. > > For "system-test" use 3 parts, with 300 tests in mind. > Currently, there is ~200 tests. > > In the end this patch reduces the number of jobs by 8 to 20, > which is ok as there is a limit of 20 running in parallel [0]. > > [0] https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration#usage-limits > Signed-off-by: Ales Musil <amusil@redhat.com> > --- Thanks, Ales, for the patch! It makes it harder to skip tests by accident in CI which is great. But there's still the downside that maintainers need to pay a bit more attention so that the last run "1001-" or "201-" doesn't take way longer than the rest. I'm OK with that but I wonder what Han, Mark and Numan think about this. If people are OK with it I can apply and backport the patch (assuming the CI is green [0]). Regards, Dumitru [0] https://github.com/ovsrobot/ovn/actions/runs/3378478500
On Wed, Nov 2, 2022 at 7:46 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 11/2/22 15:35, Ales Musil wrote: > > To prevent some jobs not running after CI scripts updates > > use numbers instead of the test flags. This still allows us > > to use parallelization, but without worrying about skipping > > some tests by mistake. > > > > For "test" suites use 3 parts, with 1500 tests in mind. > > That should give as additional space for future tests. > > Currently, there is ~1200 tests. For sanitizers use 5 parts > > instead, as they are slower in general. > > > > For "system-test" use 3 parts, with 300 tests in mind. > > Currently, there is ~200 tests. > > > > In the end this patch reduces the number of jobs by 8 to 20, > > which is ok as there is a limit of 20 running in parallel [0]. > > > > [0] https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration#usage-limits > > Signed-off-by: Ales Musil <amusil@redhat.com> > > --- > > Thanks, Ales, for the patch! > > It makes it harder to skip tests by accident in CI which is great. > > But there's still the downside that maintainers need to pay a bit more > attention so that the last run "1001-" or "201-" doesn't take way longer > than the rest. I agree with this, but maybe not a big deal. So I am ok with it, too. > > I'm OK with that but I wonder what Han, Mark and Numan think about this. > > If people are OK with it I can apply and backport the patch (assuming > the CI is green [0]). Backporting may be a problem if the older branch doesn't have test 1200-. Otherwise should be good. Thanks, Han > > Regards, > Dumitru > > [0] https://github.com/ovsrobot/ovn/actions/runs/3378478500 >
On 11/4/22 22:45, Han Zhou wrote: > On Wed, Nov 2, 2022 at 7:46 AM Dumitru Ceara <dceara@redhat.com> wrote: >> >> On 11/2/22 15:35, Ales Musil wrote: >>> To prevent some jobs not running after CI scripts updates >>> use numbers instead of the test flags. This still allows us >>> to use parallelization, but without worrying about skipping >>> some tests by mistake. >>> >>> For "test" suites use 3 parts, with 1500 tests in mind. >>> That should give as additional space for future tests. >>> Currently, there is ~1200 tests. For sanitizers use 5 parts >>> instead, as they are slower in general. >>> >>> For "system-test" use 3 parts, with 300 tests in mind. >>> Currently, there is ~200 tests. >>> >>> In the end this patch reduces the number of jobs by 8 to 20, >>> which is ok as there is a limit of 20 running in parallel [0]. >>> >>> [0] > https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration#usage-limits >>> Signed-off-by: Ales Musil <amusil@redhat.com> >>> --- >> >> Thanks, Ales, for the patch! >> >> It makes it harder to skip tests by accident in CI which is great. >> >> But there's still the downside that maintainers need to pay a bit more >> attention so that the last run "1001-" or "201-" doesn't take way longer >> than the rest. > > I agree with this, but maybe not a big deal. So I am ok with it, too. Actually, I have already missed this. :) Branches 22.03 and 22.06 have 2000+ tests because the test matrix depended on dp-groups being enabled/disabled too. That's not the case since >= 22.09. I still don't think it's a big deal in the end though. >> >> I'm OK with that but I wonder what Han, Mark and Numan think about this. >> >> If people are OK with it I can apply and backport the patch (assuming >> the CI is green [0]). > > Backporting may be a problem if the older branch doesn't have test 1200-. > Otherwise should be good. > I pushed this to the main branch and only backported to branch-22.09 for now. We have ~1300 tests there so we're ok. I'll wait with backporting to 22.06 and 22.03 for now. Thanks, Dumitru > Thanks, > Han > >> >> Regards, >> Dumitru >> >> [0] https://github.com/ovsrobot/ovn/actions/runs/3378478500 >> >
diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh index 2b0782aea..dc1ca5240 100755 --- a/.ci/linux-build.sh +++ b/.ci/linux-build.sh @@ -47,15 +47,10 @@ else fi if [ "$TESTSUITE" ]; then - TESTSUITEFLAGS="" - if [[ ! -z $TESTSUITE_KW ]]; then - TESTSUITEFLAGS="-k $TESTSUITE_KW" - fi - if [ "$TESTSUITE" = "system-test" ]; then configure_ovn $OPTS make -j4 || { cat config.log; exit 1; } - if ! sudo make -j4 check-kernel TESTSUITEFLAGS="$TESTSUITEFLAGS" RECHECK=yes; then + if ! sudo make -j4 check-kernel TESTSUITEFLAGS="$TEST_RANGE" RECHECK=yes; then # system-kmod-testsuite.log is necessary for debugging. cat tests/system-kmod-testsuite.log exit 1 @@ -67,7 +62,7 @@ if [ "$TESTSUITE" ]; then export DISTCHECK_CONFIGURE_FLAGS="$OPTS" if ! make distcheck CFLAGS="${COMMON_CFLAGS} ${OVN_CFLAGS}" -j4 \ - TESTSUITEFLAGS="$TESTSUITEFLAGS -j4" RECHECK=yes + TESTSUITEFLAGS="-j4 $TEST_RANGE" RECHECK=yes then # testsuite.log is necessary for debugging. cat */_build/sub/tests/testsuite.log diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 7a59cd478..88c48dd2c 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -24,7 +24,7 @@ jobs: M32: ${{ matrix.cfg.m32 }} OPTS: ${{ matrix.cfg.opts }} TESTSUITE: ${{ matrix.cfg.testsuite }} - TESTSUITE_KW: ${{ matrix.cfg.testsuite_kw }} + TEST_RANGE: ${{ matrix.cfg.test_range }} SANITIZERS: ${{ matrix.cfg.sanitizers }} name: linux ${{ join(matrix.cfg.*, ' ') }} @@ -36,31 +36,23 @@ jobs: cfg: - { compiler: gcc, opts: --disable-ssl } - { compiler: clang, opts: --disable-ssl } - - { compiler: gcc, testsuite: test, testsuite_kw: "parallelization=yes,ovn_monitor_all=yes" } - - { compiler: gcc, testsuite: test, testsuite_kw: "parallelization=yes,ovn_monitor_all=no" } - - { compiler: gcc, testsuite: test, testsuite_kw: "parallelization=no,ovn_monitor_all=yes" } - - { compiler: gcc, testsuite: test, testsuite_kw: "parallelization=no,ovn_monitor_all=no" } - - { compiler: gcc, testsuite: test, testsuite_kw: "!ovn-northd" } - - { compiler: clang, testsuite: test, sanitizers: sanitizers, testsuite_kw: "parallelization=yes,ovn_monitor_all=yes" } - - { compiler: clang, testsuite: test, sanitizers: sanitizers, testsuite_kw: "parallelization=yes,ovn_monitor_all=no" } - - { compiler: clang, testsuite: test, sanitizers: sanitizers, testsuite_kw: "parallelization=no,ovn_monitor_all=yes" } - - { compiler: clang, testsuite: test, sanitizers: sanitizers, testsuite_kw: "parallelization=no,ovn_monitor_all=no" } - - { compiler: clang, testsuite: test, sanitizers: sanitizers, testsuite_kw: "!ovn-northd" } - - { compiler: gcc, testsuite: test, libs: -ljemalloc, testsuite_kw: "parallelization=yes,ovn_monitor_all=yes" } - - { compiler: gcc, testsuite: test, libs: -ljemalloc, testsuite_kw: "parallelization=yes,ovn_monitor_all=no" } - - { compiler: gcc, testsuite: test, libs: -ljemalloc, testsuite_kw: "parallelization=no,ovn_monitor_all=yes" } - - { compiler: gcc, testsuite: test, libs: -ljemalloc, testsuite_kw: "parallelization=no,ovn_monitor_all=no" } - - { compiler: gcc, testsuite: test, libs: -ljemalloc, testsuite_kw: "!ovn-northd" } - - { compiler: clang, testsuite: test, libs: -ljemalloc, testsuite_kw: "parallelization=yes,ovn_monitor_all=yes" } - - { compiler: clang, testsuite: test, libs: -ljemalloc, testsuite_kw: "parallelization=yes,ovn_monitor_all=no" } - - { compiler: clang, testsuite: test, libs: -ljemalloc, testsuite_kw: "parallelization=no,ovn_monitor_all=yes" } - - { compiler: clang, testsuite: test, libs: -ljemalloc, testsuite_kw: "parallelization=no,ovn_monitor_all=no" } - - { compiler: clang, testsuite: test, libs: -ljemalloc, testsuite_kw: "!ovn-northd" } - - { compiler: gcc, testsuite: system-test, testsuite_kw: "parallelization=yes,ovn_monitor_all=yes" } - - { compiler: gcc, testsuite: system-test, testsuite_kw: "parallelization=yes,ovn_monitor_all=no" } - - { compiler: gcc, testsuite: system-test, testsuite_kw: "parallelization=no,ovn_monitor_all=yes" } - - { compiler: gcc, testsuite: system-test, testsuite_kw: "parallelization=no,ovn_monitor_all=no" } - - { compiler: gcc, testsuite: system-test, testsuite_kw: "!ovn-northd" } + - { compiler: gcc, testsuite: test, test_range: "-500" } + - { compiler: gcc, testsuite: test, test_range: "501-1000" } + - { compiler: gcc, testsuite: test, test_range: "1001-" } + - { 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, libs: -ljemalloc, test_range: "-500" } + - { compiler: clang, testsuite: test, libs: -ljemalloc, test_range: "501-1000" } + - { compiler: clang, testsuite: test, libs: -ljemalloc, test_range: "1001-" } + - { 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, m32: m32, opts: --disable-ssl} steps:
To prevent some jobs not running after CI scripts updates use numbers instead of the test flags. This still allows us to use parallelization, but without worrying about skipping some tests by mistake. For "test" suites use 3 parts, with 1500 tests in mind. That should give as additional space for future tests. Currently, there is ~1200 tests. For sanitizers use 5 parts instead, as they are slower in general. For "system-test" use 3 parts, with 300 tests in mind. Currently, there is ~200 tests. In the end this patch reduces the number of jobs by 8 to 20, which is ok as there is a limit of 20 running in parallel [0]. [0] https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration#usage-limits Signed-off-by: Ales Musil <amusil@redhat.com> --- .ci/linux-build.sh | 9 ++------ .github/workflows/test.yml | 44 ++++++++++++++++---------------------- 2 files changed, 20 insertions(+), 33 deletions(-)