Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1703353002:
Attempt to make GN waterfall bots not on the CQ alert but not close trees. (Closed)
|
Created:
4 years, 10 months ago by Dirk Pranke Modified:
4 years, 9 months ago CC:
chromium-reviews, infra-reviews+build_chromium.org, kjellander-cc_chromium.org Target Ref:
refs/heads/master Project:
build Visibility:
Public. |
DescriptionAttempt to make GN waterfall bots not on the CQ alert but not close trees.
I think that if we make all of the steps on the GN bots non-closing,
that will keep them from closing the tree, but the events will
still propagate through to sheriff-o-matic.
R=stip@chromium.org, seanmccullough@chromium.org
BUG=497970
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299395
Patch Set 1 : patch for review #
Total comments: 1
Patch Set 2 : rework approach to use closing_optional instead #Patch Set 3 : go back to patchset #1 #Messages
Total messages: 23 (9 generated)
Description was changed from ========== Attempt to make GN waterfall bots alert but not close trees. If I'm reading the gatekeeper_extras.py code in builder_alerts properly, it looks like removing the GN bots from the "excluded_builders" section but adding "would_close_tree": false per builder should have the effect that failures will not close the tree, but will still propagate through to sheriff-o-matic. R=stip@chromium.org, seanmccullough@chromium.org BUG=497970 ========== to ========== Attempt to make GN waterfall bots alert but not close trees. If I'm reading the gatekeeper_extras.py code in builder_alerts properly, it looks like removing the GN bots from the "excluded_builders" section but adding "close_tree": false per builder should have the effect that failures will not close the tree, but will still propagate through to sheriff-o-matic. R=stip@chromium.org, seanmccullough@chromium.org BUG=497970 ==========
Patchset #1 (id:1) has been deleted
Please take a look? The code I'm referring to is https://code.google.com/p/chromium/codesearch#chromium/infra/infra/services/b... (though the comment there might actually already be false, as there is a "close_tree" setting in the infra.cron master section.
seanmccullough@google.com changed reviewers: + seanmccullough@google.com
I'm not that familiar with how gatekeeper works, but the code looks like *builder_alerts* will interpret this json change the way you think it will. However, is this the same code that gatekeeper recipe uses to determine if should actually close the tree? I don't actually know.
On 2016/02/18 01:25:06, seanmccullough1 wrote: > I'm not that familiar with how gatekeeper works, but the code looks like > *builder_alerts* will interpret this json change the way you think it will. > However, is this the same code that gatekeeper recipe uses to determine if > should actually close the tree? I don't actually know. Good question. I'm not sure, which is also why I copied stip on this :).
Pinging stip ... I know you're mostly-OOO but if you felt like weighing in on this that would be helpful.
great care was made to synchronize builder_alerts and the tree. I assume this is for onboarding purposes? https://codereview.chromium.org/1703353002/diff/20001/scripts/slave/gatekeepe... File scripts/slave/gatekeeper.json (right): https://codereview.chromium.org/1703353002/diff/20001/scripts/slave/gatekeepe... scripts/slave/gatekeeper.json:279: "close_tree": false IIUC this feature isn't particularly used and may have suffered bitrot. regardless, the codepaths seem to respect it
whoops, lgtm
On 2016/02/26 01:01:52, stip (damaged and oooish) wrote: > great care was made to synchronize builder_alerts and the tree. I assume this is > for onboarding purposes? I'm not sure I understand the question, but I'll guess it's some variant of "why do we want alerts but not to close the tree" ... The answer is, you're right, we do normally want that, but in this case we don't yet have the resources to put these bots in the CQ, and we don't want bots that aren't in the CQ to close the tree, but we do want sheriffs looking at the failures, once tree-closing failures are in. Given that we want sheriff-o-matic to be a one-stop-shop, that means that it needs to see the alerts if I understand the system properly. Hence this change. Does that make sense?
The CQ bit was checked by dpranke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1703353002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1703353002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build Presubmit on tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Build%20Presubmit/build...)
Description was changed from ========== Attempt to make GN waterfall bots alert but not close trees. If I'm reading the gatekeeper_extras.py code in builder_alerts properly, it looks like removing the GN bots from the "excluded_builders" section but adding "close_tree": false per builder should have the effect that failures will not close the tree, but will still propagate through to sheriff-o-matic. R=stip@chromium.org, seanmccullough@chromium.org BUG=497970 ========== to ========== Attempt to make GN waterfall bots not on the CQ alert but not close trees. I think that if we make all of the steps on the GN bots non-closing, that will keep them from closing the tree, but the events will still propagate through to sheriff-o-matic. R=stip@chromium.org, seanmccullough@chromium.org BUG=497970 ==========
Okay, let's try this again. The validator code didn't like 'close_tree: false' per-builder, and it looks like maybe `closing_optional: []` should have the same effect (and at least passes the tests). Please take another look?
having closing_optional: [] turns off alerting altogether, which is not what you want IIUC. I have a CL to fix the close_tree processing: https://codereview.chromium.org/1789693003. Once that lands, we should go back to the original CL
On 2016/03/11 23:08:36, stip wrote: > having closing_optional: [] turns off alerting altogether, which is not what you > want IIUC. Hm. I got the idea for closing_optional: [] from the 'iOS Simulator (dbg)' configuration on line 338. Maybe that line isn't doing what the comment says it is doing? From what I can tell, that bug was fixed and it's out of date either way ... > I have a CL to fix the close_tree processing: > https://codereview.chromium.org/1789693003. Once that lands, we should go back > to the original CL Ack.
The CQ bit was checked by dpranke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stip@chromium.org Link to the patchset: https://codereview.chromium.org/1703353002/#ps60001 (title: "go back to patchset #1")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1703353002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1703353002/60001
Message was sent while issue was closed.
Description was changed from ========== Attempt to make GN waterfall bots not on the CQ alert but not close trees. I think that if we make all of the steps on the GN bots non-closing, that will keep them from closing the tree, but the events will still propagate through to sheriff-o-matic. R=stip@chromium.org, seanmccullough@chromium.org BUG=497970 ========== to ========== Attempt to make GN waterfall bots not on the CQ alert but not close trees. I think that if we make all of the steps on the GN bots non-closing, that will keep them from closing the tree, but the events will still propagate through to sheriff-o-matic. R=stip@chromium.org, seanmccullough@chromium.org BUG=497970 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299395 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=299395 |
