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

Issue 615244 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

circular dependent cl keep canceling each other out

Project Member Reported by kevcheng@chromium.org, May 26 2016

Issue description

https://chromium-review.googlesource.com/#/c/344933 and https://chromium-review.googlesource.com/#/c/344809

Those two cls depend on each other but unfortunately keep canceling each other out.  Here's a timeline of what happens:

2:53 - both cls are marked CQ+1
2:54 - on 344933: The Pre-Commit Queue failed to submit your change in https://uberchromegw.corp.google.com/i/chromeos/builders/pre-cq-launcher/builds/7020 . CL:344933 depends on CL:344809, which was rejected by the CQ.
3:23 - on 344809: The Pre-Commit Queue failed to apply your change in https://uberchromegw.corp.google.com/i/chromeos/builders/pre-cq-launcher/builds/7020 . CL:344809 depends on CL:344933, which is not marked Commit-Queue>=+1.

Seems like it held stale rejection info on 344809? 
 
Labels: -Pri-3 Pri-1
Owner: akes...@chromium.org
Status: Started (was: Untriaged)
This is a regression. I'll take a look.
Project Member

Comment 2 by bugdroid1@chromium.org, May 26 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/8c1efbf59d5475399d118a683b6b987fdef6ee07

commit 8c1efbf59d5475399d118a683b6b987fdef6ee07
Author: Aviv Keshet <akeshet@chromium.org>
Date: Thu May 26 23:36:01 2016

sync_stages: add some logging to pre-cq launcher

BUG= chromium:615244 
TEST=None

Change-Id: I8a1fe5cfd25b4e2154ed7abdf5cf5599243b398b
Reviewed-on: https://chromium-review.googlesource.com/347698
Reviewed-by: Kevin Cheng <kevcheng@chromium.org>
Tested-by: Aviv Keshet <akeshet@chromium.org>

[modify] https://crrev.com/8c1efbf59d5475399d118a683b6b987fdef6ee07/cbuildbot/stages/sync_stages.py

Note -- https://chromium-review.googlesource.com/#/c/344933 is targeted at a branch, not at master. I wonder if that is the problem or is tickling it (since I'm pretty sure pre-cq works otherwise on closed dependency graphs).
I'm sure that's it.

What can we reasonably do to improve our error messages in this case?
Yeah... sorry, my bad. The official master branch for coreboot changed just a few days ago and I hadn't resynced my manifest yet. That was probably it (but it looks like I'll have to do a bunch of rebasing to confirm, so I won't know until tomorrow). Sorry for the noise.
My theory -- the bad logging is coming from validation_pool:471

"raise dep_change.GetMergeException() or PatchRejected(dep_change)"

in the case of a patch that returns None from GetMergeException (such as that wrong-branch patch that nevertheless had all the right labels/approvals) we assume it must be some generic "was rejected" reason and raise a failure with the "was rejected by the CQ" message. That seems wrong. We need some check up front to first handle patches for the wrong manifest, or to understand why we even got to this line of validation_pool.
We get to that line because of validation_pool:467

elif limit_to is not None and dep_change not in limit_to:

The wrong-branch change is not in limit_to, probably, because of some filtering that takes on wrong branch changes.
The easiest way I can think of to fix this is to just have a better log message emitted by that line (rather than trying to fix it before we even get there).

See https://chromium-review.googlesource.com/#/c/347951/
Project Member

Comment 9 by bugdroid1@chromium.org, May 28 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/a26b17af27ba6ae82adc03b010b8b227a29bd228

commit a26b17af27ba6ae82adc03b010b8b227a29bd228
Author: Aviv Keshet <akeshet@chromium.org>
Date: Fri May 27 17:54:12 2016

validation_pool: clearer logging for ineligible changes

This CL address previously unclear logging message "was rejected by the
CQ" that was given as the failure reason for patches in a variety of
cases that were not really caused by being rejected by the CQ (in
particular: when encountering a wrong-manifest change in the dependency
graph of a change in the cq or pre-cq).

BUG= chromium:615244 
TEST=existing unit tests pass

Change-Id: I82036266ab98f28d75edab72b428a6639471c1c6
Reviewed-on: https://chromium-review.googlesource.com/347951
Commit-Ready: Aviv Keshet <akeshet@chromium.org>
Tested-by: Aviv Keshet <akeshet@chromium.org>
Reviewed-by: Don Garrett <dgarrett@chromium.org>
Reviewed-by: Kevin Cheng <kevcheng@chromium.org>

[modify] https://crrev.com/a26b17af27ba6ae82adc03b010b8b227a29bd228/cbuildbot/validation_pool.py

Looks fixed to me.
Status: Fixed (was: Started)

Sign in to add a comment