New issue
Advanced search Search tips

Issue 845642 link

Starred by 4 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Replace EnsureCQIncludeTrybotsAreAdded PRESUBMIT hook with something more centrally managed

Project Member Reported by dpranke@chromium.org, May 22 2018

Issue description

The 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. 
 

Comment 1 by estaab@chromium.org, May 26 2018

There's a configuration schema that we could use for inspiration here:
http://shortn/_F6Bzz2LZnS

It's ubiquitous internally.
@estaab - thanks, that's helpful.
Friendly ping... any update on this? This is a blocking bug for cit-pm-84. Thanks!
No update. It's somewhere a ways down on my backlog :(.
Cc: tandrii@chromium.org
> 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.
Cc: ehmaldonado@chromium.org ajp@chromium.org
FYI, I'm working on a design doc for this.

Comments welcome :)

https://docs.google.com/document/d/1F1zNY7y-e28UMwkCAmd64dNExdNsWmW6Pasa1SNIQ9c
> 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.
Project Member

Comment 9 by bugdroid1@chromium.org, 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

Project Member

Comment 10 by bugdroid1@chromium.org, 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