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

Issue 800355 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 731553



Sign in to add a comment

get-builders uses stable builder-to-bucket map resulting in git-cl not working for builders migrated to LUCI

Project Member Reported by machenb...@chromium.org, Jan 9 2018

Issue description

Repro:
Upload a CL, e.g.:
https://chromium-review.googlesource.com/c/v8/v8/+/857502

On cmd line:
git cl try -b v8_linux64_rel_ng
Tried jobs on:
Bucket: master.tryserver.v8
  v8_linux64_rel_ng: []

The job appears on CL and stays gray.

Expectation: Either "git cl try" with -b option should default to luci.v8.try or fail, e.g. requiring also -m.

 
Blocking: 747960
Components: Infra>Platform>Milo
Looks like builders-map, which is used by git-cl, has been broken forever and returns stale builder-to-bucket map. This is because it fetches http://chrome-build-extract.appspot.com/get_masters?json=1, which returns 404.
Summary: get-builders uses stable builder-to-bucket map resulting in git-cl not working for builders migrated to LUCI (was: V8: Trybot map for command line usage has still buildbot default)
FWIW, see issue 570733.
Also issue 700552 as another problem with this app. The app and all paths leading to it should die.

Comment 6 by no...@chromium.org, Jan 9 2018

Cc: aga...@chromium.org
Components: -Infra>Platform>Milo Infra>SDK
see also  bug 632835 

basically, use -B flag. builders-map is incorrect and it is not needed given that for the vast majority of project that switched to LUCI, the problem that builders-map was trying to solve, does not exist because there is just one bucket: luci.xxx.try.

perhaps we should define a default -B value, perhaps at the repo level (e.g. git config, or codereview.settings file). +agable for input.
This is a change from the past where developers did not need to specify '-B' flag, i.e. "git cl try -b v8_presubmit" just worked. What makes this worse is that we still run the tryserver instance with fake slaves (to keep logs) and thus git-cl-try successfully schedules things, but they remain forever grey. The plan is to remove the master completely in the end of January, so at least users will start receiving an error. However, I think a PSA should be sent to Chromium and client teams explaining how to trigger bots. Perhaps -B flag should be made mandatory too? WDYT?
If or if not users receive an error doesn't depend on the master, but on the buildbucket. We could remove the bucket now or make it readonly to trigger errors immediately, but still keep the master for the logs.
Good point: https://crrev.com/c/861802
$ git cl try -b v8_presubmit
ERROR: Access denied: User user:sergiyb@chromium.org cannot add builds to bucket master.tryserver.v8

The error message is composed by "ERROR: Access denied:" coming from git_cl.py and the reset of the error is coming from the buildbucket server response. I propose to add another sentence in git_cl.py after the error from buildbucket: "If your builder has been recently migrated to the LUCI, you may need to add -B flag". WDYT?
+1 for better help message, +2 for providing a project default somehow, e.g. in codereview.settings a field BUILDBUCKET_DEFAULT=luci.v8.try or similar? Or generically infer luci.X.try where X=project name (already present in codereview.settings - but not sure if that convention holds everywhere)?
Cc: no...@chromium.org iannucci@chromium.org

Comment 13 by no...@chromium.org, Jan 12 2018

Cc: jbudorick@chromium.org estaab@chromium.org
i don't think project in codereview.settings matches LUCI project. The project in codereview.settings predates LUCI. I think it was used for Rietveld or something. Anyway, I've heard agable has plans to kill codereview.settings

Although the problem seems to be almost solved for v8, this workaround won't help Chromium. Migration of Chromium builders will much longer, so the period where some builders are on Buildbot and some are on LUCI, will be longer than in v8. Requiring users to pass -B means users will have to think whether a builder *currently* is on LUCI or still Buildbot. Also it means they won't be able to run one git-cl-try to schedule builds on LUCI and Buildbot builders. A default bucket won't help either because it would send builds to LUCI unconditionally, even if a builder is not ready for LUCI.

It looks like we need to preserve current git-cl-try behavior. One way to solve this is:
- change migration procedure to check "LUCI is PROD" on migration app not only for CI builders, not try builders too
- change git-cl-try to send a request like
https://luci-migration.appspot.com/masters/tryserver.chromium.linux/builders/linux_chromium_rel_ng?format=json
to decide 1) whether to send a tryjob to LUCI or not 2) which bucket to send the build to.

estab, jbudorick, WDYT?

Comment 14 by no...@chromium.org, Jan 12 2018

s/estab/estaab/

Note that Gerrit's Choose tryjobs dialog window has the same kind of problem, but we have better control there, i.e. we can update https://chromium.googlesource.com/chromium/src/+/refs/meta/config/buildbucket.config
to move a builder to [bucket "luci.chromium.try"] section once it is stable on LUCI. So for a relatively short period between ["LUCI is Prod" is checked for a builder] and [builder is deleted on Buildbot], Gerrit will continue sending builds to Buildbot. This is fine, IMO.
Maybe we just always choose the buildbot builder when both exist since that should always work whereas LUCI won't always work, i.e. when the builder is first set up. I'm not sure that would work though - how does buildbucket know what builders exist for buildbot buckets?

During the transition we could get the full list of builders from milo if needed, but that's a bit hacky.
Always choosing buildbot when both exists may be suboptiomal solution:
 - to check that both exists, we'd need to fix deprecated builders-map app, thus wasting engineering cycles on deprecated services
 - projects may choose to keep old buildbot instances even after all builders are migrated to LUCI to keep old links to builds working, e.g. V8 tryserver is still online and will be online until end of January despite all builder having been migrated to LUCI in the end of December.

Comment 17 by no...@chromium.org, Jan 16 2018

buildbucket server does not know what builders buildbot has. Buildbot-buildbucket integration picks builds up and a builder does not exists, the build is marked as INVALID_BUILD_DEFINITION.
Project Member

Comment 18 by bugdroid1@chromium.org, Jan 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/87570c4c5d8b3ad1fd9a721eee50d745b9230394

commit 87570c4c5d8b3ad1fd9a721eee50d745b9230394
Author: Sergiy Byelozyorov <sergiyb@chromium.org>
Date: Thu Jan 18 13:08:43 2018

Remove buildbucket config from deprecated master

TBR=machenbach@chromium.org

Bug:  800355 
Change-Id: I62ddbe155583641cee31a267b53a914a7491e7c5
Reviewed-on: https://chromium-review.googlesource.com/873637
Commit-Queue: Sergiy Byelozyorov <sergiyb@chromium.org>
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Reviewed-by: Sergiy Byelozyorov <sergiyb@chromium.org>

[modify] https://crrev.com/87570c4c5d8b3ad1fd9a721eee50d745b9230394/masters/master.tryserver.v8/master_site_config.py

Cc: -serg...@chromium.org
Components: -Infra>Client>V8
This is not so relevant to V8 anymore, but I'm leaving the bug open since Nodir suggested in #13 that it may be still relevant for Chromium.
Blocking: -747960

Comment 21 by no...@chromium.org, Jan 25 2018

Owner: no...@chromium.org
Status: Assigned (was: Untriaged)

Comment 22 by no...@chromium.org, Jan 25 2018

latest plan:

1) update builders-map.appspot.com to return bucket, not master. Update git-cl-try to accept a bucket and stop converting master->bucket
2) update builders-map.appspot.com to ask luci-migration.appspot.com if a builder was migrated to LUCI. If so, return the LUCI bucket instead.

Then the tech debt is added to the deprecated builders-map.appspot.com, as opposed to git-cl-try
+1 for plan in #22.
Project Member

Comment 24 by bugdroid1@chromium.org, Jan 25 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/44e01ff3063f17d204c574bf623da4c561d4e69a

commit 44e01ff3063f17d204c574bf623da4c561d4e69a
Author: Nodir Turakulov <nodir@google.com>
Date: Thu Jan 25 19:19:03 2018

[git-cl-try] accept buckets from builders-map app

Context:
https://bugs.chromium.org/p/chromium/issues/detail?id=800355#c22

Prepare git-cl-try to a new format of builders-map.appspot.com response
where each entry contains buckets, as opposed to masters.

Bug:  800355 
Change-Id: I5a90c6c4860a7e1fca843067c477a272d782cba1
Reviewed-on: https://chromium-review.googlesource.com/885100
Commit-Queue: Nodir Turakulov <nodir@chromium.org>
Reviewed-by: Aaron Gable <agable@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>

[modify] https://crrev.com/44e01ff3063f17d204c574bf623da4c561d4e69a/git_cl.py

Project Member

Comment 25 by bugdroid1@chromium.org, Jan 25 2018

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

commit dcc00b8bb7916d0c527147d247c91207508f3aa4
Author: Nodir Turakulov <nodir@google.com>
Date: Thu Jan 25 19:23:12 2018

[builders-map] return LUCI buckets

Change response format from {builder: [master]} to
{builder: {'buckets': [bucket]}}
Corresponding git-cl-try update:
https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/885100

Check if a (master, builder) pair were flipped to LUCI. If so, return the LUCI
bucket instead.

Bug:  800355 
Change-Id: Ib1f29a7e0e7462c1a564888da43c886843b5811f
Reviewed-on: https://chromium-review.googlesource.com/885464
Reviewed-by: Sergey Berezin <sergeyberezin@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/dcc00b8bb7916d0c527147d247c91207508f3aa4/appengine/experimental/builders_map/builders_map.py

Project Member

Comment 26 by bugdroid1@chromium.org, Jan 25 2018

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

commit 06f13bcb0e636bbd8760a60fd64736e514b5f118
Author: Nodir Turakulov <nodir@google.com>
Date: Thu Jan 25 21:59:32 2018

[luci-migration] expose is_prod on try builders

Now that builders-map consults luci-migration to decide whether a tryjob should
go to LUCI or Buildbot, "LUCI is Prod" makes sense for tryjobs.

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

[modify] https://crrev.com/06f13bcb0e636bbd8760a60fd64736e514b5f118/go/src/infra/appengine/luci-migration/app/templates/pages/master.html
[modify] https://crrev.com/06f13bcb0e636bbd8760a60fd64736e514b5f118/go/src/infra/appengine/luci-migration/app/templates/pages/builder.html
[modify] https://crrev.com/06f13bcb0e636bbd8760a60fd64736e514b5f118/go/src/infra/appengine/luci-migration/app/builder.go

Project Member

Comment 27 by bugdroid1@chromium.org, Jan 29 2018

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

commit e32b9e606aefbb3896a5fdd8b9f0fba10bac433e
Author: Nodir Turakulov <nodir@google.com>
Date: Mon Jan 29 23:31:56 2018

[builders-map] hardcode master list

builders-map app loads the list of master from CBE endpoint, but that endpoint
returns 404. Hardcode the list of masters instead. The list taken from the
current builders-map state.

Bug:  800355 
Change-Id: I6a071973de61162054cc7e0df5aa2ca1ad6b4961
Reviewed-on: https://chromium-review.googlesource.com/891996
Reviewed-by: Sergey Berezin <sergeyberezin@chromium.org>
Commit-Queue: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/e32b9e606aefbb3896a5fdd8b9f0fba10bac433e/appengine/experimental/builders_map/builders_map.py

Nodir, what's the status here?
Blocking: 731553
ping
Labels: LUCI-Backlog
Project Member

Comment 32 by bugdroid1@chromium.org, Feb 21 2018

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

commit d6209f3afcc64786c7fa350935a01dcfa5766a20
Author: Nodir Turakulov <nodir@google.com>
Date: Wed Feb 21 02:27:23 2018

[builders-map] hardcode the map

The app was broken for months and nobody noticed.
Hardcode the current state and repurpose its cron job only to poll migration
app.

The hardcoded map was trimmed:
- non-tryserver masters removed.
- builders that have multiple masters removed.
  git-cl-try didn't support them anyway

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

[modify] https://crrev.com/d6209f3afcc64786c7fa350935a01dcfa5766a20/appengine/experimental/builders_map/builders_map.py
[add] https://crrev.com/d6209f3afcc64786c7fa350935a01dcfa5766a20/appengine/experimental/builders_map/hardcoded.py

Project Member

Comment 33 by bugdroid1@chromium.org, Feb 21 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/b422e687a5b098a3284ffa754bac7f945019663f

commit b422e687a5b098a3284ffa754bac7f945019663f
Author: Nodir Turakulov <nodir@google.com>
Date: Wed Feb 21 06:57:17 2018

[git-cl-try] update builders-map response handling

Remove code path.

Update new code path to expect one bucket, instead of a list.
Related:
https://chromium-review.googlesource.com/c/infra/infra/+/927792

TBR=tandrii@chromium.org
Bug:  800355 
Change-Id: I60fbcf92dc242fc9b448760aaa5399320e257323
Reviewed-on: https://chromium-review.googlesource.com/928053
Commit-Queue: Nodir Turakulov <nodir@chromium.org>
Reviewed-by: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/b422e687a5b098a3284ffa754bac7f945019663f/tests/git_cl_test.py
[modify] https://crrev.com/b422e687a5b098a3284ffa754bac7f945019663f/git_cl.py

Comment 34 by no...@chromium.org, Feb 21 2018

Status: Fixed (was: Assigned)

Sign in to add a comment