Replace EnsureCQIncludeTrybotsAreAdded PRESUBMIT hook with something more centrally managed |
|||
Issue descriptionThe Cq-Include-Trybots Gerrit footer exists to instruct the CQ service to include additional trybots in a CQ run in addition to the set configured by default in cq.cfg. Currently we have many directories in Chromium that are configured to automatically add certain builders when files in certain directories are modified, via PRESUBMIT.py hooks like https://cs.chromium.org/chromium/src/cc/PRESUBMIT.py?l=297 . This is a problem because these hooks are expected (by developers) to be mandatory, but the configuration for them is scattered across the repository and so Ops people are unaware of them, not generally reviewing changes to them, and as a result often not incorporating the resulting workload into our capacity forecasts or monitoring. It's also hard to actually see the total picture of what is run when (or why) because these configurations are embedded in the presubmit code, rather than being declaratively managed somewhere. We should fix this by replacing this mapping with something centrally managed / OWNED and more declarative, e.g., a single config file in //infra/config mapping directories to builders plus a single presubmit hook in //PRESUBMIT.py.
,
May 30 2018
@estaab - thanks, that's helpful.
,
Aug 29
Friendly ping... any update on this? This is a blocking bug for cit-pm-84. Thanks!
,
Aug 30
No update. It's somewhere a ways down on my backlog :(.
,
Oct 2
> and as a result often not incorporating the resulting workload into our capacity forecasts or monitoring. what do we incorporate into our capacity forecasts? don't we use past runs (BQ) for forecasts, which does not depend on whether a builder is in cq.cfg or the footer. unrelated, is there a concern that CQ does not actually require those builders to run? i.e. it is possible to upload a CL without a footer and get it landed? I am thinking of two options: 1) expand cq.cfg config schema to add optional file name regex. If specified and matches -> require the builder. 2) simpler but slower: require all builders in cq.cfg, but exit early in the build if relevant files are not modified (does analyze step essentially does that?). This requires LUCI to have minimal overhead, i.e. minimize latency between build is created and "analyze" step is run. Both options make Cq-Include-Trybots footer unnecessary. a special file, like BUILDERS, scattered in the tree may be difficult to implement for CQ because it essentially requires either CQ to read them (like in the old days with OWNERS files) or requires presubmit builder to read them and ensure that CL footer has them, essentially communicating with CQ via CL description.
,
Oct 2
,
Oct 2
FYI, I'm working on a design doc for this. Comments welcome :) https://docs.google.com/document/d/1F1zNY7y-e28UMwkCAmd64dNExdNsWmW6Pasa1SNIQ9c
,
Oct 2
> what do we incorporate into our capacity forecasts? don't we use past runs (BQ) > for forecasts, which does not depend on whether a builder is in cq.cfg or > the footer. With buildbot, we would do a mix of per-builder and per-pool forecasting (e.g., do we have enough bots in the linux cq pool to add another builder the size of linux_chromium_rel_ng)? I don't know how jbudorick and team have been doing capacity forecasts if we now have pool-per-builder. Either way, though, if someone causes an "optional" trybot to suddenly become much slower or run on a lot more things, it'd be totally off our radar. > unrelated, is there a concern that CQ does not actually require those builders to run? No, that's not the concern. The concern is that we have poor visibility into when builders *are* added, because the logic is scattered all over tree and changed in files that don't require CCI review. So, yes, something like (1) or (2) would address the problem. The alternative I had in mind was replacing the entries in N different OWNERS files with a single BUILDERS file (that would still function via presubmit), not N BUILDERS files. I think my idea is functionally equivalent to your (1), so (1) would probably be a better way of solving the problem.
,
Nov 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/tools/depot_tools/+/020b59722ab078d6cca25cb767ef7b65dfcc56a4 commit 020b59722ab078d6cca25cb767ef7b65dfcc56a4 Author: Edward Lemur <ehmaldonado@chromium.org> Date: Fri Nov 30 18:19:50 2018 presubmit: Remove EnsureCQIncludeTrybotsAreAdded Bug: 845642 Change-Id: I552b2da4abbac7ca7f6f3961d2b1c5002fb26f47 Reviewed-on: https://chromium-review.googlesource.com/c/1351509 Reviewed-by: Andy Perelson <ajp@chromium.org> Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org> Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org> [modify] https://crrev.com/020b59722ab078d6cca25cb767ef7b65dfcc56a4/presubmit_support.py [modify] https://crrev.com/020b59722ab078d6cca25cb767ef7b65dfcc56a4/tests/presubmit_unittest.py
,
Nov 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8eb3f00fa3d8bfcef6a8e2f94acdadc897ec197b commit 8eb3f00fa3d8bfcef6a8e2f94acdadc897ec197b Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com> Date: Fri Nov 30 20:11:26 2018 Roll src/third_party/depot_tools 7ae5d7a9b12c..020b59722ab0 (1 commits) https://chromium.googlesource.com/chromium/tools/depot_tools.git/+log/7ae5d7a9b12c..020b59722ab0 git log 7ae5d7a9b12c..020b59722ab0 --date=short --no-merges --format='%ad %ae %s' 2018-11-30 ehmaldonado@chromium.org presubmit: Remove EnsureCQIncludeTrybotsAreAdded Created with: gclient setdep -r src/third_party/depot_tools@020b59722ab0 The AutoRoll server is located here: https://autoroll.skia.org/r/depot-tools-chromium-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary. BUG=chromium:845642 TBR=agable@chromium.org Change-Id: I32ac7a57384716d8c246a50a4a62cab231b53b91 Reviewed-on: https://chromium-review.googlesource.com/c/1357344 Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com> Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com> Cr-Commit-Position: refs/heads/master@{#612747} [modify] https://crrev.com/8eb3f00fa3d8bfcef6a8e2f94acdadc897ec197b/DEPS |
|||
►
Sign in to add a comment |
|||
Comment 1 by estaab@chromium.org
, May 26 2018