In theory, this idea is great. By removing the recheck, our CI will
potentially turn red more often, resulting in us investigating our tests
to resolve what makes them flaky. Eventually, we will resolve all of the
flakes and CI will be green. From that point on, any time the CI turns
red, it will immediately get our attention and we will fix whatever
needs fixing.
I have two main fears here:
1) If we don't fix the existing flaky tests quickly enough, it will be
considered "normal" for CI to be red, meaning we are less likely to
notice genuine failures when they happen. Worse, developers familiar
with the flaky reputation of tests will just issue their own rechecks
until the CI turns green.
2) Let's say we track down and fix all current flaky tests. Developers
will soon write new tests, and those tests may end up being flaky. Then
our CI will start turning red again due to test failures seemingly
unrelated to code being committed. The result is the same as (1).
By leaving the recheck intact, we are less likely to notice and fix
flaky tests. That's also bad.
I think that if we are going to remove the recheck now, then we have to
be fully dedicated to eradicating all existing flaky tests. If we as a
project can be dedicated to fixing flaky tests *immediately* when they
are found, then we could move forward with this change now. I just don't
trust us to actually do it.
I think a more plausible idea would be to identify all known flaky
tests, fix those, and then remove the recheck. This way, we only have to
deal with new flaky tests that get written in the future. I think we can
deal with this situation more easily than suddenly going from green CI
to red CI because of existing flaky tests.
Everything below this sentence is wishful thinking and shouldn't get in
the way of approving/rejecting this change.
I also think that we need to identify the common causes of flaky tests,
and take measures to prevent them from happening in the future. There
are a few ways I can think of to accomplish this:
1) Ensure people performing code reviews are aware of these patterns and
point them out during code review.
2) Automatically prevent known flaky patterns from existing in our code.
For instance, we could introduce a build-time check that ensures that
nobody attempts to sleep during a test to "let things settle."
3) Provide functions/macros to use in the testsuite that avoid
potentially flaky behavior. As a hypothetical example, let's say that a
common cause of flakes is not using "--wait=hv" or "--wait=sb" after a
series of ovn-nbctl commands. We could provide a macro like:
OVN_NBCTL([hv], [
lr-add ro0
ls-add sw0
])
(That's probably bad syntax, but I think you get the idea)
This would expand to something like:
check ovn-nbctl lr-add ro0
check ovn-nbctl ls-add sw0
check ovn-nbctl --wait=hv sync
If we combine this with the idea from (2), we could prevent all bare
uses of `ovn-nbctl` in tests, thus preventing this flaky behavior from
existing.
4) Identify commonly used patterns in tests and provide macros that
essentially do the work for us. Some examples might be:
a) Common logical network topologies could be set up for you by calling
a single macro.
b) Checking logical flows could have a macro that dumps the information
you care about, sorts the output, anonymizes the table numbers, and
eliminates odd spacing in the output.
c) Ensuring TCP/UDP/IP connectivity between two endpoints could
potentially be done with a macro or two, assuming that the traffic you
are sending is not overly specific.
The idea here is that by having commonly used macros that are
bulletproof, we are less likely to introduce flakes by hand-coding the
same scenarios. It also would likely reduce the code size of the
testsuite, which is a nice bonus.
5) Enhanced debugability of tests would be great. There are too many
times that I've looked at testsuite.log on a failing test and had no
idea where to start looking for the failure. There are other times where
it's clear what failed, but trying to figure out why is more difficult.
Unfortunately, there isn't likely to be a magic cure-all for this problem.
On 6/8/23 10:04, Dumitru Ceara wrote:
If we want to catch new failures faster we have a better chance if CI
doesn't auto-retry (once).
Signed-off-by: Dumitru Ceara <[email protected]>
---
NOTE: Sending this as RFC to start the discussion with the community.
---
.ci/linux-build.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index 907a0dc6c9..64f7a96d91 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -66,7 +66,7 @@ function execute_tests()
export DISTCHECK_CONFIGURE_FLAGS="$OPTS"
if ! make distcheck CFLAGS="${COMMON_CFLAGS} ${OVN_CFLAGS}" $JOBS \
- TESTSUITEFLAGS="$JOBS $TEST_RANGE" RECHECK=yes
+ TESTSUITEFLAGS="$JOBS $TEST_RANGE"
then
# testsuite.log is necessary for debugging.
cat */_build/sub/tests/testsuite.log
@@ -81,7 +81,7 @@ function execute_system_tests()
configure_ovn $OPTS
make $JOBS || { cat config.log; exit 1; }
- if ! sudo make $JOBS $type TESTSUITEFLAGS="$TEST_RANGE" RECHECK=yes; then
+ if ! sudo make $JOBS $type TESTSUITEFLAGS="$TEST_RANGE"; then
# $log_file is necessary for debugging.
cat tests/$log_file
exit 1
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
