get-builders uses stable builder-to-bucket map resulting in git-cl not working for builders migrated to LUCI |
||||||||||||
Issue descriptionRepro: 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.
,
Jan 9 2018
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.
,
Jan 9 2018
,
Jan 9 2018
FWIW, see issue 570733.
,
Jan 9 2018
Also issue 700552 as another problem with this app. The app and all paths leading to it should die.
,
Jan 9 2018
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.
,
Jan 10 2018
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?
,
Jan 10 2018
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.
,
Jan 11 2018
Good point: https://crrev.com/c/861802
,
Jan 11 2018
$ 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?
,
Jan 12 2018
+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)?
,
Jan 12 2018
,
Jan 12 2018
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?
,
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.
,
Jan 12 2018
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.
,
Jan 15 2018
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.
,
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.
,
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
,
Jan 22 2018
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.
,
Jan 22 2018
,
Jan 25 2018
,
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
,
Jan 25 2018
+1 for plan in #22.
,
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
,
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
,
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
,
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
,
Feb 13 2018
Nodir, what's the status here?
,
Feb 13 2018
,
Feb 20 2018
ping
,
Feb 20 2018
,
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
,
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
,
Feb 21 2018
|
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by machenb...@chromium.org
, Jan 9 2018