Android: improves test sharding reliability.

build/android/pylib/base_test_sharder.py

Index: build/android/pylib/base_test_sharder.py
diff --git a/build/android/pylib/base_test_sharder.py b/build/android/pylib/base_test_sharder.py
index 5306769155050ba032364650daeecaa1581e609b..1418cf0f6fd0d1b296ef3db41d0a6dd7b88c83a5 100644
--- a/build/android/pylib/base_test_sharder.py
+++ b/build/android/pylib/base_test_sharder.py
@@ -7,6 +7,7 @@ import android_commands
 import logging
 import multiprocessing
 
+from android_commands import errors
 from test_result import TestResults
 
 
@@ -43,7 +44,9 @@ class BaseTestSharder(object):
 
   def __init__(self, attached_devices):
     self.attached_devices = attached_devices
-    self.retries = 1
+    # Worst case scenario: a device will drop offline per run, so we need
+    # to retry until we're out of devices.
+    self.retries = len(self.attached_devices)
     self.tests = []
 
   def CreateShardedTestRunner(self, device, index):
@@ -83,12 +86,20 @@ class BaseTestSharder(object):
       logging.warning('Try %d of %d', retry + 1, self.retries)
       self.SetupSharding(self.tests)
       test_runners = []
-      for index, device in enumerate(self.attached_devices):
-        logging.warning('*' * 80)
-        logging.warning('Creating shard %d for %s', index, device)
-        logging.warning('*' * 80)
-        test_runner = self.CreateShardedTestRunner(device, index)
-        test_runners += [test_runner]
+
+      # Try to create N shards, and retrying on failure.
+      try:

[Yaron, 2012/10/31 17:18:51]

Shouldn't this try be in the for loop? |device| is only defined in there.

[bulach, 2012/10/31 18:10:35]

not really.. we need to create all shards at once, or fail.
python allows for |device| to be available outside the for scope.
if we move the try inside the for loop, we'd need to somehow abort the current
test_runners creation and retry all over again..

+        for index, device in enumerate(self.attached_devices):
+          logging.warning('*' * 80)
+          logging.warning('Creating shard %d for %s', index, device)
+          logging.warning('*' * 80)
+          test_runner = self.CreateShardedTestRunner(device, index)
+          test_runners += [test_runner]
+      except errors.DeviceUnresponsiveError as e:
+        logging.critical('****Failed to create a shard: [%s]', e)
+        self.attached_devices.remove(device)
+        continue

[Yaron, 2012/10/31 17:18:51]

By continuing, we're not updating |final_results|. It looks like it might only
be tabulating successful tests in which case it's ok to skip?

[bulach, 2012/10/31 18:10:35]

this continue here would be for the loop on 85, so there's nothing to update yet
here...

the flow is basically:

- try to create N shards.
|-- if it fails, retry.
- dispatch and collect results for the N
|-- if it fails, retry

at the end, since we got N results for N devices, try to optimize what should
run next..

does it make sense?

[Yaron, 2012/10/31 18:37:23]

Yes. My point was though that it looks like it's doing special accounting on the
last run of the loop. If you "continue" here, it never gets a chance to do that
and we can't report that any tests successfully ran. I guess if we end up with 0
devices it'll be a pretty big failure anyway so it's ok.

However, looking at OnTestsCompleted in build/android/run_tests.py it assumes
that test_runners will be non-empty and that's not true.

[bulach, 2012/10/31 19:18:17]

ahn, yes, good point!
I added an else: clause to raise an exception if we run out of retries... at
that point, there's no possible recovery anyways, but this at least clarifies
which condition we hit..

+
       logging.warning('Starting...')
       pool = multiprocessing.Pool(len(self.attached_devices),
                                   SetTestsContainer,
@@ -96,8 +107,12 @@ class BaseTestSharder(object):
       # map can't handle KeyboardInterrupt exception. It's a python bug.
       # So use map_async instead.
       async_results = pool.map_async(_ShardedTestRunnable, test_runners)
-      results_lists = async_results.get(999999)
-
+      try:
+        results_lists = async_results.get(999999)

[Yaron, 2012/10/31 17:18:51]

Will the exception get thrown while the other shards are still running? If so,
does this mean that we'll whip around and retry shards of tests on devices that
are still running their old tests?

[bulach, 2012/10/31 18:10:35]

it's not clear from the python documentation if the exception is raised as soon
as the shard throws, or later when all the results are collected.

thankfully, it doesn't matter :)

we'll create the shards again, and that re-installs the apks, which kills the
process.

[Yaron, 2012/10/31 18:37:23]

Ah, right. Thanks.

+      except errors.DeviceUnresponsiveError as e:
+        logging.critical('****Failed to run test: [%s]', e)
+        self.attached_devices = android_commands.GetAttachedDevices()
+        continue
       test_results = TestResults.FromTestResults(results_lists)
       # Re-check the attached devices for some devices may
       # become offline