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

appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py

Index: appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py
diff --git a/appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py b/appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py
index bae4096c6d5d0edd8e7bb787c44c99a1db310b93..f661e2e92cb0123f5aab46461b14deaec6b6410a 100644
--- a/appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py
+++ b/appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py
@@ -88,11 +88,15 @@ class ProcessBaseSwarmingTaskResultPipeline(BasePipeline):
           task_id, self.HTTP_CLIENT)
 
       if error:
-        # An error occurred when trying to contact the swarming server.
-        task.status = analysis_status.ERROR
+        # An error occurred at some point when trying to retrieve data from
+        # the swarming server, even if eventually successful.
         task.error = error
         task.put()
-        break
+
+        if not data:
+          # Even after retry, no data was recieved.
+          task.status = analysis_status.ERROR

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

This status is not saved in data store?

[lijeffrey, 2016/11/28 23:55:24]

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

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

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?

[lijeffrey, 2016/11/30 18:18:47]

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]

As discussed offline, this change is not needed.

+          break
 
       task_state = data['state']
       exit_code = (data.get('exit_code') if
@@ -110,11 +114,15 @@ class ProcessBaseSwarmingTaskResultPipeline(BasePipeline):
           output_json, error = swarming_util.GetSwarmingTaskFailureLog(
               outputs_ref, self.HTTP_CLIENT)
 
+          task.status = analysis_status.COMPLETED
+
           if error:
-            task.status = analysis_status.ERROR
             task.error = error
-          else:
-            task.status = analysis_status.COMPLETED
+            task.put()

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

This put is not necessary?

[lijeffrey, 2016/11/28 23:55:24]

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?

+
+            if not output_json:

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

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.

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

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.

+              # Retry was ultimately unsuccessful.
+              task.status = analysis_status.ERROR
 
           tests_statuses = self._CheckTestsRunStatuses(output_json, *call_args)

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

This line will affect ln 125, right?

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

This comment is not valid, please ignore.

           task.tests_statuses = tests_statuses