I was going through the list of bugs that are a blocker for Skia's migration to PG. Should this be a P1? I imagine this might be a blocker for a lot of projects including Chromium.


Also, a lot of the blocked on bugs here do not appear to be related to Gerrit:
issue 366564
 issue 433934 
 issue 475082 
issue 476690
 issue 521375 
 issue 581091 
 issue 582242 
 issue 590104 
 issue 590356 
 issue 598328 
 issue 602351 

I don't know why so many bugs are blocker for this, tbh. The cc bug in issue 366564 is definitely not a blocker for *this*.

*This* bug though is blocked on internal work done by Gerrit team (note: this is a public bug, so i'm not disclosing more here).

(By the way, do you know about the brand spakin' new Monorail feature where you can order the "Blocked on" bugs by importance/order they should be done vs just in numerical order?  It can help make sense of big blocker lists like this.  Just click on "View detail" to see the titles of all the issues, and drag to reorder!)

Haha. Indeed I HAVE, Julie:  https://crbug.com/monorail/1845  :)

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/infradata/config.git/+/293c499bbf9ed3a6d4a667aca85c543ba901e184

commit 293c499bbf9ed3a6d4a667aca85c543ba901e184
Author: Nodir Turakulov <nodir@google.com>
Date: Mon Oct 17 23:27:49 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/gerrit-plugins/buildbucket/+/a5a2b211ec1e93c65a3fa392aa7ccfd3e8b79faa

commit a5a2b211ec1e93c65a3fa392aa7ccfd3e8b79faa
Author: Andrew Bonventre <andybons@chromium.org>
Date: Fri Oct 14 22:04:40 2016

Pass the correct context and remove the OAuth guard

+ Instead of passing `this`, it should have been passing `self`.
+ Since OAuth is now supported, remove the guard to fetch the
  right tokens.
+ Some more careful guards in error cases.

BUG= 618023 

Change-Id: I84dba781c7a6b58918486912897b5bc3945a4c8a

[modify] https://crrev.com/a5a2b211ec1e93c65a3fa392aa7ccfd3e8b79faa/src/main/resources/static/buildbucket.js
[modify] https://crrev.com/a5a2b211ec1e93c65a3fa392aa7ccfd3e8b79faa/src/main/resources/static/cr-buildbucket-view.js

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/gerrit-plugins/buildbucket/+/557bec865898982124aec2768675ca5da7f22d90

commit 557bec865898982124aec2768675ca5da7f22d90
Author: Andrew Bonventre <andybons@chromium.org>
Date: Thu Oct 20 22:57:30 2016

Replace capability-based restriction with logged-in check

+ Instead of needing to grant all users of the plugin “list”
  capability, just check to see whether the user is logged in
  before returning a list of buckets. If they are not logged in,
  an empty bucket list is returned.

BUG= 618023 

Change-Id: If35e5af5aef2cee57c437ac8fd330ef950993074

[modify] https://crrev.com/557bec865898982124aec2768675ca5da7f22d90/src/main/java/com/googlesource/chromium/plugins/buildbucket/ListBuckets.java

Here's how tryjob triggering works in Rietveld currently as far as I understand, skipping the problem of discovering available bucket+builder combinations (I assume PolyGerrit plugin will have a nice configuration somewhere for this).

1. User clicks a bunch of checkboxes on "Choose trybots" popover and clicks "Update" button.
2. This sends HTTP POST to /<issue>/edit_flags?last_patchset=... with POST body containing a list of "<bucket>:<builder>" pairs, e.g. "master.tryserver.chromium.linux:linux_chromium_asan_rel_ng". 
3. This POST is handled here: https://chromium.googlesource.com/infra/infra/+/master/appengine/chromium_rietveld/codereview/views_chromium.py#338 Eventually it calls 'buildbucket.schedule' function passing it a list of (bucket, builder) pairs and ID of the patchset.
4. 'buildbucket.schedule' is here: https://chromium.googlesource.com/infra/infra/+/master/appengine/chromium_rietveld/codereview/buildbucket.py#207
5. It prepares buildbucket builds (derives properties, tags, etc) and posts them all to buildbucket via single put_batch call (impersonating the end user).

The format of properties and tags is what is most important here. Properties later are used by the actual tryjobs (to know what to build), tags are used by Buildbucket plugin to list pending builds. 

The format of properties for Gerrit CLs should match what CQ uses. This one, I believe, but I'm not 100% sure. tandrii@ can confirm: https://chrome-internal.googlesource.com/infra/infra_internal/+/master/commit_queue/pending_manager/gerrit.py#310 

I think the buildbucket plugin is supposed to perform the same logic, but entirely on the client side, using end user's OAuth access token for authentication. 

We do it through server on Rietveld because we have no forwardable end user credential on Rietveld's client side. (GAE cookie is usable only by the app that set it and can't be forwarded).

I'm tandrii@ and I confirm & approve Vadim's comment above.

 Issue skia:5675  has been merged into this issue.

Some UI feedback/requests: please don't match the Rietveld UI *too* closely :).

In particular, in the existing "deprecated UI" layout in rietveld, when you click on the "choose trybots" link, brings up a popup window that displays every master rietveld knows about, and lays out builders in tables with multiple columns that aren't sized properly (so some builder names display as wrapped lines). 

The non-"deprecated UI" layout appears to be better, in that at least all of the builder names are in one long column with no wrapping. However, we still see an odd assortment of masters or buckets (I'm not sure what these are). Ideally we'd not display masters that are totally irrelevant for a given patch.

For example, a chromium patch should not see masters for Fuchsia or BoringSSL.

For bonus points, we'd have some way to indicate which builders are part of the CQ and which are optional, though it might be that we'd be better off handling that by splitting builders between masters better.

"Ideally we'd not display masters that are totally irrelevant for a given patch."
This would be great but note that, thanks to tandrii@'s work, you can run Chromium tryjobs from non-Chromium projects. Eg: Skia patches can run on Chromium tryjobs. This is very useful to dry run risky changes. We can always use 'CQ_INCLUDE_TRYBOTS' but it is a pain to come up with the precise string. Selecting from UI is much simpler.

go/gerrit-buildbucket spec says that each gerrit project configures a list of buckets it is interested in and only those will be displayed in Gerrit's "choose trybots" equivalent

That sounds perfect.

 Issue gerrit:5405  has been merged into this issue.

From Nodir’s review. To be done in a follow-up change:
consider

accumulating errors and then display them all. Note that results are in the same order as "builds" in the request you sent, so you can figure out which builds failed to be added.
for each successfully added build (no error), consider unselecting it in _selectedBuilds (I assume this will lead to the corresponding checkbox to be unchecked)

note that there is a potential problem. Imagine:
1. user checks builders A and B
2. tries to schedule. A succeeded, B fails. client op is not cleared
3. user unchecks everything and then checks A again, meaning they want schedule A for the second time for whatever reason
4. build is deduped because client operation id was not reset in (3)
5. user is confused

this could be solved if we generated client operation id for each bucket:builder individually, stored them inside _selectedBuilds. When a build is scheduled successfully, we simply remove that property in _selectedBuiilds (which implies forgetting the client op id).

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/gerrit-plugins/buildbucket/+/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5

commit 66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5
Author: Andrew Bonventre <andybons@chromium.org>
Date: Wed Feb 15 00:12:43 2017

Add ability to schedule builds

In addition to adding the UI components for scheduling builds,
this change introduces a new endpoint to get the plugin config via
All-Projects/buildbucket.config. These values are used to generate the
buildset tags and for adding certain values to the payload sent to
BuildBucket.

It also updates the README to include config examples.

BUG= 618023 

Change-Id: I8b65767b1bc17c19e1f923230b80271892de1c1b
Reviewed-on: https://chromium-review.googlesource.com/440376
Reviewed-by: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5/src/main/resources/static/buildbucket.js
[delete] https://crrev.com/6675a6a7dae3a36eef690466ffcc4a3953549166/.buckconfig
[modify] https://crrev.com/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5/test/index.html
[add] https://crrev.com/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5/src/main/resources/static/cr-tryjob-picker.js
[add] https://crrev.com/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5/src/main/resources/static/cr-tryjob-picker.html
[add] https://crrev.com/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5/test/cr-tryjob-picker_test.html
[modify] https://crrev.com/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5/src/main/resources/static/cr-buildbucket-view.html
[modify] https://crrev.com/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5/README.md
[modify] https://crrev.com/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5/src/main/java/com/googlesource/chromium/plugins/buildbucket/BuildBucketModule.java
[modify] https://crrev.com/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5/test/cr-buildbucket-view_test.html
[modify] https://crrev.com/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5/test/buildbucket_test.html
[add] https://crrev.com/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5/src/main/java/com/googlesource/chromium/plugins/buildbucket/GetConfig.java
[modify] https://crrev.com/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5/src/main/resources/static/cr-buildbucket-view.js

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/gerrit-plugins/buildbucket/+/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5

commit 66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5
Author: Andrew Bonventre <andybons@chromium.org>
Date: Wed Feb 15 00:12:43 2017

Add ability to schedule builds

In addition to adding the UI components for scheduling builds,
this change introduces a new endpoint to get the plugin config via
All-Projects/buildbucket.config. These values are used to generate the
buildset tags and for adding certain values to the payload sent to
BuildBucket.

It also updates the README to include config examples.

BUG= 618023 

Change-Id: I8b65767b1bc17c19e1f923230b80271892de1c1b
Reviewed-on: https://chromium-review.googlesource.com/440376
Reviewed-by: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5/src/main/resources/static/buildbucket.js
[delete] https://crrev.com/6675a6a7dae3a36eef690466ffcc4a3953549166/.buckconfig
[modify] https://crrev.com/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5/test/index.html
[add] https://crrev.com/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5/src/main/resources/static/cr-tryjob-picker.js
[add] https://crrev.com/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5/src/main/resources/static/cr-tryjob-picker.html
[add] https://crrev.com/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5/test/cr-tryjob-picker_test.html
[modify] https://crrev.com/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5/src/main/resources/static/cr-buildbucket-view.html
[modify] https://crrev.com/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5/README.md
[modify] https://crrev.com/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5/src/main/java/com/googlesource/chromium/plugins/buildbucket/BuildBucketModule.java
[modify] https://crrev.com/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5/test/cr-buildbucket-view_test.html
[modify] https://crrev.com/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5/test/buildbucket_test.html
[add] https://crrev.com/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5/src/main/java/com/googlesource/chromium/plugins/buildbucket/GetConfig.java
[modify] https://crrev.com/66dd9d0ec02cdd9e41c8acf05b2bf549b8ee7aa5/src/main/resources/static/cr-buildbucket-view.js

 Issue 692603  has been merged into this issue.

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/gerrit-plugins/buildbucket/+/1090ccb7607e0074086d7259535197512f2fb887

commit 1090ccb7607e0074086d7259535197512f2fb887
Author: Andrew Bonventre <andybons@chromium.org>
Date: Thu Feb 16 21:41:45 2017

Fix issue where patch_ref was not being set in properties

BUG= 618023 

Change-Id: Ice6e4ed7ad036641bffe7f24b181337ac87aa906
Reviewed-on: https://chromium-review.googlesource.com/444364
Reviewed-by: Aaron Gable <agable@chromium.org>

[modify] https://crrev.com/1090ccb7607e0074086d7259535197512f2fb887/test/cr-tryjob-picker_test.html
[modify] https://crrev.com/1090ccb7607e0074086d7259535197512f2fb887/src/main/resources/static/cr-tryjob-picker.js

Marking this fixed; it's not truly blocked on the other bug, that bug is just about deploying this ability to all repos that want it.

ok - so how do I use the new feature? Is there any documentation on this?

You click the "Choose try jobs" link in the trybots box. If that link isn't there, we add a buildbucket.config file to your refs/meta/config listing the bots that you want to have available.

I have done so here: https://chromium.googlesource.com/v8/v8/+/1a5797acae96293842dd4531d91c2f5ce9905337/buildbucket.config

You can see the result here: https://screenshot.googleplex.com/1cDjhvAze8L

I think #54 indicates a UX problem. I've filed  bug 706032 

How will this work for Skia's bots?
Will we have to explicitly list them all under the "skia.primary" bucket? or is there a way for it to automatically display all bots under the specified bucket?

Yes, you have to list all the bots in the bucket. It can't automatically list all bots in a specified bucket. (Even the bucket doesn't know what bots are valid in it, so there's not an API to query for that.)

We don't simply replicate the CQ config either, because there are often trybots which aren't in the CQ, and there are CQ bots (such as triggered testers) which can't/shouldn't be run as independent trybots.

So IIUC we commit the list to buildbucket.cfg in the root of refs/meta/config?

FTR there are 3 consumers of buildbucket builds: buildbot, LUCI and Skia's internal service owned by borenet.

For buildbot, there is https://chrome-internal.googlesource.com/infradata/hosts/+/master/buildermap.json (aka go/buildermap). Files in this repo are ingested by chrome-infra-botmap.appspot.com that has API. It is easy to add get_builders(bucket) API to the service.

For LUCI, there is https://cr-buildbucket.appspot.com/_ah/api/explorer/#p/swarmbucket/v1/get_builders that returns the builders.

For Skia, I don't know.

borenet, the extension is ".config", not ".cfg"

buildbucket.config, yes. The format is documented in the buildbucket plugin
https://chromium.googlesource.com/infra/gerrit-plugins/buildbucket/+/master/README.md

You can see examples in chromium and v8
https://chromium.googlesource.com/chromium/src/+/25d224c80a7a1d3a2c92869dee0332ad41eb024e/buildbucket.config
https://chromium.googlesource.com/v8/v8/+/1a5797acae96293842dd4531d91c2f5ce9905337/buildbucket.config

@nodir:

go/buildermap doesn't seem to list any buckets. Is mastername overloaded for that purpose?

The swarmbucket API is great but obviously doesn't yet list the vast majority of the builders we care about. It also doesn't give any indication of whether the builder is one that makes sense to be selected (e.g. triggered builders shouldn't be exposed in the "choose try jobs" list).

This is awesome, thanks!

Any chance we can inherit the config from Chromium? E.g. V8 is able to run all trybots Chromium offers as well. For a start we can maintain a list manually, but I guess it'll get out-of-sync fast...

buildbucket configs actually are inherited, but they're inherited the same way everything in /refs/meta/config is: you set the parent project and then you get everything of theirs plus your own modifications. So inheriting from chromium would involve also inheriting chromium's repo ref ACLs, which seems like a not-good idea.

go/buildermap is buildbot-specific. For a buildbot master X, the bucket is "master.{{X}}".

re swarmbucket: yes, only few builders migrated to LUCI as of now, but this is the direction. Yes, it does not indicate which builders should be in the "Choose trybots" window. We can either add the indicative to the builder definition, or expand gerrit's buildbucket config language to indicate "all builders of a bucket". This is a p2-p3 though. FWIW, Rietveld displays all builders of a LUCI bucket https://chromium.googlesource.com/infra/infra/+/971326c1023798960091bd1925c1c92af380f107/appengine/chromium_rietveld/codereview/buildbucket.py#295

Nodir, thanks for all the awesome info! I've filed a bug to track lowering the maintenance burden of buildbucket.config files here: https://bugs.chromium.org/p/chromium/issues/detail?id=706095

Thanks!

First trial failed:
https://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/23447

Looks like it uses the gerrit revision of the change when triggering the trybot. bot_update can't handle it as the cache doesn't contain it (maybe?). IMO this should have the same behavior like cmd line "git cl try".

Could you have a look? Or is there something wrong in V8 configs?

Tracking that in a different bug: https://bugs.chromium.org/p/chromium/issues/detail?id=706135

I added a buildbucket.config for Skia:
https://skia.googlesource.com/skia/+/d8aba39cf74724a51af3c44ec8f05eb2b2860a41%5E%21/#F0

I see a javascript error when trying to filter trybots: https://skia-review.googlesource.com/c/10119/
"TypeError: Cannot read property 'toLowerCase' of undefined"

Did I make a mistake in the buildbucket.config file?

I see the same error for v8: https://chromium-review.googlesource.com/c/461181/

Awesome! This bug was reported about a week ago but I couldn't get it to reproduce. It is now reproducing for me. Tracking separately here: https://bugs.chromium.org/p/chromium/issues/detail?id=705012