[Findit] Implement retry within swarming_util.py when making server calls

[Findit] Implement retry within swarming_util.py when making server calls

BUG=668262
Committed: https://chromium.googlesource.com/infra/infra/+/4d4e14a7bb428645ef77f04659f0b582b31466fc

[lijeffrey, 2016-11-23 21:09:37]

Description was changed from

==========
[Findit] Implement retry within swarming_util.py when making server calls

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

to

==========
[Findit] Implement retry within swarming_util.py when making server calls

BUG=668262
==========

[lijeffrey, 2016-11-23 21:09:37]

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

[lijeffrey, 2016-11-23 21:09:43]

ptal

[chanli, 2016-11-23 23:45:32]

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

https://codereview.chromium.org/2526963002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:98:
task.status = analysis_status.ERROR
This status is not saved in data store?

https://codereview.chromium.org/2526963002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:121:
task.put()
This put is not necessary?

https://codereview.chromium.org/2526963002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:127:
tests_statuses = self._CheckTestsRunStatuses(output_json, *call_args)
This line will affect ln 125, right?

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

https://codereview.chromium.org/2526963002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/swarming_util.py:104: should_retry =
swarming_settings.get('should_retry_server')
I have a question: How long is the server down typically? How do we decide the
time we wait and retry for a task?

https://codereview.chromium.org/2526963002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/swarming_util.py:164: if should_retry:
Shouldn't you also handle case when should_retry == False and should break the
loop?

[chanli, 2016-11-28 23:44:18]

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

https://codereview.chromium.org/2526963002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:127:
tests_statuses = self._CheckTestsRunStatuses(output_json, *call_args)
On 2016/11/23 23:45:32, chanli wrote:
> This line will affect ln 125, right?

This comment is not valid, please ignore.

[lijeffrey, 2016-11-28 23:55:25]

Comments addressed, no code changes so far

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

https://codereview.chromium.org/2526963002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:98:
task.status = analysis_status.ERROR
On 2016/11/23 23:45:32, chanli wrote:
> This status is not saved in data store?

This should be fine, outside the while loop there is a task.put() call.

https://codereview.chromium.org/2526963002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:121:
task.put()
On 2016/11/23 23:45:32, chanli wrote:
> This put is not necessary?

This can be a design choice, the idea is to capture the error as early as
possible so even flake swarming tasks in progress that are experiencing issues
can be detected/queried for earlier rather than waiting for the whole thing to
complete. WDYT?

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

https://codereview.chromium.org/2526963002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/swarming_util.py:104: should_retry =
swarming_settings.get('should_retry_server')
On 2016/11/23 23:45:32, chanli wrote:
> I have a question: How long is the server down typically? How do we decide the
> time we wait and retry for a task?

It's difficult to say just from the logs, which using data from October 2016
doesn't seem to be available anymore :(. Originally when I was seeing these
errors we only get 1 at a time, since there is no retry logic that 1 flake
analysis that happened to have been requested at that 1 time would see it, then
it would be a period of several hours until the next request came in which may
or may not end in the same error.

I plan to hook up an upcoming CL to use ts_mon for each retry to log the errors
encountered, so it should be possible to detect a continuous outage and we can
use that to adjust our timeouts accordingly.

https://codereview.chromium.org/2526963002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/swarming_util.py:164: if should_retry:
On 2016/11/23 23:45:32, chanli wrote:
> Shouldn't you also handle case when should_retry == False and should break the
> loop?

If I'm understanding your comment correctly, the should_retry == False too will
break the loop. We only want to include more information in the error dict that
Findit eventually gave up on trying after retrying for a period of time.

[chanli, 2016-11-29 18:49:38]

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

https://codereview.chromium.org/2526963002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:98:
task.status = analysis_status.ERROR
On 2016/11/28 23:55:24, lijeffrey wrote:
> On 2016/11/23 23:45:32, chanli wrote:
> > This status is not saved in data store?
> 
> This should be fine, outside the while loop there is a task.put() call.

I just want to make it consistent. There is a task.put() at line94, so maybe
remove that line? Or as you mentioned in the comment at ln121, save it right
away?

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

https://codereview.chromium.org/2526963002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/swarming_util.py:104: should_retry =
swarming_settings.get('should_retry_server')
On 2016/11/28 23:55:24, lijeffrey wrote:
> On 2016/11/23 23:45:32, chanli wrote:
> > I have a question: How long is the server down typically? How do we decide
the
> > time we wait and retry for a task?
> 
> It's difficult to say just from the logs, which using data from October 2016
> doesn't seem to be available anymore :(. Originally when I was seeing these
> errors we only get 1 at a time, since there is no retry logic that 1 flake
> analysis that happened to have been requested at that 1 time would see it,
then
> it would be a period of several hours until the next request came in which may
> or may not end in the same error.
> 
> I plan to hook up an upcoming CL to use ts_mon for each retry to log the
errors
> encountered, so it should be possible to detect a continuous outage and we can
> use that to adjust our timeouts accordingly.

Acknowledged.

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

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

https://codereview.chromium.org/2526963002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:98:
task.status = analysis_status.ERROR
On 2016/11/29 18:49:38, chanli wrote:
> On 2016/11/28 23:55:24, lijeffrey wrote:
> > On 2016/11/23 23:45:32, chanli wrote:
> > > This status is not saved in data store?
> > 
> > This should be fine, outside the while loop there is a task.put() call.
> 
> I just want to make it consistent. There is a task.put() at line94, so maybe
> remove that line? Or as you mentioned in the comment at ln121, save it right
> away?

I don't think the put() call belongs here, since immediately after the break
there are the calls to update the time stamps which have a put() after them too
so there would be 2 writes in close succession. The case for the error is we
want to write the error, even if the status is eventually COMPLETED but we want
to note that an error was encountered so when we measure performance we don't
penalize the speed of the swarming task unnecessarily because of unexpected
outages.

It is possible for both data and error to be returned, meaning the call was
eventually successful but issues were encountered so the .put() call is needed
for the if error branch

[chanli, 2016-11-30 18:53:29]

lgtm with 2 nits.

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

https://codereview.chromium.org/2526963002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:98:
task.status = analysis_status.ERROR
On 2016/11/30 18:18:47, lijeffrey wrote:
> On 2016/11/29 18:49:38, chanli wrote:
> > On 2016/11/28 23:55:24, lijeffrey wrote:
> > > On 2016/11/23 23:45:32, chanli wrote:
> > > > This status is not saved in data store?
> > > 
> > > This should be fine, outside the while loop there is a task.put() call.
> > 
> > I just want to make it consistent. There is a task.put() at line94, so maybe
> > remove that line? Or as you mentioned in the comment at ln121, save it right
> > away?
> 
> I don't think the put() call belongs here, since immediately after the break
> there are the calls to update the time stamps which have a put() after them
too
> so there would be 2 writes in close succession. The case for the error is we
> want to write the error, even if the status is eventually COMPLETED but we
want
> to note that an error was encountered so when we measure performance we don't
> penalize the speed of the swarming task unnecessarily because of unexpected
> outages.
> 
> It is possible for both data and error to be returned, meaning the call was
> eventually successful but issues were encountered so the .put() call is needed
> for the if error branch

As discussed offline, this change is not needed.

https://codereview.chromium.org/2526963002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:123: if
not output_json:
If not output_json, this task is actually failed and we should break the loop
right away. I just committed a CL yesterday with this change, so you need to
rebase and (possibly) address conflicts.

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

https://codereview.chromium.org/2526963002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/swarming_util.py:118: headers['Content-Length'] =
len(post_data)
Nit: This part can be done only once outside of the loop.

[lijeffrey, 2016-11-30 20:12:11]

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

https://codereview.chromium.org/2526963002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:123: if
not output_json:
On 2016/11/30 18:53:29, chanli wrote:
> If not output_json, this task is actually failed and we should break the loop
> right away. I just committed a CL yesterday with this change, so you need to
> rebase and (possibly) address conflicts.

Rebased. As discussed offline, it's possible we get output_refs, but then when
trying to contact the isolated server we experience problems, so this should be
ok as it is.

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

https://codereview.chromium.org/2526963002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/swarming_util.py:118: headers['Content-Length'] =
len(post_data)
On 2016/11/30 18:53:29, chanli wrote:
> Nit: This part can be done only once outside of the loop.

Done.

[lijeffrey, 2016-11-30 20:12:56]

The CQ bit was checked by lijeffrey@chromium.org

[lijeffrey, 2016-11-30 20:12:58]

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

[commit-bot: I haz the power, 2016-11-30 20:13:03]

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-30 20:28:17]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480536776373270,
"parent_rev": "0ed361e65ed78f95b9b4f5b43400fe3dba887457", "commit_rev":
"4d4e14a7bb428645ef77f04659f0b582b31466fc"}

[commit-bot: I haz the power, 2016-11-30 20:28:20]

Description was changed from

==========
[Findit] Implement retry within swarming_util.py when making server calls

BUG=668262
==========

to

==========
[Findit] Implement retry within swarming_util.py when making server calls

BUG=668262

Committed:
https://chromium.googlesource.com/infra/infra/+/4d4e14a7bb428645ef77f04659f0b...
==========

[commit-bot: I haz the power, 2016-11-30 20:28:22]

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

[stgao, 2016-12-01 20:33:46]

stgao@chromium.org changed reviewers:
+ stgao@chromium.org

[stgao, 2016-12-01 20:33:47]

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

https://codereview.chromium.org/2526963002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/swarming_util.py:99: swarming_task (WfSwarmingTask,
FlakeSwarmingTask): An optional swarming
This seems invalid now.

https://codereview.chromium.org/2526963002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/swarming_util.py:119: post_data =
json.dumps(post_data, sort_keys=True, separators=(',', ':'))
Why we need to sort?

[lijeffrey, 2016-12-01 20:51:12]

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

https://codereview.chromium.org/2526963002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/swarming_util.py:99: swarming_task (WfSwarmingTask,
FlakeSwarmingTask): An optional swarming
On 2016/12/01 20:33:46, stgao (slow on Monday) wrote:
> This seems invalid now.

Good catch, originally I had designed this to consume a swarming task that can
capture the error but ended up putting that logic in the calling function
instead. I will remove this line in a follow up CL.

https://codereview.chromium.org/2526963002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/swarming_util.py:119: post_data =
json.dumps(post_data, sort_keys=True, separators=(',', ':'))
On 2016/12/01 20:33:46, stgao (slow on Monday) wrote:
> Why we need to sort?

Chan may be more familiar with how post_data needs to be formatted, my change is
only to move this particular line it outside of the while loop/try statement so
it's not regenerated each time.