Implemented retries in download_sdk_extras.py

build/download_sdk_extras.py

Index: build/download_sdk_extras.py
diff --git a/build/download_sdk_extras.py b/build/download_sdk_extras.py
index d7c5d6cad6c9eec10690644bf7ba485189e3f287..d55ea7bd0d453629ae22e4d0a0934a06fd47d12f 100755
--- a/build/download_sdk_extras.py
+++ b/build/download_sdk_extras.py
@@ -45,6 +45,17 @@ def clean_and_extract(dir_name, package_name, zip_file):
     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.
@@ -54,17 +65,23 @@ def main():
     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__':