New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 807846 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Builder page for luci CI bots is missing changes count

Project Member Reported by bpastene@chromium.org, Feb 1 2018

Issue description

Components: -Infra>Platform>Milo Infra>Platform>Milo>LUCI
This is blamelists not being exposed in builder view.

Comment 2 by no...@chromium.org, Feb 1 2018

Cc: iannucci@chromium.org
+iannucci, the blamelist lord
Cc: -iannucci@chromium.org
Owner: iannucci@chromium.org
Status: Assigned (was: Untriaged)
So I thought about this a bit more. This is tricky because buildbot and LUCI deal with blamelists differently.

Buildbot:
  * poll repo
  * gather changes
  * associate changes into a changelist
  * fire a build with a changelist
  * use associated changelist to show console/blamelist/'changes' column

LUCI
  * poll repo
  * fire a build with a commit
  * when displaying commit-oriented views (like console), query git for a chunk of CLs, then find all builds matching those CLs.

The key difference is that LUCI allows bisection/backfilling due to this design; adding a new build with an old commit will simply start displaying 'in between' the two commits that it bisects. In buildbot this is not possible (since the changes have been 'used up' by the previously fired builds). 

In addition force/rebuilds on buildbot will have NO associated changes and NO associated blamelist. On LUCI, they will all have the same blamelist (since lists of commits are dynamically associated with the build instead of statically).

That leads us to the builder view. The builder view shows builds ordered by TIME (not by commit). While it would potentially be possible to add changes into this view, it would be tricky to do. IMO, we should remove the changes column from the LUCI builder view, since this view is time oriented and not commit oriented.

If this feature is really, truly needed (i.e. it helps people do some workflow), then I can definitely implement it.. if it's just for visual parity with buildbot, then I'm inclined to remove the column.

What do folks think?
Speaking as someone that has no business knowing/caring about the implementation, the changes column is nice to have. At the time of filing, I think I was trying to answer the question "what kind of effect does length-of-blamelist have on build duration?"

Having that column filled out made it trivial. Additionally, it can give you a good impression on roughly how many changes a CI bot picks up every cycle. If I were sheriffing/monitoring a waterfall, that would be nice info to have.

That said, if the solution is too difficult, feel free to wontfix it. (Just know that buildbot will have won over luci in at least one aspect :P )
ah ha! that is the great lie :D

That sort of determination only works if the pool has /exactly/ one build, AND there are never any force builds/retries.

Comment 7 by hinoka@chromium.org, Feb 10 2018

That happens to be how our luci builder are configured right at the moment.  The cost will come when we want to move beyond that.

Comment 8 by hinoka@chromium.org, Feb 10 2018

More precisely, the cost will be realized when we want to move beyond that*
So I think a different (potentially better) way to do this sort of analysis is to have the bot report (as a source manifest? :)) the state of its checkout BEFORE it does the bot_update step. Then you can actually get a datapoint which is `<commitA>..<commitB> && compile takes XX minutes`.

I'm going to remove the column for now.

Comment 10 by no...@chromium.org, Feb 10 2018

+1 to c#4 and removing column

Ben, doesn’t console view solved your problem? It is the view ordered by commit.

We could potentially add a radio button for order (time or commit) to the builder view. If commit, then do what console does, essentially. 
Project Member

Comment 11 by bugdroid1@chromium.org, Feb 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/ba2998b2e20f8634ee551aaac497de51eec1d6ff

commit ba2998b2e20f8634ee551aaac497de51eec1d6ff
Author: Robert Iannucci <iannucci@chromium.org>
Date: Mon Feb 12 19:29:49 2018

[milo] Remove blank Changes column from buildbucket builder pages.

We can add it back if we determine that there's a need for this (and
a meaningful implementation).

In Buildbot, changes are directly associated with Build objects. Retries
and forced builds have no 'changes' associated with them. In
Buildbucket, builds record the fact of "I built revision X", and then
milo dynamically computes the blamelist between a build and the previous
build for that builder. This is done in order to support
backfilling/bisection on main waterfall builders at a later time.
However, due to this design, meaningful display of a changelist on this
view is more difficult than it is for buildbot.

Coupled with the fact that the primary use case for this information is
also less relevant than it was for Buildbot (namely; the implication
that the changelist here correlates with the incremental build that the
bot did. On Swarming, there can be multiple bots in the pool, the cached
build can be removed between tasks, force/retry builds won't be in
commit order, etc.)

Diagnosing this sort of thing on the bot (speed of incremental builds)
will be better served by reporting the 'currently checked out version'
from the bot before the build. Then we'll know the actual starting
revision, the checked out revision, and the duration of the build.

R=hinoka@chromium.org, jchinlee@chromium.org

Bug:  807846 
Change-Id: I04727eb8487653a01deadd3e1b863082a2b863ef
Reviewed-on: https://chromium-review.googlesource.com/912564
Reviewed-by: Jao-ke Chin-Lee <jchinlee@chromium.org>
Commit-Queue: Robbie Iannucci <iannucci@chromium.org>

[modify] https://crrev.com/ba2998b2e20f8634ee551aaac497de51eec1d6ff/milo/buildsource/buildbucket/expectations/master.tryserver.infra/InfraPresubmit.Swarming.json
[modify] https://crrev.com/ba2998b2e20f8634ee551aaac497de51eec1d6ff/milo/buildsource/builder.go
[modify] https://crrev.com/ba2998b2e20f8634ee551aaac497de51eec1d6ff/milo/frontend/appengine/templates/pages/builder.html
[modify] https://crrev.com/ba2998b2e20f8634ee551aaac497de51eec1d6ff/milo/frontend/expectations/buildbot.builder-Basic_Test_no_builds.html
[modify] https://crrev.com/ba2998b2e20f8634ee551aaac497de51eec1d6ff/milo/frontend/expectations/buildbot.builder-Basic_Test_with_builds.html
[modify] https://crrev.com/ba2998b2e20f8634ee551aaac497de51eec1d6ff/milo/frontend/routes_test.go
[modify] https://crrev.com/ba2998b2e20f8634ee551aaac497de51eec1d6ff/milo/frontend/ui/builder.go

Status: Fixed (was: Assigned)
Fixed for now.

When we're fully migrated (or at least more-migrated), it would be great to sit down and make a proper requirements list for our UI so we can design it to match user workflows.

e.g. if we had monitoring for #commits v. incremental build time, I think that would be a better way to measure this effect than relying on the UI.

Comment 13 by efoo@chromium.org, Feb 28 2018

Labels: LUCI-Chromium-CQSets LUCI-KnownIssues-UI

Sign in to add a comment