[Findit] Adding improved error detection to MonitorTryJobPipeline

[Findit] Adding improved error detection to MonitorTryJobPipeline

BUG=605344, 606038
Committed: https://chromium.googlesource.com/infra/infra/+/4167fabff66f36b317553ea8ee8a2a165891e383

[lijeffrey, 2016-04-25 00:47:13]

Description was changed from

==========
[Findit] Adding improved error detection to MonitorTryJobPipeline

BUG=
==========

to

==========
[Findit] Adding improved error detection to MonitorTryJobPipeline

BUG=605344
==========

[lijeffrey, 2016-04-25 00:47:14]

lijeffrey@chromium.org changed reviewers:
+ chanli@chromium.org, stgao@chromium.org

[lijeffrey, 2016-04-25 00:48:41]

ptal

[chanli, 2016-04-25 20:51:28]

https://codereview.chromium.org/1921493002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/monitor_try_job_pipeline.py (right):

https://codereview.chromium.org/1921493002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/monitor_try_job_pipeline.py:39: def
_GetError(buildbucket_response, buildbucket_error, timed_out):
Will this handle cases like:
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_var...

The tryjob failed but I didn't find any 'error' in buildbucket_response.

We have 'result'='FAILURE' and 'failure_reason'='BUILD_FAILURE' though.

https://codereview.chromium.org/1921493002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/monitor_try_job_pipeline.py:82: }
Will it be better if 
error_dict = {
    'message': message,
    'reason': 'Try job could not be triggered.'
}
?

[lijeffrey, 2016-04-25 21:49:38]

Description was changed from

==========
[Findit] Adding improved error detection to MonitorTryJobPipeline

BUG=605344
==========

to

==========
[Findit] Adding improved error detection to MonitorTryJobPipeline

BUG=605344, 606038
==========

[lijeffrey, 2016-04-25 22:54:16]

https://codereview.chromium.org/1921493002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/monitor_try_job_pipeline.py (right):

https://codereview.chromium.org/1921493002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/monitor_try_job_pipeline.py:39: def
_GetError(buildbucket_response, buildbucket_error, timed_out):
On 2016/04/25 20:51:28, chanli wrote:
> Will this handle cases like:
>
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_var...
> 
> The tryjob failed but I didn't find any 'error' in buildbucket_response.
> 
> We have 'result'='FAILURE' and 'failure_reason'='BUILD_FAILURE' though.

This try job was triggered outside of Findit, but if we find one similar to this
one that Findit encounters then this case will have to be accounted for in a
separate CL

https://codereview.chromium.org/1921493002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/monitor_try_job_pipeline.py:82: }
On 2016/04/25 20:51:28, chanli wrote:
> Will it be better if 
> error_dict = {
>     'message': message,
>     'reason': 'Try job could not be triggered.'
> }
> ?

Reason should be the root cause, not the observed effect. In this case 'Try job
could not be triggered' was the observed effect, not root cause. Unfortunately
from buildbucket the error dict uses 'message' to tell us the root cause, but we
use 'message' as the observed effect. I'll change the variable name to
root_cause to make it less confusing

[chanli, 2016-04-25 23:04:48]

On 2016/04/25 22:54:16, lijeffrey wrote:
>
https://codereview.chromium.org/1921493002/diff/1/appengine/findit/waterfall/...
> File appengine/findit/waterfall/monitor_try_job_pipeline.py (right):
> 
>
https://codereview.chromium.org/1921493002/diff/1/appengine/findit/waterfall/...
> appengine/findit/waterfall/monitor_try_job_pipeline.py:39: def
> _GetError(buildbucket_response, buildbucket_error, timed_out):
> On 2016/04/25 20:51:28, chanli wrote:
> > Will this handle cases like:
> >
>
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_var...
> > 
> > The tryjob failed but I didn't find any 'error' in buildbucket_response.
> > 
> > We have 'result'='FAILURE' and 'failure_reason'='BUILD_FAILURE' though.
> 
> This try job was triggered outside of Findit, but if we find one similar to
this
> one that Findit encounters then this case will have to be accounted for in a
> separate CL
> 
>
https://codereview.chromium.org/1921493002/diff/1/appengine/findit/waterfall/...
> appengine/findit/waterfall/monitor_try_job_pipeline.py:82: }
> On 2016/04/25 20:51:28, chanli wrote:
> > Will it be better if 
> > error_dict = {
> >     'message': message,
> >     'reason': 'Try job could not be triggered.'
> > }
> > ?
> 
> Reason should be the root cause, not the observed effect. In this case 'Try
job
> could not be triggered' was the observed effect, not root cause. Unfortunately
> from buildbucket the error dict uses 'message' to tell us the root cause, but
we
> use 'message' as the observed effect. I'll change the variable name to
> root_cause to make it less confusing

lgtm

[stgao, 2016-04-25 23:44:40]

https://codereview.chromium.org/1921493002/diff/20001/appengine/findit/model/...
File appengine/findit/model/try_job_error.py (right):

https://codereview.chromium.org/1921493002/diff/20001/appengine/findit/model/...
appengine/findit/model/try_job_error.py:5: # Represents possible error codes for
what can go wrong during try jobs.
Could we move this to findit/common/waterfall/ instead?

https://codereview.chromium.org/1921493002/diff/20001/appengine/findit/waterf...
File appengine/findit/waterfall/monitor_try_job_pipeline.py (right):

https://codereview.chromium.org/1921493002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/monitor_try_job_pipeline.py:88: error_code =
try_job_error.TRYBOT_NOT_FOUND
Why we want to handle this case specially?

https://codereview.chromium.org/1921493002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/monitor_try_job_pipeline.py:105:
report.get('metadata', {}).get('infra_failure')):
Why we need to check both?

https://codereview.chromium.org/1921493002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/monitor_try_job_pipeline.py:105:
report.get('metadata', {}).get('infra_failure')):
It is not added to findit/chromium/test.py yet.

[lijeffrey, 2016-04-26 00:50:08]

https://codereview.chromium.org/1921493002/diff/20001/appengine/findit/model/...
File appengine/findit/model/try_job_error.py (right):

https://codereview.chromium.org/1921493002/diff/20001/appengine/findit/model/...
appengine/findit/model/try_job_error.py:5: # Represents possible error codes for
what can go wrong during try jobs.
On 2016/04/25 23:44:39, stgao wrote:
> Could we move this to findit/common/waterfall/ instead?

Done.

https://codereview.chromium.org/1921493002/diff/20001/appengine/findit/waterf...
File appengine/findit/waterfall/monitor_try_job_pipeline.py (right):

https://codereview.chromium.org/1921493002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/monitor_try_job_pipeline.py:88: error_code =
try_job_error.TRYBOT_NOT_FOUND
On 2016/04/25 23:44:39, stgao wrote:
> Why we want to handle this case specially?

This case should be rare and never occur once it's detected and the config
updated. I've renamed it BUILDBUCKET_REPORTED_ERROR for cases where the error is
reported in the result_details_json, since there may be other types of failures
that are reported here.

https://codereview.chromium.org/1921493002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/monitor_try_job_pipeline.py:105:
report.get('metadata', {}).get('infra_failure')):
On 2016/04/25 23:44:39, stgao wrote:
> Why we need to check both?

Done. Good point, I think just metadata should be sufficient which should
account for test.py once the change is made to add it to that recipe.

[stgao, 2016-04-26 17:33:07]

lgtm with nits.

https://codereview.chromium.org/1921493002/diff/40001/appengine/findit/waterf...
File appengine/findit/waterfall/monitor_try_job_pipeline.py (right):

https://codereview.chromium.org/1921493002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/monitor_try_job_pipeline.py:83:
try_job_error.BUILDBUCKET_REPORTED_ERROR)
Maybe rename it to CI_REPORTED_ERROR.

result_details_json is reported by buildbot. Later we will have DM. So using CI
could avoid rename later.

https://codereview.chromium.org/1921493002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/monitor_try_job_pipeline.py:98: # A report should
always be included as prt of the properties. If it is
typo: prt -> part?

https://codereview.chromium.org/1921493002/diff/40001/appengine/findit/waterf...
File appengine/findit/waterfall/try_job_error.py (right):

https://codereview.chromium.org/1921493002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/try_job_error.py:1: # Copyright 2016 The Chromium
Authors. All rights reserved.
hmm, as findit/model/wf_try_job_data.py might refer to the error code here, it
seems better to add this module in findit/common/waterfall.
What your thought on leaving it here?

https://codereview.chromium.org/1921493002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/try_job_error.py:11: # The try job did not finish
within the required time and was was abandoned.
typo: 'was was'

[lijeffrey, 2016-04-26 19:07:25]

https://codereview.chromium.org/1921493002/diff/40001/appengine/findit/waterf...
File appengine/findit/waterfall/monitor_try_job_pipeline.py (right):

https://codereview.chromium.org/1921493002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/monitor_try_job_pipeline.py:83:
try_job_error.BUILDBUCKET_REPORTED_ERROR)
On 2016/04/26 17:33:06, stgao wrote:
> Maybe rename it to CI_REPORTED_ERROR.
> 
> result_details_json is reported by buildbot. Later we will have DM. So using
CI
> could avoid rename later.

Done.

https://codereview.chromium.org/1921493002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/monitor_try_job_pipeline.py:98: # A report should
always be included as prt of the properties. If it is
On 2016/04/26 17:33:06, stgao wrote:
> typo: prt -> part?

Oops, good catch.

https://codereview.chromium.org/1921493002/diff/40001/appengine/findit/waterf...
File appengine/findit/waterfall/try_job_error.py (right):

https://codereview.chromium.org/1921493002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/try_job_error.py:1: # Copyright 2016 The Chromium
Authors. All rights reserved.
On 2016/04/26 17:33:06, stgao wrote:
> hmm, as findit/model/wf_try_job_data.py might refer to the error code here, it
> seems better to add this module in findit/common/waterfall.
> What your thought on leaving it here?

I think you're right, common/waterfall is actually better

https://codereview.chromium.org/1921493002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/try_job_error.py:11: # The try job did not finish
within the required time and was was abandoned.
On 2016/04/26 17:33:07, stgao wrote:
> typo: 'was was'

Done.

[lijeffrey, 2016-04-26 19:07:39]

The CQ bit was checked by lijeffrey@chromium.org

[lijeffrey, 2016-04-26 19:07:40]

The patchset sent to the CQ was uploaded after l-g-t-m from chanli@chromium.org,
stgao@chromium.org
Link to the patchset: https://codereview.chromium.org/1921493002/#ps60001
(title: "Addressing comments")

[commit-bot: I haz the power, 2016-04-26 19:07:43]

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

[commit-bot: I haz the power, 2016-04-26 19:13:41]

Description was changed from

==========
[Findit] Adding improved error detection to MonitorTryJobPipeline

BUG=605344, 606038
==========

to

==========
[Findit] Adding improved error detection to MonitorTryJobPipeline

BUG=605344, 606038

Committed:
https://chromium.googlesource.com/infra/infra/+/4167fabff66f36b317553ea8ee8a2...
==========

[commit-bot: I haz the power, 2016-04-26 19:13:47]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/infra/infra/+/4167fabff66f36b317553ea8ee8a2...