Hi Mark,
Thanks for your feedback on this!
On 6/8/23 21:58, Mark Michelson wrote:
> 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.
>
What about the following compromise instead?
Identify all known flaky tests, fix the ones we can fix without too much
effort, tag the ones that we couldn't easily fix with a new keyword
(e.g., "unstable" or "flaky" or whatever), and only run recheck for
those.
In support of this I did a test last night. I force pushed the commit
that removes the --recheck multiple times in my fork to trigger CI.
The GitHub action ran 23 times which means the unit and system tests
were ran 3 x 23 = 69 times. I collected the results and grouped them
by test and counted how many times they failed (disclaimer: it's not
completely fair because some tests run with different variations more
times than others but it should be a good enough first approximation).
I got:
40 ovn.at:30812 propagate Port_Binding.up to NB and OVS --
ovn-northd
34 ovn-northd.at:9487 LSP incremental processing -- ovn-northd
8 ovn.at:14436 options:multiple requested-chassis for logical
port: unclaimed behavior -- ovn-northd
7 ovn.at:16010 tug-of-war between two chassis for the same port --
ovn-northd
6 ovn.at:29369 nb_cfg timestamp -- ovn-northd
5 ovn-northd.at:4507 northd ssl file change -- ovn-northd
4 ovn-performance.at:227 ovn-controller incremental processing
3 ovn.at:34900 Check default openflow flows -- ovn-northd
3 ovn.at:34466 recomputes -- ovn-northd
3 ovn.at:14284 options:multiple requested-chassis for logical port
-- ovn-northd
2 ovn.at:7909 policy-based routing IPv6: 1 HVs, 3 LSs, 1
lport/LS, 1 LR -- ovn-northd
1 system-ovn.at:6525 egress qos -- ovn-northd
1 system-ovn.at:11151 ovn mirroring -- ovn-northd
1 ovn-northd.at:8809 Address set incremental processing -- ovn-northd
1 ovn-controller.at:702 ovn-controller - ovn action metering --
ovn-northd
1 ovn.at:29467 ARP replies for SNAT external ips -- ovn-northd
1 ovn.at:13171 1 LR with HA distributed router gateway port --
ovn-northd
The first two are probably real bugs and I reported them separately
yesterday [0] but the other 15 didn't fail that often. I would even
go one step further and ignore the ones that only failed once (it's
acceptable IMO every now and then a maintainer has to re-trigger CI).
That would leave us with 9 tests that failed during this exercise and
need either to be fixed or to be tagged as "unstable":
8 ovn.at:14436 options:multiple requested-chassis for logical
port: unclaimed behavior -- ovn-northd
7 ovn.at:16010 tug-of-war between two chassis for the same port --
ovn-northd
6 ovn.at:29369 nb_cfg timestamp -- ovn-northd
5 ovn-northd.at:4507 northd ssl file change -- ovn-northd
4 ovn-performance.at:227 ovn-controller incremental processing
3 ovn.at:34900 Check default openflow flows -- ovn-northd
3 ovn.at:34466 recomputes -- ovn-northd
3 ovn.at:14284 options:multiple requested-chassis for logical port
-- ovn-northd
2 ovn.at:7909 policy-based routing IPv6: 1 HVs, 3 LSs, 1
lport/LS, 1 LR -- ovn-northd
Would you consider this compromise as a good alternative?
[0] https://mail.openvswitch.org/pipermail/ovs-dev/2023-June/405418.html
>
> 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.
>
I think all 5 points above are things that are nice to have regardless
of --recheck happening or not.
Thanks,
Dumitru
> 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
