[Findit] Implementing swarming task error detection

[Findit] Swarming task error detection

BUG=663712
Committed: https://chromium.googlesource.com/infra/infra/+/3ad87a6d11f9b7bd1d0fb75b7fce64b0725beec2

[lijeffrey, 2016-11-09 12:50:25]

Description was changed from

==========
.


.


.


.


Fixing pylint


[Findit] Implementing smarter error detection in monitoring and triggering
swarming task pipelines


Addressing comments


Addressing comments


Addressing comments


Fixing pylint issues


[Findit] Refactoring monitor swarming task pipelines

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

to

==========
[Findit] Refactoring monitor swarming task pipelines

BUG=663712
==========

[lijeffrey, 2016-11-09 12:50:25]

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

[lijeffrey, 2016-11-09 12:51:37]

Description was changed from

==========
[Findit] Refactoring monitor swarming task pipelines

BUG=663712
==========

to

==========
[Findit] Swarming task error detection

BUG=663712
==========

[lijeffrey, 2016-11-09 12:52:40]

note this change is based on the refactoring CL
https://codereview.chromium.org/2477343003/, so a lot of changes aren't actually
made in this CL

ptal when you have the chance

[chanli, 2016-11-11 00:05:13]

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

https://codereview.chromium.org/2491473002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:92:
logging.error(error.get('message'))
In swarming_util._SendRequestToServer the error message has been logged.

https://codereview.chromium.org/2491473002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:115:
logging.error(error.get('message'))
Same here

https://codereview.chromium.org/2491473002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:126:
swarming_util.UNKNOWN)
I think UNKNOWN is misleading here. If task_state in
swarming_util.STATES_NOT_RUNNING, the exit code might be empty but the error is
rather clear: timed_out, user canceled, etc. In other words, exit_code should be
used as error code only if the state is STATE_COMPLETED.

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

https://codereview.chromium.org/2491473002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/swarming_util.py:13: DeadlineExceededError,
DownloadError, ConnectionClosedError)
Nit: according to
https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s...

Imports should be on separate lines.

https://codereview.chromium.org/2491473002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/swarming_util.py:28: UNKNOWN = 0
Based on process_base_swarming_task_result_pipeline.py, this value is being used
as exit_code as well?

https://codereview.chromium.org/2491473002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/swarming_util.py:29: 
In my opinion, since they are all error codes so the 2 empty lines could be
removed?

https://codereview.chromium.org/2491473002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/swarming_util.py:35: EXCEEDED_MAX_RETRIES_ERROR = 210
Maybe we can use enum for error codes?

https://codereview.chromium.org/2491473002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/swarming_util.py:38: # Swarming task exit codes.
Should we use enum here?

https://codereview.chromium.org/2491473002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/swarming_util.py:38: # Swarming task exit codes.
I was confused then I realize you mixed swarming states and exit codes and then
used them as error codes. 

Actually TIMED_OUT, EXPIRED, BOT_DIED and CANCELED don't belong to here. 

And in the rest of exit_codes, only TASK_FAILED should be reported as error.

My suggestion is adding new error codes for the failing states and TASK_FAILED.

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

https://codereview.chromium.org/2491473002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:175:
swarming_task.task_id = task_id
If the swarming_task is not even triggered, should we save the None task_id and
parameters of the request?

https://codereview.chromium.org/2491473002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:184:
logging.info('A Swarming task was triggered:%s', task_id)
If error, we shouldn't have this log.

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

https://codereview.chromium.org/2491473002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/trigger_swarming_task_pipeline.py:164:
swarming_task.error = error
Comments of trigger_base_swarming_task_pipeline.py apply here

[lijeffrey, 2016-11-11 20:55:41]

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

https://codereview.chromium.org/2491473002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:92:
logging.error(error.get('message'))
On 2016/11/11 00:05:13, chanli wrote:
> In swarming_util._SendRequestToServer the error message has been logged.

Done.

https://codereview.chromium.org/2491473002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:115:
logging.error(error.get('message'))
On 2016/11/11 00:05:13, chanli wrote:
> Same here

Done.

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

https://codereview.chromium.org/2491473002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/swarming_util.py:13: DeadlineExceededError,
DownloadError, ConnectionClosedError)
On 2016/11/11 00:05:13, chanli wrote:
> Nit: according to
>
https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s...
> 
> Imports should be on separate lines.

Done.

https://codereview.chromium.org/2491473002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/swarming_util.py:28: UNKNOWN = 0
On 2016/11/11 00:05:13, chanli wrote:
> Based on process_base_swarming_task_result_pipeline.py, this value is being
used
> as exit_code as well?

Yes that probably is confusing so i made UNKNOWN its own thing separate from
exit codes

https://codereview.chromium.org/2491473002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/swarming_util.py:29: 
On 2016/11/11 00:05:13, chanli wrote:
> In my opinion, since they are all error codes so the 2 empty lines could be
> removed?

Done.

https://codereview.chromium.org/2491473002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/swarming_util.py:35: EXCEEDED_MAX_RETRIES_ERROR = 210
On 2016/11/11 00:05:13, chanli wrote:
> Maybe we can use enum for error codes?

I don't think there's a python enum?

https://codereview.chromium.org/2491473002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/swarming_util.py:38: # Swarming task exit codes.
On 2016/11/11 00:05:13, chanli wrote:
> I was confused then I realize you mixed swarming states and exit codes and
then
> used them as error codes. 
> 
> Actually TIMED_OUT, EXPIRED, BOT_DIED and CANCELED don't belong to here. 
> 
> And in the rest of exit_codes, only TASK_FAILED should be reported as error.
> 
> My suggestion is adding new error codes for the failing states and
TASK_FAILED.

Moved to a separate dict

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

https://codereview.chromium.org/2491473002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:175:
swarming_task.task_id = task_id
On 2016/11/11 00:05:13, chanli wrote:
> If the swarming_task is not even triggered, should we save the None task_id
and
> parameters of the request?

I think we can still save the parameters, but they wouldn't get used. This can
be a design choice, I can't think of a clear reason why or why not to go either
way. Maybe in case we want to implement retry with the same parameters as before
that failed, but I don't feel strongly about going either way

https://codereview.chromium.org/2491473002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:184:
logging.info('A Swarming task was triggered:%s', task_id)
On 2016/11/11 00:05:13, chanli wrote:
> If error, we shouldn't have this log.

Done.

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

https://codereview.chromium.org/2491473002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/trigger_swarming_task_pipeline.py:164:
swarming_task.error = error
On 2016/11/11 00:05:13, chanli wrote:
> Comments of trigger_base_swarming_task_pipeline.py apply here

Done. This file will be refactored in an upcoming CL as well, so this shouldn't
be a duplicated issue any longer

[chanli1, 2016-11-16 17:37:43]

chanli@google.com changed reviewers:
+ chanli@google.com

[chanli1, 2016-11-16 17:37:43]

lgtm

[lijeffrey, 2016-11-16 18:48:18]

The CQ bit was checked by lijeffrey@chromium.org

[lijeffrey, 2016-11-16 18:48:19]

The patchset sent to the CQ was uploaded after l-g-t-m from chanli@google.com
Link to the patchset: https://codereview.chromium.org/2491473002/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2016-11-16 18:48:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 19:03:48]

Description was changed from

==========
[Findit] Swarming task error detection

BUG=663712
==========

to

==========
[Findit] Swarming task error detection

BUG=663712

Committed:
https://chromium.googlesource.com/infra/infra/+/3ad87a6d11f9b7bd1d0fb75b7fce6...
==========

[commit-bot: I haz the power, 2016-11-16 19:03:49]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/infra/infra/+/3ad87a6d11f9b7bd1d0fb75b7fce6...