add more lessons learned

Author: matthiaskrgr

src/release/rollups.md

@@ -2,16 +2,15 @@
 
 ## Background
 
-The Rust project has a policy that every pull request must be tested after merge
-before it can be pushed to master. As PR volume increases, this can scale poorly,
-especially given the long (~3.5hr) current CI duration for Rust.
+The Rust project has a policy that every pull request must be tested before it can be merged to master.
+As PR volume increases, this can scale poorly, especially given the long (~3.5hr) current CI duration for Rust.
 
 Enter rollups! Changes that are small, not performance sensitive, or not platform
 dependent are marked with the `rollup` command to bors (`@bors r+ rollup` to
 approve a PR and mark it as a rollup, `@bors rollup` to mark a previously approved
 PR, `@bors rollup-` to un-mark as a rollup).  'Performing a Rollup' then means
-collecting these changes into one PR and merging them all at once. The rollup
-command accepts four values: `always`, `maybe`, `iffy`, and `never`. See [the
+collecting these independent changes into one bigger PR with the goal of merging them all at once.
+The rollup command accepts four values: `always`, `maybe`, `iffy`, and `never`. See [the
 Rollups section] of the review policies for guidance on what these different
 statuses mean.
 
@@ -48,40 +47,40 @@ queue has been merged.
 
    Tools and subtree syncs should use `p=5`, like rollups, so they interleave between rollups, as
    tools and subtree syncs are typically more tricky to fix than rebasing PRs.
+   As the only exception, you shall chose a minimally higher priority (`x+1`) if a PR you include (such as a subtree sync) is already of `p=x`,
+   with the purpose of ensuring that the rollup is tested earlier than subtree sync it includes.
 
-3. If the rollup fails, use the logs rust-log-analyzer
-   provides to bisect the failure to a specific PR and
-   `@bors r-` the PR. If the PR is running, you need to do `@bors r- retry`. Otherwise,
-   your rollup succeeded. If it did, proceed to the next rollup (every now and
-   then let `rollup=never/iffy` and toolstate PRs progress).
+3. If the rollup fails, use the rust-log-analyzer or ci logs
+   to pinpoint the failure to a specific PR and unapprove (`@bors r-`) the PR.
+   If the PR is running, you need to do `@bors r- retry`. Otherwise,
+   your rollup succeeded, great! If it did, proceed to the next rollup (every other time, let the `rollup=never/iffy` and toolstate PRs progress).
 4. Recreate the rollup without the offending PR starting again from **1.**. 
    There's a link in the rollup PR's body to automatically prefill the rollup UI 
    with the existing PRs (minus any PRs that have been `r-`d)
    Try avoiding adding any additional PR to the current "batch" as this
-   unnecessarily increases the chance of test failures.
-   Rollups should not overlap, if a PR is already contained in a rollup that is not closed,
-   it should not be added to another different rollup at the same time.
+   unnecessarily increases the chance of test failures: select a batch of hopefully conflict-free PRs, weed out the failures, merge, repeat!
+   Rollups should not overlap, if a PR is already contained in a rollup that waiting in the queue, it should not be added to another different rollup at the same time.
    If a rollup fails and you are not sure which PR caused the problem, 
    you may bisect the rollup and split it up into two rollups until the offending PR becomes clear.
 
 ## Selecting Pull Requests
 
 The queue is sorted by rollup status. In general:
 
-- A good rollup contains a bunch of `maybe` (unmarked) PRs, and a large pile of `always` PRs. You
-  can include one or two `iffy` PRs to amortize the cost of testing full CI.
+- A good rollup contains a bunch of `maybe` (unmarked) PRs, and a large pile of `always` PRs.
+  You can include one or two `iffy` PRs to amortize the cost of testing full CI.
 - If you have a lot of time, you can also make a rollup just from `iffy` PRs (if there are enough of
   them) and weed out the failures one by one.
 - A rollup must **never** include `rollup=never` PRs.
 
 The actual absolute size of the rollup can depend based on experience and current length of the queue.
 People new to making rollups might start with including 1 `iffy`, 2 `maybe`s, and 4 `always`s. Usually 6-8 PRs per rollup is a good compromise.
-There is rarely a need to roll up more than 10 PRs at once (unless there are >30 PRs waiting the queue), keep in mind that we also try to minimize regressions per merge.
+There is rarely a need to roll up more than 15 PRs at once (unless there are >40 PRs waiting the queue), keep in mind that we also try to minimize regressions per merge.
 
 Limit the size of rollups, *even if* the queue is backed up. Large rollups run the risk of failing
 or merge conflicts, and smaller rollups keep potential regressions [bisectable]. On average, rollups
-are 7 PRs large, often varying from 5 to 10 depending on the number of `rollup=always` PRs that can
-be included.
+are 7 PRs large, often varying from 5 to 12 depending on the number of `rollup=always` PRs that can
+be included. Rollups of 15+ prs can be made for the sake of weeding out failures via try-jobs but should not be put into the merge queue as to not block other prs from being merged in the meantime
 
 Don't hesitate to downgrade the rollup status of a PR! If your intuition tells you that a `rollup=always` PR has some chances for failures, mark it `rollup=maybe` or better `rollup=iffy`. A lot of the unmarked `maybe` PRs are categorized as such because the reviewer may not have considered rollupability, so it's always worth picking them with a critical eye. Similarly, if a PR causes your rollup to fail, it's worth considering changing its rollup status.
 
@@ -90,14 +89,14 @@ Generally, PRs that touch CI configuration or the bootstrapping process are prob
 Avoid having too many PRs with large diffs or subtree changes in the same rollup. Self-contained submodule changes (such as `miri` updates) on the other hand may be fine to be rolled up with other unrelated PRs.
 Also avoid having PRs you suspect will have large perf impacts and mark them as `rollup=never`.
 
-If an `iffy` PR keeps failing in a rollups, it should be marked `never` to prevent it from causing further problems inside unrelated rollups. This will also cause it to bump up in front of all `maybe`s in the queue and the author will get feedback quicker in case of subsequent failures.
+If an `iffy` PR keeps failing in a rollup, it should be marked `never` to prevent it from causing further problems inside unrelated rollups. This will also cause it to bump up in front of all `maybe`s in the queue and the author will get feedback quicker in case of subsequent failures.
 It should be noted which runner the PR failed on, to run this runner as a `try-job` job and make sure it succeeds there before another merge is attempted (example on syntax [here]).
 In general, if possible, try to test a failed PR via a handful of carefully selected try-jobs instead of having to run the full battery of all 60 runners on if it's likely a PR may fail again.
 
-To not have `never` or `iffy` PRs stuck in the queue indefinitely, it is recommended to alternate
+To not have `never` or `iffy` PRs stuck in the queue indefinitely, we should alternate
 between rollup and non-rollup prs, so one `never`, one rollup, one `iffy`, one `rollup`, one `never`
 etc, until most of the `never`s are merged. You can selectively bump the priority on certain
-`iffy`/`never` PRs to `p=1` (or even `p=5`) if you want to interleave them between rollups.
+`iffy`/`never` PRs to `p=1` (up to `p=5`) if you want to interleave them between rollups, while keeping a rollup / non-rollup alternation.
 
 If you are the only person making rollups, you can also leave a couple of `never`/`iffy`s for a time
 where you know nobody will be doing rollups actively, or for weekends which generally see a lower
@@ -115,14 +114,15 @@ repeatedly, to have the same priority as rollups or sandwich between rollups.
 
 Ultimately, we want to keep the number of regressions per merge at a minimum while also minimizing
 the amount of time between approval and the final merge of a PR, to avoid unnecessary merge
-conflicts and rebases.
+conflicts and rebases. Aim for a steady, continuous and even flow of similarly-sized rollups alternated by non-rollups.
+
 
 ## Failed rollups
 
-If the rollup has failed, run the `@bors retry` command if the
+If a rollup has failed, run the `@bors retry` command if the
 failure was spurious (e.g. due to a network problem or a timeout).
-There may be a matching `CI-spurious-fail-.*` label that you can use to tag the PR as such, to help
-discover common fail patterns.
+There may be a matching `CI-spurious-fail-.*` [label] that you can use to tag the PR as such, to help
+discover common failure patterns.
 
 If it wasn't spurious, find the offending PR and return it to the author to be fixed by copying a
 link to the `rust-log-analyzer` comment, and leaving a comment like
@@ -138,25 +138,35 @@ respective line in the log output. Hopefully, the author or reviewer will give f
 PR fixed or confirm that it's not at fault. The failed rollup PR should then be closed.
 
 Once you've removed the offending PR, recreate your rollup without it (see 1.).
-Merge one batch of PRs by throwing out the failures one by one instead of adding new PRs to it, as this may introduce additional points of failure.
+Merge one batch of PRs by throwing out the failures one by one instead of adding new PRs to it, as this may introduce additional points of failure,
+as don't want to block the queue forever until we have identified every single failing pr in it.
 
 Sometimes it is hard to find the offending PR. There are usually three strategies to figure out the
 offending PR:
 
 1. **Avoid the problem**: Use your intuition to avoid the PRs that you suspect are the problem and
-   recreate the rollup.
+   recreate the rollup without them
 2. **Test suspected PRs alone**: Raise the priority of the PRs you suspect, mark them as
    `rollup=never` (or `iffy`) and let bors test them standalone to dismiss or confirm your
-   hypothesis.
+   hypothesis. Or even better, run a `try job` on the pr to confirm it is cause of the failure.
+   Note: you will have to unapprove the pr before running a try job, do not forget to re-approve if the pr turns out to not be the cause.
 3. **Divide and conquer**: Split rollup into 2 smaller ones until you are certain of the failure
    cause. If a PR was found to be the cause and other PRs were "wrongfully" marked `iffy`, they can
    of course be marked as `maybe` again with `@bors rollup=maybe` or `@bors rollup-`.
 
 If a PR in a rollup continues to fail you can run the `@bors rollup=never` command to ensure the PR
 gets tested independently, since it's likely it will fail again in the future.
 
+Make sure you do not forget to triage your failed rollups, don't just close them!
+Otherwise someone else might roll up the same failing pr again which is just a waste of time.
+Likewise, its often a good idea to quickly check the latest closed rollup to make sure it was triaged properly.
+
+If you don't have time to triage your rollups after failure, consider leaving rollup creation
+to someone that will have time to do the triage later on.
+
 
 [Homu queue]: https://bors.rust-lang.org/queue/rust
 [the Rollups section]: ../compiler/reviews.md#rollups
 [here]: https://github.com/rust-lang/rust/pull/132434#issue-2628063878
 [bisectable]: https://rust-lang.github.io/cargo-bisect-rustc/
+[label]: https://github.com/rust-lang/rust/labels?q=CI-spurious