Implemented retries in download_sdk_extras.py

build/download_sdk_extras.py

 #!/usr/bin/env python
 # Copyright 2014 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.
 
 """Script to download sdk/extras packages on the bots from google storage.
 
 The script expects arguments that specify zips file in the google storage
 bucket named: <dir in SDK extras>_<package name>_<version>.zip. The file will
 be extracted in the android_tools/sdk/extras directory on the test bots. This
@@ skipping 27 matching lines @@
 
 def clean_and_extract(dir_name, package_name, zip_file):
   local_dir = '%s/%s/%s' % (SDK_EXTRAS_PATH, dir_name, package_name)
   if os.path.exists(local_dir):
     shutil.rmtree(local_dir)
   local_zip = '%s/%s' % (SDK_EXTRAS_PATH, zip_file)
   with zipfile.ZipFile(local_zip) as z:
     z.extractall(path=SDK_EXTRAS_PATH)
 
 
+def download_package(remote_file, local_file):
+  if not os.path.exists(local_file):
+    try:
+      subprocess.check_call(['python', GSUTIL_PATH, '--force-version', '4.7',
+                             'cp', remote_file, local_file])
+    except subprocess.CalledProcessError:
+      print ('WARNING: Failed to download SDK packages. If this bot compiles '
+             'for Android, it may have errors.')
+      return 0
+
+
 def main():
   if not os.environ.get('CHROME_HEADLESS'):
     # This is not a buildbot checkout.
     return 0
   # Update the android_sdk_extras.json file to update downloaded packages.
   with open(SDK_EXTRAS_JSON_FILE) as json_file:
     packages = json.load(json_file)
   for package in packages:
     local_zip = '%s/%s' % (SDK_EXTRAS_PATH, package['zip'])
-    if not os.path.exists(local_zip):
-      package_zip = '%s/%s' % (SDK_EXTRAS_BUCKET, package['zip'])
+    package_zip = '%s/%s' % (SDK_EXTRAS_BUCKET, package['zip'])
+    for attempt in xrange(2):
+      print '(%d) Downloading package %s' % (attempt + 1, package['zip'])
+      download_package(package_zip, local_zip)
       try:
-        subprocess.check_call(['python', GSUTIL_PATH, '--force-version', '4.7',
-                               'cp', package_zip, local_zip])
-      except subprocess.CalledProcessError:
-        print ('WARNING: Failed to download SDK packages. If this bot compiles '
-               'for Android, it may have errors.')
-        return 0
-    # Always clean dir and extract zip to ensure correct contents.
-    clean_and_extract(package['dir_name'], package['package'], package['zip'])
+        # Always clean dir and extract zip to ensure correct contents.
+        clean_and_extract(package['dir_name'],
+                          package['package'],
+                          package['zip'])
+      except zipfile.BadZipfile:
+        print 'Failed unpacking zip file. Deleting and retrying...'
+        os.remove(local_zip)
+        continue

[dnj, 2015/07/30 22:49:03]

I don't like this continue/break thing. Either break in the "try" (continue by
default), or make this entire download/retry loop a separate function that you
can return from.

[pgervais, 2015/07/30 23:05:16]

Done.

+      break
+    else:
+      print ('WARNING: Failed to get SDK packages. If this bot compiles '
+             'for Android, it may have errors.')

[dnj, 2015/07/30 22:49:03]

This should return an error on failure.

[pgervais, 2015/07/30 23:05:16]

Note the return 0 on line 56: it previously exited with 0 when the download
failed (btw there is another issue there that I should fix).

[navabi, 2015/07/30 23:20:31]

This will cause failures when gsutil cp fails to copy the file from the google
storage bucket. An example of this is when google storage is down, which is not
uncommon. Another example is the kind of failures we experienced last week which
saturated our downloads. 

Give me a second to see if I can find previous discussion on this and remind
myself how big of a deal it is when we can't get the zip file.

[navabi, 2015/07/30 23:40:39]

This can not simply return an error on failure. If we want to fail, we need to
make sure that the reason it failed is not because AccessDeniedException. If it
fails because AcccessDeniedException, then it should not error on failure or a
lot of bots will error on failure that should not.

The reason for this is that we don't have a good way to tell what bots need to
download the sdk. Thus, there are bots that call this, that do not need the sdk
and fail because AccessDeniedException. 

It's ugly. To see the discussion see this bug:
https://code.google.com/p/chromium/issues/detail?id=460463

Specifically, we can get rid of this ugliness if we can do what the original
code attempted to do as explained in comment #6. I don't know how to do that
(i.e. how to say, if this is an android builder bot).

[dnj, 2015/07/30 23:45:34]

:(

 
 
 if __name__ == '__main__':
   sys.exit(main())