<div dir="ltr"><div dir="ltr">The folks on the Infrastructure Working Group: <a href="https://foundation.llvm.org/docs/infrastructure-wg/">https://foundation.llvm.org/docs/infrastructure-wg/</a> might have some context on this (I think Christian Kuhnel is part of that & he's on this list).<br><br>On Fri, Sep 10, 2021 at 11:53 AM Philip Reames via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br></div><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div>
<p><br>
</p>
<div>On 9/10/21 11:36 AM, Mehdi AMINI wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr"><br>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Thu, Sep 9, 2021 at 3:18
PM Philip Reames via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div>
<p>I've been noticing a trend where there is more and more
false positive email notifications sent out on valid
commits. This is getting really problematic as real
signal is being lost in the noise. I've had several
cases in the last few weeks where I did not see a "real"
failure notice because it was buried in a bunch of false
positives. </p>
<p>Let me run through a few sources of what I consider
false positives, and suggest a couple things we could do
to clean these up. Note that the recommendations here
are entirely independent and we can adopt any subset.</p>
<p><b>Slow Try Bots</b></p>
<p>ex: "This revision was landed with ongoing or failed
builds." on <a href="https://reviews.llvm.org/D109091" target="_blank">https://reviews.llvm.org/D109091</a></p>
<p>Someone - I'm not really sure who - enabled builds for
all reviews, and this notice on landed commits. Given
it's utterly routine to make a last few style fixes
before landing an LGTMed change</p>
</div>
</blockquote>
<div><br>
</div>
<div>I do such "few style fixes", but I don't re-upload a
revision before landing, so I don't see this "false
positive" in general.</div>
</div>
</div>
</blockquote>
I don't explicit upload the final patch either, but something in the
close automation does. <br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div>What I frequently see is that the pre-merge config is
broken for some other reason, and that's quite annoying. One
aspect of the issue is that the is no buildbot tracking the
pre-merge configuration so it can be broken without
notification (there is a buildkite job tracking it, but
buildkite does not support blamelist notifications).</div>
</div>
</div>
</blockquote>
<p>Hm, maybe I misinterpreted the cause of these entirely? Your
explanation sounds plausible as well.</p>
<p>If your explanation is correct, that would lean strongly to the
"just disable" option. <br>
</p>
<p>Do you know who to contact about this? (i.e. Who owns the
automation here? Or where is the appropriate code to adjust?) </p></div></blockquote><div>Looks like: <a href="https://reviews.llvm.org/harbormaster/plan/5/">https://reviews.llvm.org/harbormaster/plan/5/</a> indicates it was setup by <a href="https://reviews.llvm.org/p/goncharov/">https://reviews.llvm.org/p/goncharov/</a> (added them to the "to" line on this email) Also looks like Kuhnel ( <a href="https://reviews.llvm.org/H576">https://reviews.llvm.org/H576</a> ) has sometthing to do with it, so I've added them too.<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div>
<p>, I consider this notice complete noise. In practice,
almost review gets tagged this way. To be clear, there
is value in being told about changes which don't build.
The false positive part is only around the "ongoing"
builds.</p>
<p>Recommendation: Disable this message for the "ongoing"
build case, and if we can't, disable them entirely. <br>
</p>
<p><b>Flaky Builders</b></p>
<p>ex: <a href="https://lab.llvm.org/buildbot/#/builders/68/builds/18250" target="_blank">https://lab.llvm.org/buildbot/#/builders/68/builds/18250</a></p>
<p>We have many build bots which are not entirely stable.
It's gotten to the point where I *expect* failure
notifications on literally every change I land. I've
been trying to reach out to individual build bot owners
to get issues resolved, and to their credit, most owners
have been very responsive. However, we have enough
builders that the situation isn't getting meaningful
better.</p>
<p>Recommendation: Introduce specific "test commits" whose
only purpose is to run the CI infrastructure. Any
builder which notifies of failure on such a commit (and
only said commit) is disabled without discussion until
human action is taken by the bot owner to re-enable.
The idea here is to a) automate the process, and b)
shift the responsibility of action to the bot owner for
any flaky bot. <br>
</p>
<p>Note: By "disabled", I specifically mean that
*notification* is disabled. Leaving it in the waterfall
view is fine, as long as we're not sending out email
about it. <br>
</p>
<p>Aside: It's really tempting to attempt to separate
builders which are "still failing" (e.g. a rare
configuration which has been broken for a few days) from
"flaky" ones. I'd argue any bot notifying on a "still
failing" case is buggy, and thus it's fine to treat them
the same as a "flaky" bot. <br>
</p>
<p><b>Slow Builders and Redundant Notices<br>
</b></p>
<p>ex: <a href="https://lab.llvm.org/buildbot#builders/67/builds/4128" target="_blank">https://lab.llvm.org/buildbot#builders/67/builds/4128</a></p>
<p>Occasionally, we have a bad commit land which breaks
every (or nearly every) builder. That happens. If you
happen to land a change just before or after it, you
then get on the blame list for every slow running
builder we have (since they tend to have large commit
windows) if they happen to cycle before the fix is
committed. This is particularly annoying since the root
issue is likely fixed quickly, but due to cycle times on
the builders, you may be getting emails for 24 hours to
come. <br>
</p>
<p>Recommendation: Introduce a new requirement for "slow"
builders (say cycle time of > 30 minutes) either a)
have a maximum commit window of ~15 commits, or b) use a
staged builder model. Personally, I'd prefer the staged
model, but the max commit window at least helps to limit
the damage. <br>
</p>
<p>By "staged builder model", I mean that slow builders
only build points in the history which have already been
successfully build by one of the fast builders. This
eliminates redundant build failures, at the cost of
delaying the slow builder slightly. As long as the slow
builder uses the "last good commit" as opposed to
waiting until the current fast builder finishes, the
delay should be very minimal for most commits. <br>
</p>
</div>
</blockquote>
<div><br>
</div>
<div>Does buildbot support staged builders? That would really
be ideal indeed!</div>
<div>If we could also disable notification to the blamelist
when it is larger than 5, that'd be great!</div>
</div>
</div>
</blockquote>
<p>I'll be honest here and say I don't know what buildbot natively
supports. Even if it doesn't, there are "easy" process
workarounds to achieve the same effect. Just as an example (i.e.
definitely not proposing this as technical solution to be
implemented right now), we could introduce a new branch in git
called e.g. "buildbot-tracking-slow" and have a specific fast
builder do a fast forward merge from main into this branch. All
"slow" builders would simply follow this branch and not main. <br>
</p>
<p>If we get consensus that this is the right approach, I am willing
to put some of my own time to figuring out how to implement this.
For my own volunteer time, I'd probably start with the flaky bot
test commit piece just because that's much easier to do manually
first and then automate, and because I find them personally more
annoying. </p></div></blockquote><div>There was some prototype buildbot based (so far as I can tell) staged builder setup years ago, it seems: <a href="https://marc.info/?l=cfe-dev&m=136442525121902&w=2">https://marc.info/?l=cfe-dev&m=136442525121902&w=2</a> - perhaps there's some commit history in the zorg repo that shows how it was configured. <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>
<p>Your point about disabling notification on a blamelist larger
than 5 seems reasonable to me, but I'd definitely consider "build
chunks of no more than N commits" and "build arbitrary sets, but
only notify if less than M people" as distinct possibilities to be
evaluated independently. </p></div></blockquote><div>"chunks of no more than N commits" risks slower builders getting behind - presumably you'd need some catchup mechanism (run a build with all the available changes, but don't notify) if they just kept falling further and further behind.<br><br>- Dave</div></div></div>
</div>