On 10/27/23 18:05, Numan Siddique wrote: > On Fri, Jul 14, 2023 at 2:12 AM Ales Musil <[email protected]> wrote: >> >> On Thu, Jul 13, 2023 at 12:52 PM Dumitru Ceara <[email protected]> wrote: >> >>> If we want to catch new failures faster we have a better chance if CI >>> doesn't auto-retry (once). >>> >>> There are some tests that are still "unstable" and fail every now and >>> then. In order to reduce the number of false negatives keep the >>> --recheck for them. To achieve that we use a new macro, TAG_UNSTABLE, >>> to tag all these tests. The list of "unstable" tests is compiled based >>> on the following discussion: >>> https://mail.openvswitch.org/pipermail/ovs-dev/2023-June/405465.html >>> >>> In order to avoid new GitHub actions jobs, we re-purpose the last job of >>> each target type to also run the unstable tests. These jobs were >>> already running less tests than others so the additional run time should >>> not be an issue. >>> >>> Signed-off-by: Dumitru Ceara <[email protected]> >>> --- >>> V3: >>> - Addressed Ales' comment: >>> - pass RECHECK to the container running the tests in ci.sh >>> V2: >>> - Addressed Ales' comments: >>> - always run stable and unstable tests before declaring pass/fail >>> Changes in v1 (since RFC): >>> - kept recheck for unstable tests >>> - introduced TAG_UNSTABLE >>> - changed test.yml to run unstable tests in the last batch of every >>> test target type. >>> --- >>> .ci/ci.sh | 2 +- >>> .ci/linux-build.sh | 76 ++++++++++++++++++++++++++++++-------- >>> .github/workflows/test.yml | 15 ++++---- >>> tests/ovn-ic.at | 1 + >>> tests/ovn-ipsec.at | 1 + >>> tests/ovn-macros.at | 5 +++ >>> tests/ovn-northd.at | 1 + >>> tests/ovn-performance.at | 1 + >>> tests/ovn.at | 13 +++++++ >>> 9 files changed, 92 insertions(+), 23 deletions(-) >>> >>> diff --git a/.ci/ci.sh b/.ci/ci.sh >>> index 10f11939c5..3a7ee97696 100755 >>> --- a/.ci/ci.sh >>> +++ b/.ci/ci.sh >>> @@ -101,7 +101,7 @@ function run_tests() { >>> && \ >>> ARCH=$ARCH CC=$CC LIBS=$LIBS OPTS=$OPTS TESTSUITE=$TESTSUITE \ >>> TEST_RANGE=$TEST_RANGE SANITIZERS=$SANITIZERS DPDK=$DPDK \ >>> - ./.ci/linux-build.sh >>> + RECHECK=$RECHECK UNSTABLE=$UNSTABLE ./.ci/linux-build.sh >>> " >>> } >>> >>> diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh >>> index 5a79a52daf..4c5361f3ca 100755 >>> --- a/.ci/linux-build.sh >>> +++ b/.ci/linux-build.sh >>> @@ -9,6 +9,7 @@ COMMON_CFLAGS="" >>> OVN_CFLAGS="" >>> OPTS="$OPTS --enable-Werror" >>> JOBS=${JOBS:-"-j4"} >>> +RECHECK=${RECHECK:-"no"} >>> >>> function install_dpdk() >>> { >>> @@ -99,6 +100,17 @@ function configure_clang() >>> COMMON_CFLAGS="${COMMON_CFLAGS} >>> -Wno-error=unused-command-line-argument" >>> } >>> >>> +function run_tests() >>> +{ >>> + if ! make distcheck CFLAGS="${COMMON_CFLAGS} ${OVN_CFLAGS}" $JOBS \ >>> + TESTSUITEFLAGS="$JOBS $TEST_RANGE" RECHECK=$RECHECK >>> + then >>> + # testsuite.log is necessary for debugging. >>> + cat */_build/sub/tests/testsuite.log >>> + return 1 >>> + fi >>> +} >>> + >>> function execute_tests() >>> { >>> # 'distcheck' will reconfigure with required options. >>> @@ -106,27 +118,61 @@ function execute_tests() >>> configure_ovn >>> >>> export DISTCHECK_CONFIGURE_FLAGS="$OPTS" >>> - if ! make distcheck CFLAGS="${COMMON_CFLAGS} ${OVN_CFLAGS}" $JOBS \ >>> - TESTSUITEFLAGS="$JOBS $TEST_RANGE" RECHECK=yes >>> - then >>> - # testsuite.log is necessary for debugging. >>> - cat */_build/sub/tests/testsuite.log >>> + >>> + local stable_rc=0 >>> + local unstable_rc=0 >>> + >>> + if ! SKIP_UNSTABLE=yes run_tests; then >>> + stable_rc=1 >>> + fi >>> + >>> + if [ "$UNSTABLE" ]; then >>> + if ! SKIP_UNSTABLE=no TEST_RANGE="-k unstable" RECHECK=yes \ >>> + run_tests; then >>> + unstable_rc=1 >>> + fi >>> + fi >>> + >>> + if [[ $stable_rc -ne 0 ]] || [[ $unstable_rc -ne 0 ]]; then >>> exit 1 >>> fi >>> } >>> >>> +function run_system_tests() >>> +{ >>> + local type=$1 >>> + local log_file=$2 >>> + >>> + if ! sudo make $JOBS $type TESTSUITEFLAGS="$TEST_RANGE" \ >>> + RECHECK=$RECHECK; then >>> + # $log_file is necessary for debugging. >>> + cat tests/$log_file >>> + return 1 >>> + fi >>> +} >>> + >>> function execute_system_tests() >>> { >>> - type=$1 >>> - log_file=$2 >>> - >>> - configure_ovn $OPTS >>> - make $JOBS || { cat config.log; exit 1; } >>> - if ! sudo make $JOBS $type TESTSUITEFLAGS="$TEST_RANGE" >>> RECHECK=yes; then >>> - # $log_file is necessary for debugging. >>> - cat tests/$log_file >>> - exit 1 >>> - fi >>> + configure_ovn $OPTS >>> + make $JOBS || { cat config.log; exit 1; } >>> + >>> + local stable_rc=0 >>> + local unstable_rc=0 >>> + >>> + if ! SKIP_UNSTABLE=yes run_system_tests $@; then >>> + stable_rc=1 >>> + fi >>> + >>> + if [ "$UNSTABLE" ]; then >>> + if ! SKIP_UNSTABLE=no TEST_RANGE="-k unstable" RECHECK=yes \ >>> + run_system_tests $@; then >>> + unstable_rc=1 >>> + fi >>> + fi >>> + >>> + if [[ $stable_rc -ne 0 ]] || [[ $unstable_rc -ne 0 ]]; then >>> + exit 1 >>> + fi >>> } >>> >>> configure_$CC >>> diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml >>> index fe2a14c401..7d40251003 100644 >>> --- a/.github/workflows/test.yml >>> +++ b/.github/workflows/test.yml >>> @@ -92,6 +92,7 @@ jobs: >>> TESTSUITE: ${{ matrix.cfg.testsuite }} >>> TEST_RANGE: ${{ matrix.cfg.test_range }} >>> SANITIZERS: ${{ matrix.cfg.sanitizers }} >>> + UNSTABLE: ${{ matrix.cfg.unstable }} >>> >>> name: linux ${{ join(matrix.cfg.*, ' ') }} >>> runs-on: ubuntu-latest >>> @@ -104,27 +105,27 @@ jobs: >>> - { 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: "1001-", >>> unstable: unstable } >>> - { 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: clang, testsuite: test, sanitizers: sanitizers, >>> test_range: "1201-", unstable: unstable } >>> - { 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: gcc, testsuite: test, libs: -ljemalloc, test_range: >>> "1001-", unstable: unstable } >>> - { 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: "201-", unstable: unstable } >>> - { 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: >>> "201-", unstable: unstable } >>> - { 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: "201-", >>> unstable: unstable } >>> - { 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: "201-", unstable: unstable } >>> - { arch: x86, compiler: gcc, opts: --disable-ssl } >>> >>> steps: >>> diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at >>> index ceee450925..285662e3b8 100644 >>> --- a/tests/ovn-ic.at >>> +++ b/tests/ovn-ic.at >>> @@ -256,6 +256,7 @@ AT_CLEANUP >>> >>> OVN_FOR_EACH_NORTHD([ >>> AT_SETUP([ovn-ic -- gateway sync]) >>> +TAG_UNSTABLE >>> >>> ovn_init_ic_db >>> net_add n1 >>> diff --git a/tests/ovn-ipsec.at b/tests/ovn-ipsec.at >>> index 10ef978780..e621547c59 100644 >>> --- a/tests/ovn-ipsec.at >>> +++ b/tests/ovn-ipsec.at >>> @@ -1,6 +1,7 @@ >>> AT_BANNER([OVN - IPsec]) >>> >>> AT_SETUP([ipsec -- basic configuration]) >>> +TAG_UNSTABLE >>> ovn_start >>> >>> # Configure the Northbound database >>> diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at >>> index 6f2d085ae4..6420721740 100644 >>> --- a/tests/ovn-macros.at >>> +++ b/tests/ovn-macros.at >>> @@ -871,3 +871,8 @@ m4_define([RUN_OVN_NBCTL], [ >>> check ovn-nbctl ${command} >>> unset command >>> ]) >>> + >>> +m4_define([TAG_UNSTABLE], [ >>> + AT_KEYWORDS([unstable]) >>> + AT_SKIP_IF([test X"$SKIP_UNSTABLE" = Xyes]) >>> +]) >>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >>> index 3e06f14c94..c1c8287e03 100644 >>> --- a/tests/ovn-northd.at >>> +++ b/tests/ovn-northd.at >>> @@ -4531,6 +4531,7 @@ AT_CLEANUP >>> >>> OVN_FOR_EACH_NORTHD_NO_HV([ >>> AT_SETUP([northd ssl file change]) >>> +TAG_UNSTABLE >>> AT_SKIP_IF([test "$HAVE_OPENSSL" = no]) >>> PKIDIR="$(cd $abs_top_builddir/tests && pwd)" >>> AT_SKIP_IF([expr "$PKIDIR" : ".*[[ '\" >>> diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at >>> index ba329f0f64..9de0a4e770 100644 >>> --- a/tests/ovn-performance.at >>> +++ b/tests/ovn-performance.at >>> @@ -225,6 +225,7 @@ m4_define([OVN_CONTROLLER_EXPECT_HIT_COND],[ >>> ]) >>> >>> AT_SETUP([ovn-controller incremental processing]) >>> +TAG_UNSTABLE >>> # Check which operations the trigger full logical flow processing. >>> # >>> # Create and destroy logical routers, switches, ports, address sets and >>> ACLs >>> diff --git a/tests/ovn.at b/tests/ovn.at >>> index cd6d4b9ff4..cd8f481bbc 100644 >>> --- a/tests/ovn.at >>> +++ b/tests/ovn.at >>> @@ -7853,6 +7853,7 @@ AT_CLEANUP >>> OVN_FOR_EACH_NORTHD([ >>> AT_SETUP([policy-based routing: 1 HVs, 2 LSs, 1 lport/LS, 1 LR]) >>> AT_KEYWORDS([policy-based-routing]) >>> +TAG_UNSTABLE >>> ovn_start >>> >>> # Logical network: >>> @@ -8025,6 +8026,7 @@ AT_CLEANUP >>> OVN_FOR_EACH_NORTHD([ >>> AT_SETUP([policy-based routing IPv6: 1 HVs, 3 LSs, 1 lport/LS, 1 LR]) >>> AT_KEYWORDS([policy-based-routing]) >>> +TAG_UNSTABLE >>> ovn_start >>> >>> # Logical network: >>> @@ -14405,6 +14407,7 @@ AT_CLEANUP >>> OVN_FOR_EACH_NORTHD([ >>> AT_SETUP([options:multiple requested-chassis for logical port]) >>> AT_KEYWORDS([multi-chassis]) >>> +TAG_UNSTABLE >>> ovn_start >>> >>> net_add n1 >>> @@ -14507,6 +14510,7 @@ AT_CLEANUP >>> OVN_FOR_EACH_NORTHD([ >>> AT_SETUP([options:multiple requested-chassis for logical port: change >>> chassis role]) >>> AT_KEYWORDS([multi-chassis]) >>> +TAG_UNSTABLE >>> ovn_start >>> >>> net_add n1 >>> @@ -14557,6 +14561,7 @@ AT_CLEANUP >>> OVN_FOR_EACH_NORTHD([ >>> AT_SETUP([options:multiple requested-chassis for logical port: unclaimed >>> behavior]) >>> AT_KEYWORDS([multi-chassis]) >>> +TAG_UNSTABLE >>> ovn_start >>> >>> net_add n1 >>> @@ -16130,6 +16135,7 @@ AT_CLEANUP >>> >>> OVN_FOR_EACH_NORTHD([ >>> AT_SETUP([tug-of-war between two chassis for the same port]) >>> +TAG_UNSTABLE >>> ovn_start >>> >>> ovn-nbctl ls-add ls0 >>> @@ -29503,6 +29509,7 @@ AT_CLEANUP >>> >>> OVN_FOR_EACH_NORTHD([ >>> AT_SETUP([nb_cfg timestamp]) >>> +TAG_UNSTABLE >>> ovn_start >>> >>> check ovn-nbctl ls-add s2 >>> @@ -29604,6 +29611,7 @@ AT_CLEANUP >>> OVN_FOR_EACH_NORTHD([ >>> AT_SETUP([ARP replies for SNAT external ips]) >>> AT_KEYWORDS([slowtest]) >>> +TAG_UNSTABLE >>> ovn_start >>> >>> net_add n1 >>> @@ -29970,6 +29978,7 @@ AT_CLEANUP >>> # It is to cover a corner case when flows are re-processed in the I-P >>> # iteration, combined with the scenario of conflicting ACLs. >>> AT_SETUP([conflict ACLs with address set]) >>> +TAG_UNSTABLE >>> ovn_start >>> >>> ovn-nbctl ls-add ls1 >>> @@ -30165,6 +30174,7 @@ AT_CLEANUP >>> # >>> OVN_FOR_EACH_NORTHD([ >>> AT_SETUP([multi-vtep SB Chassis encap updates]) >>> +TAG_UNSTABLE >>> ovn_start >>> >>> net_add n1 >>> @@ -32354,6 +32364,7 @@ AT_CLEANUP >>> # - 2 for expanding the port group @pg1 to the 2 locally bound lports. >>> OVN_FOR_EACH_NORTHD([ >>> AT_SETUP([ACL with Port Group conjunction flow efficiency]) >>> +TAG_UNSTABLE >>> ovn_start >>> >>> net_add n1 >>> @@ -34606,6 +34617,7 @@ AT_CLEANUP >>> >>> OVN_FOR_EACH_NORTHD([ >>> AT_SETUP([recomputes]) >>> +TAG_UNSTABLE >>> ovn_start >>> >>> net_add n1 >>> @@ -35040,6 +35052,7 @@ MULTIPLE_OVS_INT([]) >>> >>> OVN_FOR_EACH_NORTHD([ >>> AT_SETUP([Check default openflow flows]) >>> +TAG_UNSTABLE >>> ovn_start >>> >>> # Check that every table has a default (i.e: priority=0) flow. >>> -- >>> 2.31.1 >>> >>> >> Looks good to me, thanks. >> >> Acked-by: Ales Musil <[email protected]> >> > > Is this patch still valid/applicable ? >
I'd really like to get this change in but I probably need to send a v4 with an updated set of tests marked as "unstable". I'll mark the v3 as "changes requested" in patchwork. Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
