CQ: assert instead of re-running verifiers

CQ: assert instead of re-running verifiers

This is not a valid state for CQ to be in,
and it makes CQ flood Rietveld with comments.

It does not happen under normal operation,
but has happened at least once. Exception
stack trace should help identify how CQ
went there and prevent it from misbehaving.

BUG=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=232201

[Paweł Hajdan Jr., 2013-10-28 18:15:09]

+Robbie

Just trying to get a review.

[Isaac (away), 2013-10-28 18:16:41]

https://codereview.chromium.org/41583004/diff/1/commit-queue/pending_manager.py
File commit-queue/pending_manager.py (left):

https://codereview.chromium.org/41583004/diff/1/commit-queue/pending_manager....
commit-queue/pending_manager.py:362: 'Re-running verififer %s for issue %s' % (
what happens when you restart CQ and add tests?

[Paweł Hajdan Jr., 2013-10-28 22:57:11]

https://codereview.chromium.org/41583004/diff/1/commit-queue/pending_manager.py
File commit-queue/pending_manager.py (left):

https://codereview.chromium.org/41583004/diff/1/commit-queue/pending_manager....
commit-queue/pending_manager.py:362: 'Re-running verififer %s for issue %s' % (
On 2013/10/28 18:16:41, Isaac wrote:
> what happens when you restart CQ and add tests?

Seems to work. Still, please note you're doing a _code_ review here.

I'm assuming lack of other comments indicates the code is good.

[Isaac (away), 2013-10-29 02:15:35]

There's no syntax errors here, but I'm not sure this is a good change, even if
you couldn't reproduce an error adding a bot.  What was the original code for?

[Paweł Hajdan Jr., 2013-10-30 02:42:58]

On 2013/10/29 02:15:35, Isaac wrote:
> There's no syntax errors here, but I'm not sure this is a good change, even if
> you couldn't reproduce an error adding a bot.  What was the original code for?

Hey Isaac, just a friendly note you could have asked about that earlier.

Please see https://codereview.chromium.org/6823083 - it says "Fixes incremental
verification when the commit queue crashes." and the change is from April 2011.

Currently getting there means things are really broken, as discussed last time
we investigated the problem.

My local CQ has been running fine for the last two weeks, and I performed the
test you asked for.

In the time it took to try to get this 5 line CL reviewed, I actually migrated
Chrome testing infrastructure to the new test launcher. Please help me fix the
CQ. It's really hard to make it more broken.

[Isaac (away), 2013-10-30 02:49:38]

On 2013/10/30 02:42:58, Paweł Hajdan Jr. wrote:
> On 2013/10/29 02:15:35, Isaac wrote:
> > There's no syntax errors here, but I'm not sure this is a good change, even
if
> > you couldn't reproduce an error adding a bot.  What was the original code
for?
> 
> Hey Isaac, just a friendly note you could have asked about that earlier.
> 
> Please see https://codereview.chromium.org/6823083 - it says "Fixes
incremental
> verification when the commit queue crashes." and the change is from April
2011.
> 
> Currently getting there means things are really broken, as discussed last time
> we investigated the problem.
> 
> My local CQ has been running fine for the last two weeks, and I performed the
> test you asked for.
> 
> In the time it took to try to get this 5 line CL reviewed, I actually migrated
> Chrome testing infrastructure to the new test launcher. Please help me fix the
> CQ. It's really hard to make it more broken.

Why not send this to M-A who landed the initial change?  You delay is because:
- your message doesn't explain your testing strategy
- your message doesn't explain why this was originally added, and why it no
longer is needed.

Your latest message does clarify point 1. This is a live CQ change, and as such
merits additional scrutiny that I cannot provide without the above context.

I believe the test launcher rollout was unsafe and should have been canaried.

[Paweł Hajdan Jr., 2013-10-30 16:09:24]

Marc-Antoine, could you take a look at this 5 line CQ change?

This is effectively a revert of https://codereview.chromium.org/6823083 from 2.5
years ago, and currently the condition on which this patch asserts results in CQ
spamming the Rietveld issue with lots of comments.

This was working fine on my local CQ for 2 weeks. If this hits again, we'll have
a stack trace and dump of state to help debugging this. Currently the state is
really lost and we don't have enough data to debug, even based on logs.

Isaac, I had a similar discussion with Mike yesterday and we can also have it
outside of this review. I'll listen very carefully to anyone who describes a
specific and reasonable landing plan for the test launcher change. Please
remember about being specific - it's easy to say "canary" or "staged rollout".
Once we get to specifics I can point out some things - e.g. some procedures I
heard being suggested would not catch the chromium.webrtc issues (btw webrtc
guys have reacted really nicely to that), or require resources which we don't
have (e.g. duplicate every waterfall).

[M-A Ruel, 2013-10-30 17:13:13]

This code path was hit when the CQ crashed and a CL was someone considered new
even if it had verifiers associated to it. The code changed significantly since
then so I'm not sure this is necessary anymore. Anyhow, the assert will catch it
if this were to cause any regression. As such, lgtm.

As for anything outside the scope of this CL, I'm going to ignore it here to
stay focused on the task at end, feel free to start a thread on the relevant
mailing list. I don't have opinion on this.

[Isaac (away), 2013-10-31 20:30:24]

(I'm comfortable w/ whatever M-A thinks is best here, lgtm too)

[commit-bot: I haz the power, 2013-10-31 20:44:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/phajdan.jr@chromium.org/41583004/1

[commit-bot: I haz the power, 2013-10-31 20:45:06]

Change committed as 232201