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

Issue 813196 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocked on:
issue 706095

Blocking:
issue 782863
issue 826731
issue 835932



Sign in to add a comment

Remove gerrit-buildbucket-config subcommand from mb

Project Member Reported by dpranke@chromium.org, Feb 16 2018

Issue description

The preferred way to update buildbucket.config on the meta/config branch (which controls what builders are available in Gerrit for try jobs) is to generate the file from the mb_config.pyl file.

However, that file doesn't know about LUCI buckets (since the recipes still use mastername and buildername), and so if we move a builder from buildbot to LUCI, mb will try to move it back.

For now, we can hand-edit the resulting file to work around this, but we'll need to fix this ASAP.
 
Is there anyone who knows the systems involved and would be a good owner for this bug?
I'm struggling to triage it, since it's a P1 that needs an owner, and I don't know of any good one...

resulting config file is trivial [1], biggest knowledge requirement is for mb, with which i'm not familiar and I doubt anyone on foundation team is.

My guess is the only thing to do is adding another field to annotate each builder which mb knows about to specify that it's on LUCI. Then, for each trybot on LUCI, mb should emit 'luci.chromium.try' bucket.

[1] https://chromium.googlesource.com/chromium/src/+/refs/meta/config/buildbucket.config
Owner: dpranke@chromium.org
Status: Assigned (was: Untriaged)
I'm a good owner for this, since it's my code ;)

I think tandrii@ is right and we'll either need to do something like that or figure out how to parse //infra/config/cq.cfg and extract the information out of that.

Do we have the generated python stubs for that proto somewhere that I can check into src (or is it already checked in somewhere)?
> Do we have the generated python stubs for that proto somewhere that I can check into src (or is it already checked in somewhere)?

turns out we do, but it's outdated and probably should be removed from there anyway: https://chromium.googlesource.com/chromium/tools/depot_tools.git/+/master/third_party/cq_client/

maintain it is a pain, because it requires manual file editing to work around python protobuf imports in google namespace :(
Mkay, do we have a vpython package for this? Or, what is the recommended way for me to read this proto from python? 
No, there is no vpython package.

But, I don't see how parsing cq.cfg is sufficient to cover all try builders, not all which are in cq.cfg, but all of which should be in gerrit UI.
The LUCI builders should all be in cr-buildbucket.cfg - https://chromium.googlesource.com/chromium/src/+/infra/config/cr-buildbucket.cfg

which is also a protobuf, though in a separate ref, and is served by luci-config.

If the overall goal here is to keep the list of tryjobs in Gerrit in sync, can buildbucket plugin read these builders directly from luci-config? Perhaps, in addition to the current config for buildbot builders (which then wouldn't need the mb change)?
> I don't see how parsing cq.cfg is sufficient to cover all try builders, 
> not all which are in cq.cfg, but all of which should be in gerrit UI.

Ah, good point.

> The LUCI builders should all be in cr-buildbucket.cfg - 
> https://chromium.googlesource.com/chromium/src/+/infra/config/cr-buildbucket.cfg
>
> which is also a protobuf, though in a separate ref, and is served by luci-config.

Being on a separate ref makes that more annoying in this case, all right (in every case, really), though I think tandrii was planning to move this to master?

Also, cr-buildbucket.cfg doesn't list which builders are present in buildbot, so it's not perfect. I'm not sure if that's enough to tell us whether to link to the LUCI builder, or the Buildbot builder, or both.

Modifying the plugin to read from luci config sounds like an interesting idea, but (a) isn't something I'm going to do, that's probably more of a tandrii thing, and (b) is probably substantially more work. 

Owner: no...@chromium.org
I think maybe the right thing to do is:

1) If the bot hasn't been migrated at all, only show the buildbot version
2) If the bot has been 100% migrated, only show the LUCI version
3) Otherwise, show both

And I think in order to do that I probably need to query the migration app for the current state, since the needed information is scattered across too many places.

@nodir - is there some way I can do that?
Re: #c5: I'd say, the recommended way to read protos in Python is through "import protobuf". We don't have special packages for reading specifically buildbucket or CQ protos, but I suppose once you know the schema, reading generically is just as easy.

There is a generic CIPD package for protobuf: infra/python/wheels/protobuf
Example usage: https://chromium.googlesource.com/chromium/src/tools/franky/+/master/test.py.vpython#67

1. Just wrote proposal for moving configs to master branch. I'm ready to execute on it on Friday if nobody finds blockers. (internal doc http://shortn/_d7DritEU3w)
2. luci-config isn't useful as it doesn't answer question which builder has migrated.

Bu drapnke@ #c9 is correct, the right way is to ask luci-migration app. Luckily it's simple urlfetch away:

https://luci-migration.appspot.com/masters/tryserver.chromium.linux/builders/chromium_presubmit/?format=json

(see https://cs.chromium.org/chromium/infra/go/src/infra/appengine/luci-migration/app/handlers.go?l=260)

Comment 12 by no...@chromium.org, Feb 22 2018

the plan in c#9 is what i had in mind, except there is
https://apis-explorer.appspot.com/apis-explorer/?base=https://cr-buildbucket.appspot.com/_ah/api#p/swarmbucket/v1/swarmbucket.get_builders
to return swarmbucket builds in JSON.

migration app currently returns state of individual builders. That would be slow. We will need to update it to return state of all builders of a master in one RPC.

Comment 13 by no...@chromium.org, Feb 22 2018

Status: Started (was: Assigned)

Comment 14 by no...@chromium.org, Feb 22 2018

Blockedon: 706095

Comment 15 by no...@chromium.org, Feb 22 2018

Owner: ----
Status: Available (was: Started)
Today I will work on BQ export in Buildbucket, not this.

Comment 16 by no...@chromium.org, Feb 22 2018

please update buildbucket.config manually in the meantime, or take this bug.
Project Member

Comment 17 by bugdroid1@chromium.org, Feb 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/4099f00969d65e9863dd534e92a8ca92208e93a2

commit 4099f00969d65e9863dd534e92a8ca92208e93a2
Author: Nodir Turakulov <nodir@google.com>
Date: Thu Feb 22 22:29:11 2018

[luci-migration] include builder states in master json response

Bug:  813196 
Change-Id: I5758b54f8e8d52fa78698f75104685322a0b6f5b
Reviewed-on: https://chromium-review.googlesource.com/931684
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/4099f00969d65e9863dd534e92a8ca92208e93a2/go/src/infra/appengine/luci-migration/app/master.go

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

Labels: LUCI-Chromium
Owner: dpranke@chromium.org
Status: Assigned (was: Available)
@nodir - does the change in #17 mean that there's an entry point I should call now?

Alternatively, this is not a performance-sensitive thing, so I don't think we care too much if it's slow, as long as you wouldn't be worried that we'd DOS buildbucket (and I'd assume we wouldn't).
the change in #17 is for buildbucket-gerrit plugin to call.  Overall, I think this bug should be fixed on the gerrit-buildbucket plugin side.
Cc: aga...@chromium.org
If you're suggesting that the gerrit-buildbucket plugin should just call buildbucket to determine the list, I'm all for that, but I have no idea when that might be done.

In the meantime, should I make it possible to edit things by hand, or should I modify mb to call something?

+agable for gerrit plugin as well.
i think doing it manually occasionally is better than you spending time writing code will be deleted in a week or two.
okay, if it'll be that soon, I agree. Thanks!

I'll leave this open until we have a proper fix where this either works correctly or (better) can be removed altogether.
Cc: dpranke@chromium.org
Owner: no...@chromium.org
 Issue 825315  has been merged into this issue.

Comment 26 by no...@chromium.org, Mar 28 2018

Status: Started (was: Assigned)

Comment 27 by no...@chromium.org, Mar 28 2018

Blocking: 826731
Project Member

Comment 28 by bugdroid1@chromium.org, Apr 23 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/232e8e9fc710de19f7a0e81114e4deaf8b297c4d

commit 232e8e9fc710de19f7a0e81114e4deaf8b297c4d
Author: Nodir Turakulov <nodir@google.com>
Date: Mon Apr 23 20:22:00 2018

[luci-migration] add CORS support

Buildbucket-Gerrit plugin needs to query luci_is_prod of builders to decide
whether to show Buildbot or LUCI variant of a builder in "Choose trybots"
dialog. Currently the browser rejects such requests due to CORS.

Add CORS support.

R=jchinlee@chromium.org

Bug:  813196 
Change-Id: Id0ef74a9a853f550c5e89b63ccb8b3cc1e712446
Reviewed-on: https://chromium-review.googlesource.com/1024510
Commit-Queue: Nodir Turakulov <nodir@chromium.org>
Reviewed-by: Jao-ke Chin-Lee <jchinlee@chromium.org>

[modify] https://crrev.com/232e8e9fc710de19f7a0e81114e4deaf8b297c4d/go/src/infra/appengine/luci-migration/app/handlers.go
[modify] https://crrev.com/232e8e9fc710de19f7a0e81114e4deaf8b297c4d/go/src/infra/appengine/luci-migration/app/master.go
[modify] https://crrev.com/232e8e9fc710de19f7a0e81114e4deaf8b297c4d/go/src/infra/appengine/luci-migration/app/builder.go

Blocking: 835932
Blocking: 782863
Project Member

Comment 31 by bugdroid1@chromium.org, Apr 25 2018

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

commit 45f02b90b32b45d97d596688dbc0e422d8fe86c0
Author: Nodir Turakulov <nodir@google.com>
Date: Wed Apr 25 03:17:25 2018

[buildbucket-gerrit] load master migration states

For a given Buildbot bucket, some builders might already be migrated to LUCI,
and some might not yet be. Whether a Buildbot builder has migrated to LUCI changes
dynamically and is available in luci-migration.appspot.com app.
Currently a given builder is shown twice, as Buildbot and LUCI variants.

For all configured Buildbot buckets (those that start with "master.")
use luci-migration API to load migration states of all of its builders.
If a builder has already migrated to LUCI, hide its Buildbot variant, otherwise
hide the LUCI variant.

Bug:  813196 
Change-Id: Ia274f5fd70d526244950c06cf1778b6ff7cb3b4e
Reviewed-on: https://chromium-review.googlesource.com/1024630
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>

[modify] https://crrev.com/45f02b90b32b45d97d596688dbc0e422d8fe86c0/test/cr-buildbucket-client_test.html
[modify] https://crrev.com/45f02b90b32b45d97d596688dbc0e422d8fe86c0/src/main/resources/static/cr-buildbucket-client.js
[modify] https://crrev.com/45f02b90b32b45d97d596688dbc0e422d8fe86c0/test/cr-tryjob-picker_test.html
[modify] https://crrev.com/45f02b90b32b45d97d596688dbc0e422d8fe86c0/src/main/resources/static/cr-tryjob-picker.js

Project Member

Comment 32 by bugdroid1@chromium.org, Apr 26 2018

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

commit c39cdbca1168e9bac33913f0b2f3d470acd52e1f
Author: Nodir Turakulov <nodir@google.com>
Date: Thu Apr 26 07:13:18 2018

[buildbucket-gerrit] simplify _updateBuckets

_updateBuckets combines the logic of computing the final list of builders
and updating the element state.

Simplify _updateBuckets by limiting its responsibility to computing and making
it pure. This simplifies its tests too. Move the responsibility of updating
element state and "canceling" previous operation to _pluginConfigChanged.

Also a minor improvements to comments and filtering out buckets
without builders.

Bug:  813196 
Change-Id: I0f466de59a3d5af4940184ecc8da29131999afcf
Reviewed-on: https://chromium-review.googlesource.com/1027207
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>

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

Owner: dpranke@chromium.org
Status: Assigned (was: Started)
Summary: Remove gerrit-buildbucket-config subcommand from mb (was: `mb gerrit-buildbucket-config` doesn't work correctly for LUCI-migrated builders)
the buildbucket.config file no longer needs to list LUCI builders. 

Dirk, I looked at mb.py trying to remove the subcommand, but I am not sure how deep should we go, e.g. I don't know if we still need luci_tryservers. Could you do this cleanup?
Cc: kbr@chromium.org
> the buildbucket.config file no longer needs to list LUCI builders

+kbr@ who recently documented the (now deprecated?) manual process
Status: Started (was: Assigned)
Project Member

Comment 36 by bugdroid1@chromium.org, May 10 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f8266167ee87cfa2b24c9c8b3379e2d603a45375

commit f8266167ee87cfa2b24c9c8b3379e2d603a45375
Author: Dirk Pranke <dpranke@chromium.org>
Date: Thu May 10 01:47:45 2018

Remove gerrit-buildbucket-config command from MB.

The Gerrit plugin will now automatically populate the list of
tryservers from LUCI, meaning that we no longer need to manually
configure things and don't need to generate a config file from
the MB entries. Accordingly, this CL removes the
`gerrit-buildbucket-config` code from MB.

Bug:  813196 
Change-Id: I1c4ba1099a11822c2c741eb21dc61e4f5a163369
Reviewed-on: https://chromium-review.googlesource.com/1053377
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Reviewed-by: John Budorick <jbudorick@chromium.org>
Commit-Queue: Dirk Pranke <dpranke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557416}
[modify] https://crrev.com/f8266167ee87cfa2b24c9c8b3379e2d603a45375/tools/mb/docs/user_guide.md
[modify] https://crrev.com/f8266167ee87cfa2b24c9c8b3379e2d603a45375/tools/mb/mb.py
[modify] https://crrev.com/f8266167ee87cfa2b24c9c8b3379e2d603a45375/tools/mb/mb_config.pyl
[modify] https://crrev.com/f8266167ee87cfa2b24c9c8b3379e2d603a45375/tools/mb/mb_unittest.py

Comment 37 by kbr@chromium.org, May 10 2018

The associated docs about updating refs/meta/config are step 5 in this section:
https://chromium.googlesource.com/chromium/src/+/master/docs/gpu/gpu_testing_bot_details.md#How-to-add-a-new-manually_triggered-trybot

Could you tell me what needs to be updated there?

Comment 39 by kbr@chromium.org, May 11 2018

Deleted in https://chromium-review.googlesource.com/1054357 . Thanks.

Sign in to add a comment