Android: improves test sharding reliability.

build/android/pylib/base_test_sharder.py

 # Copyright (c) 2012 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.
 
 
 import android_commands
 import logging
 import multiprocessing
 
+from android_commands import errors
 from test_result import TestResults
 
 
 def _ShardedTestRunnable(test):
   """Standalone function needed by multiprocessing.Pool."""
   log_format = '[' + test.device + '] # %(asctime)-15s: %(message)s'
   if logging.getLogger().handlers:
     logging.getLogger().handlers[0].setFormatter(logging.Formatter(log_format))
   else:
     logging.basicConfig(format=log_format)
@@ skipping 16 matching lines @@
   """Base class for sharding tests across multiple devices.
 
   Args:
     attached_devices: A list of attached devices.
   """
   # See more in SetTestsContainer.
   tests_container = None
 
   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):
     """Factory function to create a suite-specific test runner.
 
     Args:
       device: Device serial where this shard will run
       index: Index of this device in the pool.
 
     Returns:
@@ skipping 19 matching lines @@
     logging.warning('Sharding in ' + str(len(self.attached_devices)) +
                     ' devices.')
     logging.warning('Note that the output is not synchronized.')
     logging.warning('Look for the "Final result" banner in the end.')
     logging.warning('*' * 80)
     final_results = TestResults()
     for retry in xrange(self.retries):
       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,
                                   [BaseTestSharder.tests_container])
       # 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
       retry_devices = set(android_commands.GetAttachedDevices())
       # Remove devices that had exceptions.
       retry_devices -= TestResults.DeviceExceptions(results_lists)
       # Retry on devices that didn't have any exception.
       self.attached_devices = list(retry_devices)
       if (retry == self.retries - 1 or
           len(self.attached_devices) == 0):
         all_passed = final_results.ok + test_results.ok
         final_results = test_results
         final_results.ok = all_passed
         break
       else:
         final_results.ok += test_results.ok
         self.tests = []
         for t in test_results.GetAllBroken():
           self.tests += [t.name]
         if not self.tests:
           break
     self.OnTestsCompleted(test_runners, final_results)
     return final_results