Fail build if triggering fails

Fail build if triggering fails

Introduce the concept of "fatal error" to AnnotatedCommand. In case of
fatal errors, mark entire build as EXCEPTION and put the details in the
fatal_errors log of the preamble.

If 'critical' is True or absent in a triggering spec and triggering
fails on the master side, it is a fatal error.

R=vadimsh@chromium.org
BUG=609166
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300505

[nodir, 2016-05-04 17:25:56]

PTAL

Tested locally that it actually adds fatal_errors log under the first step with
an contents:
step trigger: could not trigger builds [u'x']: [Failure instance: Traceback:
<class 'master.buildbucket.common.Error'>: In order to trigger builds through
buildbucket, ActiveMaster must be passed to AnnotatorFactory
/Users/nodir/cr/infra/build/scripts/master/chromium_step.py:1056:handleOutputLine
/Users/nodir/cr/infra/build/scripts/common/annotator.py:324:MatchAnnotation
/Users/nodir/cr/infra/build/scripts/master/chromium_step.py:1263:STEP_TRIGGER
/Users/nodir/cr/infra/build/third_party/twisted_10_2/twisted/internet/defer.py:1141:unwindGenerator
--- <exception caught here> ---
/Users/nodir/cr/infra/build/third_party/twisted_10_2/twisted/internet/defer.py:1020:_inlineCallbacks
/Users/nodir/cr/infra/build/scripts/master/chromium_step.py:1375:triggerBuildsViaBuildBucket
]

[nodir, 2016-05-04 17:26:25]

cc related people

[Vadim Sh., 2016-05-04 19:24:44]

lgtm

https://codereview.chromium.org/1949753003/diff/1/scripts/master/chromium_ste...
File scripts/master/chromium_step.py (right):

https://codereview.chromium.org/1949753003/diff/1/scripts/master/chromium_ste...
scripts/master/chromium_step.py:883: # This might happen if the section was
finished with an premature error
nit: "a premature error"

https://codereview.chromium.org/1949753003/diff/1/scripts/master/chromium_ste...
scripts/master/chromium_step.py:1232: self.finishStep(section, builder.FAILURE,
ex)
what if 'section' is already finished?

[Dirk Pranke, 2016-05-04 22:59:10]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org, iannucci@chromium.org, martiniss@chromium.org

[Dirk Pranke, 2016-05-04 22:59:10]

It seems like we should just mark this step as purple; can't we do that with
existing code (since presumably a trigger failure is either an infra failure or
an indication of some misconfigured bot)?

+iannucci, +martiniss

[martiniss, 2016-05-04 23:15:13]

On 2016/05/04 at 22:59:10, dpranke wrote:
> It seems like we should just mark this step as purple; can't we do that with
existing code (since presumably a trigger failure is either an infra failure or
an indication of some misconfigured bot)?
Yes, we can. I think we can't though in this case because only the master knows
if triggering fails, not the recipe.

> 
> +iannucci, +martiniss

[nodir, 2016-05-05 00:03:12]

Dirk: marked purple (by using EXCEPTION instead of FAILURE)

Verified the change with a local master

https://codereview.chromium.org/1949753003/diff/1/scripts/master/chromium_ste...
File scripts/master/chromium_step.py (right):

https://codereview.chromium.org/1949753003/diff/1/scripts/master/chromium_ste...
scripts/master/chromium_step.py:883: # This might happen if the section was
finished with an premature error
On 2016/05/04 19:24:44, Vadim Sh. wrote:
> nit: "a premature error"

Done.

https://codereview.chromium.org/1949753003/diff/1/scripts/master/chromium_ste...
scripts/master/chromium_step.py:1232: self.finishStep(section, builder.FAILURE,
ex)
On 2016/05/04 19:24:44, Vadim Sh. wrote:
> what if 'section' is already finished?

Done

[nodir, 2016-05-05 00:04:16]

Patchset #2 (id:20001) has been deleted

[Dirk Pranke, 2016-05-05 02:16:07]

I don't really know this code, so maybe my questions don't make any sense, but
...

https://codereview.chromium.org/1949753003/diff/40001/scripts/master/chromium...
File scripts/master/chromium_step.py (left):

https://codereview.chromium.org/1949753003/diff/40001/scripts/master/chromium...
scripts/master/chromium_step.py:1251: self.finishStep(self.cursor,
builder.FAILURE, ex)
I'm confused ... why isn't it sufficient to just
s/builder.FAILURE/builder.EXCEPTION/ here and make sure that trigger is a
'can_fail_build' step?

[nodir, 2016-05-05 16:45:32]

https://codereview.chromium.org/1949753003/diff/40001/scripts/master/chromium...
File scripts/master/chromium_step.py (left):

https://codereview.chromium.org/1949753003/diff/40001/scripts/master/chromium...
scripts/master/chromium_step.py:1251: self.finishStep(self.cursor,
builder.FAILURE, ex)
On 2016/05/05 02:16:07, Dirk Pranke wrote:
> I'm confused ... why isn't it sufficient to just
> s/builder.FAILURE/builder.EXCEPTION/ here 

because triggering is an async operation, in particular the "it is not
configured" exception is raised asynchronously . Note handle_exception is passed
to addErrback

> and make sure that trigger is a
> 'can_fail_build' step?

I believe can_fail_build concept was removed from recipe engine in favor of
python exceptions
https://github.com/luci/recipes-py/blob/0eba5ac3953b892d26a5a91decf84730261e1...
+martiniss to verify

[Dirk Pranke, 2016-05-05 16:56:10]

well, I defer to the others here since I don't really know this code ...

https://codereview.chromium.org/1949753003/diff/40001/scripts/master/chromium...
File scripts/master/chromium_step.py (left):

https://codereview.chromium.org/1949753003/diff/40001/scripts/master/chromium...
scripts/master/chromium_step.py:1251: self.finishStep(self.cursor,
builder.FAILURE, ex)
On 2016/05/05 16:45:32, nodir wrote:
> On 2016/05/05 02:16:07, Dirk Pranke wrote:
> > I'm confused ... why isn't it sufficient to just
> > s/builder.FAILURE/builder.EXCEPTION/ here 
> 
> because triggering is an async operation, in particular the "it is not
> configured" exception is raised asynchronously . Note handle_exception is
passed
> to addErrback

Ah, okay.

> 
> > and make sure that trigger is a
> > 'can_fail_build' step?
> 
> I believe can_fail_build concept was removed from recipe engine in favor of
> python exceptions
>
https://github.com/luci/recipes-py/blob/0eba5ac3953b892d26a5a91decf84730261e1...
> +martiniss to verify

Yeah, I was vaguely referring to whatever the equivalent concept was ...

[nodir, 2016-05-05 16:58:20]

https://codereview.chromium.org/1949753003/diff/40001/scripts/master/chromium...
File scripts/master/chromium_step.py (left):

https://codereview.chromium.org/1949753003/diff/40001/scripts/master/chromium...
scripts/master/chromium_step.py:1251: self.finishStep(self.cursor,
builder.FAILURE, ex)
On 2016/05/05 16:45:32, nodir wrote:
> On 2016/05/05 02:16:07, Dirk Pranke wrote:
> > I'm confused ... why isn't it sufficient to just
> > s/builder.FAILURE/builder.EXCEPTION/ here 
> 
> because triggering is an async operation, in particular the "it is not
> configured" exception is raised asynchronously . Note handle_exception is
passed
> to addErrback
> 
> > and make sure that trigger is a
> > 'can_fail_build' step?
> 
> I believe can_fail_build concept was removed from recipe engine in favor of
> python exceptions
>
https://github.com/luci/recipes-py/blob/0eba5ac3953b892d26a5a91decf84730261e1...
> +martiniss to verify

iannucci verified that exceptions are used in recipes to cause a step to fail
entire bulid.

I forgot to say something that may be non-obvious: can_fail_build is a recipe
concept. We never had it on the master side. Now can_fail_build does not exist,
so there is no way to check if step is critical at the time STEP_TRIGGER
annotation is produced.

[iannucci, 2016-05-05 17:00:08]

There is an 'infra_step' concept though which will make a regular api.step
turn purple if it would otherwise be red.

On Thu, May 5, 2016, 09:58 <nodir@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1949753003/diff/40001/scripts/master/chromium...
> File scripts/master/chromium_step.py (left):
>
>
>
https://codereview.chromium.org/1949753003/diff/40001/scripts/master/chromium...
> scripts/master/chromium_step.py:1251: self.finishStep(self.cursor,
> builder.FAILURE, ex)
> On 2016/05/05 16:45:32, nodir wrote:
> > On 2016/05/05 02:16:07, Dirk Pranke wrote:
> > > I'm confused ... why isn't it sufficient to just
> > > s/builder.FAILURE/builder.EXCEPTION/ here
> >
> > because triggering is an async operation, in particular the "it is not
> > configured" exception is raised asynchronously . Note handle_exception
> is passed
> > to addErrback
> >
> > > and make sure that trigger is a
> > > 'can_fail_build' step?
> >
> > I believe can_fail_build concept was removed from recipe engine in
> favor of
> > python exceptions
> >
>
>
https://github.com/luci/recipes-py/blob/0eba5ac3953b892d26a5a91decf84730261e1...
> > +martiniss to verify
>
> iannucci verified that exceptions are used in recipes to cause a step to
> fail entire bulid.
>
> I forgot to say something that may be non-obvious: can_fail_build is a
> recipe concept. We never had it on the master side. Now can_fail_build
> does not exist, so there is no way to check if step is critical at the
> time STEP_TRIGGER annotation is produced.
>
> https://codereview.chromium.org/1949753003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nodir, 2016-05-09 15:39:45]

The CQ bit was checked by nodir@chromium.org

[nodir, 2016-05-09 15:39:46]

The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org
Link to the patchset: https://codereview.chromium.org/1949753003/#ps40001
(title: "addressed comments")

[commit-bot: I haz the power, 2016-05-09 15:39:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1949753003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1949753003/40001

[commit-bot: I haz the power, 2016-05-09 15:44:28]

Description was changed from

==========
Fail build if triggering fails

Introduce the concept of "fatal error" to AnnotatedCommand. In case of
fatal errors, mark entire build as EXCEPTION and put the details in the
fatal_errors log of the preamble.

If 'critical' is True or absent in a triggering spec and triggering
fails on the master side, it is a fatal error.

R=vadimsh@chromium.org
BUG=609166
==========

to

==========
Fail build if triggering fails

Introduce the concept of "fatal error" to AnnotatedCommand. In case of
fatal errors, mark entire build as EXCEPTION and put the details in the
fatal_errors log of the preamble.

If 'critical' is True or absent in a triggering spec and triggering
fails on the master side, it is a fatal error.

R=vadimsh@chromium.org
BUG=609166

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

[commit-bot: I haz the power, 2016-05-09 15:44:29]

Committed patchset #2 (id:40001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=300505