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

appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py

 # Copyright 2016 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 from collections import defaultdict
 import datetime
 import logging
 import time
 
 from common.http_client_appengine import HttpClientAppengine as HttpClient
@@ skipping 70 matching lines @@
     task_completed = False
     tests_statuses = {}
     step_name_no_platform = None
     task = self._GetSwarmingTask(*call_args)
 
     while not task_completed:
       data, error = swarming_util.GetSwarmingTaskResultById(
           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
                    task_state == swarming_util.STATE_COMPLETED else None)
       step_name_no_platform = (
           step_name_no_platform or swarming_util.GetTagValue(
               data.get('tags', {}), 'ref_name'))
 
       if task_state not in swarming_util.STATES_RUNNING:
         task_completed = True
 
         if (task_state == swarming_util.STATE_COMPLETED and
             int(exit_code) != swarming_util.TASK_FAILED):
           outputs_ref = data.get('outputs_ref')
           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
           task.put()
         else:
           if exit_code is not None:
             # Swarming task completed, but the task failed.
             code = int(exit_code)
             message = swarming_util.EXIT_CODE_DESCRIPTIONS[code]
           else:
             # The swarming task did not complete.
             code = swarming_util.STATES_NOT_RUNNING_TO_ERROR_CODES[task_state]
@@ skipping 62 matching lines @@
       task_id (str): The task id to query the swarming server on the progresss
         of a swarming task.
 
     Returns:
       A dict of lists for reliable/flaky tests.
     """
     call_args = self._GetArgs(master_name, builder_name, build_number,
                               step_name, *args)
     step_name_no_platform = self._MonitorSwarmingTask(task_id, *call_args)
     return step_name, step_name_no_platform