On 6/9/23 22:21, Han Zhou wrote: > On Fri, Jun 9, 2023 at 11:53 AM Mark Michelson <[email protected]> wrote: >> >> On 6/9/23 06:25, Dumitru Ceara wrote: >>> 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 > > The first is a bug, and here is the fix: > https://patchwork.ozlabs.org/project/ovn/list/?series=359066 > > The second may be categorized as unstable test, and here is an attempt to > make it stable (not sure if it can be 100% stable because performance > counter related checks are always tricky): > https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ > >>> 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? >> > > +1 for this good alternative. > >> I think this can be a good alternative. I think if we go with this, then >> we need to be vigilant and be willing to adjust if it turns out that we >> end up having more CI failures than expected. >> >> The other thing we have to avoid is haphazardly labeling tests as >> "unstable" instead of actually trying to fix them. >> >> And finally, if tests are marked as "unstable", we have to answer the >> question of whether they should even remain in the testsuite. If they're >> always being skipped and we never intend to fix the tests, then they >> probably should be removed and replaced with something else. >> > I'd like to interpret "unstable" as "should pass in most cases but may > occasionally fail, and >90% (or even higher) chance a retry will pass if it > fails in the first attempt", rather than "very likely to fail". > I think at least today we should not have test cases that fail more than > this in CI. > Of course if there are any new "very likely to fail" test cases we should > treat it as CI broken and fix them immediately (if unfortunately wasn't > caught before merging for whatever reason, which shouldn't happen in most > cases). >
I posted a patch to implement the alternative discussed above: https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
