[CI] Add Design Doc for Premerge Advisor #599
Conversation
boomanaiden154
force-pushed
the
premerge-advisor-design-doc
branch
2 times, most recently
from
d12d861 to
e688059
Compare
This patch adds a design document for the premerge advisor that we plan on implementing.
boomanaiden154
force-pushed
the
premerge-advisor-design-doc
branch
from
e688059 to
3195b70
Compare
boomanaiden154
requested review from
Keenuts,
cmtice,
dschuff,
gburgessiv and
lnihlen
premerge/premerge-advisor.md
Outdated
| failures. It is also not reasonable to expect PR authors to spend time | ||
| familiarizing themselves with all known flaky tests and dig through postcommit | ||
| testing logs to see if the failures in their premerge run also occur in main. |
There was a problem hiding this comment.
nit/maybe out-of-topic: is that really unreasonable to look at failure logs and eyeball if we are responsible? Knowing which tests are flaky yes, but we should expect contributors to at least look at the logs if something fails
There was a problem hiding this comment.
The majority of people already look at the logs. But you also need to be able to correlate the failures in your PR to postcommit failures, which is this missing piece. The heuristics I've seen most people use to determine if their PR is the culprit don't take into account whether or not there is a correlation and thus they are pretty error prone.
| We plan on implementing the web server in python with the `flask` library. All | ||
| contributors to the premerge infrastructure are already familiar with Python, | ||
| building web servers in Python with `flask` is relatively easy, and we do not | ||
| need high performance or advanced features. For storage, we plan on using SQLite | ||
| as it has support built in to Python, does not require any additional complexity | ||
| in terms of infrastructure setup, and is reliable. |
There was a problem hiding this comment.
Could we consider going without the DB?
A DB, even SQLite means we have a schema, and if we change it, have a migration path.
What we need is history on the N last commits runs on main. Could this be stored in memory in the flask process? (assuming memory shared across the N workers if we use waitress)
It means if we restart the cluster, the first commit won't have history, but this shouldn't be that often and would simplify greatly incremental work on this advisor.
There was a problem hiding this comment.
Whether we keep things in-memory or inside a DB, we still need a schema. The schema that we would need is also not complicated. We just need the file, the workflow run number/commit SHA, the failure message, and maybe a timestamp/otherwise unique ID. Keeping the data for all of eternity also isn't that necessary. We can just dump the existing DB and start fresh, just like we would if we were storing info in memory and the server restarted.
Storing the information in memory makes it much harder to keep track of flaky tests over long periods of time, and we lose all information on flaky tests everytime the server restarts, which isn't ideal. Server restarts are normal and expected with k8s, so the design needs to take them into account.
| Given we have two identical clusters that need to be able to function | ||
| independently of each other, we also need to duplicate the web server for the | ||
| premerge advisor. Queries and failure information uploads will go to the | ||
| cluster-local premerge advisor web server. Periodically (eg once every thirty |
There was a problem hiding this comment.
By not polling, and requesting each job to upload data to the service/DB, while also having clustering means we now need to handle data inconsistencies.
What about:
- each job uploads an artifact to the github action storage (meaning retention policies etc is handled by Github).
- one instance of this service is polling (as the metric container does), and fetches those artifacts as they are published.
- the service keeps the test state in memory (no DB schema to migrate as we iterate on this service)
- the service then posts a message to each PR as workflows are polled depending on the stored state.
We could also have a single instance of this service as this is not a critical piece, a failure would just prevent sending helpful comments, but wouldn't back-up the queue.
There was a problem hiding this comment.
We don't really need to handle data inconsistencies. The system is designed for eventual consistency on the order of minutes because having slightly inconsistent results doesn't really matter.
We can't use Github artifacts because we also need to support uploading data from the postcommit builders running on buildbot.
I think this is reasonably critical to the UX of the premerge system and not having two means we probably need to push maintenance to off-hours more often, but ack on looking more into keeping a single instance.
There was a problem hiding this comment.
You should explicitly state in the design doc why the inconsistencies won't be a problem. It would also be a good idea to outline the basic DB schema. Try to design it so that it's easily extensible if/when we decide we need to extend it.
There was a problem hiding this comment.
Also, if we're planning on using the DB to help identify flaky tests over time, which your comments imply, then that should be stated in the design doc as well.
There was a problem hiding this comment.
What was the upshot of "looking more into keeping a single instance"?
premerge/premerge-advisor.md
Outdated
| While the premerge infrastructure is generally reliable, tests that are broken | ||
| at HEAD, or tests that are flaky can significantly degrade the user experience. | ||
| Failures unrelated to the PR being worked on can lead to warning fatigue which | ||
| can compound this problem when people land failing tests into main that they |
There was a problem hiding this comment.
The entire phrase starting with "which can compound" needs to be reworked -- it is not clear to me what you are trying to say.
There was a problem hiding this comment.
Added some text to describe what I'm trying to get at. Please take a look and let me know what you think.
premerge/premerge-advisor.md
Outdated
| occur regularly whether due to flaky tests, or due to main being broken. Efforts | ||
| to fix these issues at the source and keep main always green and deflake tests | ||
| are laudable, but not a scalable solution to the problem of false positive | ||
| failures. It is also not reasonable to expect PR authors to spend time |
There was a problem hiding this comment.
suggested changes: "laudable, but not a scalable solution to" => "ongoing, but are not sufficient to prevent" and "failures." => "failure reports."
There was a problem hiding this comment.
Changed from laudable to ongoing.
I think it's still important to mention that this is a scalability problem. And fixing flaky tests fixes the failures (which is ideal) rather than just making them go unreported.
premerge/premerge-advisor.md
Outdated
| Information on how the premerge jobs will query the server and display results | ||
| about flaky/already broken tests is in | ||
| [the section below](#informing-the-user). We plan on having both the premerge | ||
| and postcommit jobs upload failure information to the web server. This enables |
There was a problem hiding this comment.
It would be appropriate to include a link to the postsubmit testing docs.
There was a problem hiding this comment.
Also, I assume the data being uploaded from postcommit and PR jobs will be tagged and treated differently somehow (i.e. we want to store one but not the other). More details about that would be good.
There was a problem hiding this comment.
We want to store both. But they will be tagged appropriately.
Added some text on how exactly we'll tag them. I don't think a design doc is the best place to get into exactly how the database schemas will be set up, but I can add that information if desired.
| workflow failure and then update that comment everytime we get new data. This | ||
| prevents creating much noise. This does mean that the comment might get buried | ||
| in long running reviews, but those are not the common case and people should | ||
| learn to expect to look for the comment earlier in the thread in such cases. |
There was a problem hiding this comment.
Shouldn't the comment about which test failures the user can ignore go into the Summary, at the very top?
| in long running reviews, but those are not the common case and people should | ||
| learn to expect to look for the comment earlier in the thread in such cases. | ||
|
|
||
| ## Alternatives Considered |
There was a problem hiding this comment.
It looks like there are a number of off-the-shelf commercially available and open source flake detector systems. Here's a quote from Gemini:
A unit test flake detector identifies tests that pass and fail inconsistently without code changes by rerunning tests multiple times and analyzing the results for differing outcomes. These tools use historical data and CI/CD pipeline logs to flag unreliable tests, with features like auto-rerunning, historical analysis, and test quarantine for managing flagged tests. Popular examples include features within CI platforms like Azure DevOps, Mergify, and TeamCity, as well as third-party tools like Trunk.io, the flaky-tests-detection PyPI package.
I think it's worth discussing the build/buy/re-use tradeoffs here with at least a few of these.
There was a problem hiding this comment.
Added a section to cover this. This actually ended up being a pretty useful exercise.
trunk.io looks almost exactly like what we want other than that it (along with all the other alternatives) only work with flaky tests rather than failures at head. We really need both to create a cohesive solution for LLVM.
| failure information so that it can be queried later. We plan on having jobs | ||
| extract failure logs and upload them to a web server. This web server in | ||
| addition to having an endpoint to accept uploads will have an endpoint that will | ||
| accept test failure information (logs and filenames) and return whether or not |
There was a problem hiding this comment.
What's the difference between accepting "uploads" and accepting "test failure information". I think I know what is meant (PR info vs postsubmit testing info) but that is not really clear here.
| about flaky/already broken tests is in | ||
| [the section below](#informing-the-user). We plan on having both the premerge | ||
| and [postcommit jobs](post-submit-testing.md) upload failure information to the | ||
| web server. This enables the web server to know about failures that are not the |
There was a problem hiding this comment.
Maybe you should say something here about the data from the post commit jobs being stored in the DB; failures form the precommit jobs get compared against the DB to determine if the failures are new or already existed.
| relatively quickly, and minor discrepancies for queries between the clusters are | ||
| not a big deal. | ||
|
|
||
| ### Identifying if Failures are Novel |
There was a problem hiding this comment.
I think this section needs a better name. Maybe something like "Classifying Test Failures"?
|
|
||
| ### Identifying if Failures are Novel | ||
|
|
||
| When the web server is sent a request to explain a list of failures, it needs to |
There was a problem hiding this comment.
Is this the same thing as the PR just uploading its test results? Or is this actually a separate request?
| be able to determine whether a failure has occurred previously. To do this, the | ||
| web server will keep a list of currently active failures in `main` and a list of | ||
| flaky tests. The list of flaky tests will be computed from historical data on | ||
| startup from a rolling window of data. The exact heuristic is left to |
There was a problem hiding this comment.
Do you have a rough heuristic in mind that you could outline here?
| list, they will be added to the currently active failure list. Failures in the | ||
| currently active list not present in the latest postcommit run will be removed | ||
| from the currently active list. In addition to data from postcommit testing, if | ||
| a PR lands with unexplained premerge failures, those failures are also added to |
There was a problem hiding this comment.
What do you mean by "unexplained premerge failures" here?
| After the response from the server has been recieved, the workflow will then | ||
| construct a comment. It will contain the failure information, and if relevant, | ||
| information/links showing the tests are flaky (and the flakiness rate) or are | ||
| broken in `main`. If all of the test failures are due to flakes or failures in |
There was a problem hiding this comment.
This sentence is nearly identical with the first sentence of the next paragraph. I think maybe you can omit it here.
| information/links showing the tests are flaky (and the flakiness rate) or are | ||
| broken in `main`. If all of the test failures are due to flakes or failures in | ||
| `main`, the comment will indicate to the user that they can safely merge their | ||
| PR despite the test failures. We plan to construct the comment in a manner |
There was a problem hiding this comment.
"comment" => "review comment" (maybe)?. Also, since (I think), we're also going to put this information into the Summary section, you should mention that.
|
|
||
| In order to ensure that main is not broken by new patches, we need to ensure | ||
| that every commit is tested directly on top of `main` before landing. This is | ||
| not feasible given LLVM's current processes where pushing directly to main is a |
There was a problem hiding this comment.
I think you should omit the sentence about direct pushes.
| compatible with the new state of main. This would most likely involve merge | ||
| queues which would introduce new CI load and are also not compatible with LLVM's | ||
| existing practices for the same reason requiring premerge checks to be run | ||
| before landing are not. |
There was a problem hiding this comment.
I think you should omit the final phrase ("for the same reason requiring premerge checks to be run before landing are not").
|
At a high level I think the design looks pretty good now. I've asked for additions or clarifications in a few places. I also have a bunch of editorial suggestions, but those can wait for now. |
4 participants
This patch adds a design document for the premerge advisor that we plan on implementing.