Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1264063002:
Implemented retries in download_sdk_extras.py (Closed)
DescriptionImplemented retries in download_sdk_extras.py
BUG=512540
Committed: https://crrev.com/c4d2114d3ca15736142f9ade1bbf273cc59f2e53
Cr-Commit-Position: refs/heads/master@{#341252}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Addressed review comments #
Total comments: 3
Patch Set 3 : Addressed more review comments #Messages
Total messages: 25 (8 generated)
pgervais@chromium.org changed reviewers: + navabi@google.com
PTAL, this should fix the 'zipfile.BadZipfile: File is not a zip file' errors we've been seeing. This code is essentially not tested because I don't have the proper credentials on my local machine.
navabi@google.com changed reviewers: + dnj@chromium.org, iannucci@chromium.org
https://chromiumcodereview.appspot.com/1264063002/diff/1/build/download_sdk_e... File build/download_sdk_extras.py (right): https://chromiumcodereview.appspot.com/1264063002/diff/1/build/download_sdk_e... build/download_sdk_extras.py:80: continue 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. https://chromiumcodereview.appspot.com/1264063002/diff/1/build/download_sdk_e... build/download_sdk_extras.py:84: 'for Android, it may have errors.') This should return an error on failure.
https://chromiumcodereview.appspot.com/1264063002/diff/1/build/download_sdk_e... File build/download_sdk_extras.py (right): https://chromiumcodereview.appspot.com/1264063002/diff/1/build/download_sdk_e... build/download_sdk_extras.py:80: continue On 2015/07/30 22:49:03, dnj wrote: > 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. Done. https://chromiumcodereview.appspot.com/1264063002/diff/1/build/download_sdk_e... build/download_sdk_extras.py:84: 'for Android, it may have errors.') On 2015/07/30 22:49:03, dnj wrote: > This should return an error on failure. 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).
New patchset filed, PTAL. Armand: could you confirm that exiting with status 0 is the right thing do when the download fails? It was the previous behavior not sure it's the best idea.
I tried that the patch and it works when the zip file is bad. navabi@yeezus:/usr/local/google/code/clankium-master/src/build$ CHROME_HEADLESS='foo' python download_sdk_extras.py (1) Downloading package google_google_play_services_21.0.0.zip Failed unpacking zip file. Deleting and retrying... (2) Downloading package google_google_play_services_21.0.0.zip Copying gs://chrome-sdk-extras/google_google_play_services_21.0.0.zip... Downloading ...dk/extras/google_google_play_services_21.0.0.zip: 3.26 MiB/3.26 MiB Downloading ...dk/extras/google_google_play_services_21.0.0.zip: 3.26 MiB/3.26 MiB Taking a look regarding the question about return code when gsutil cp of the zip file fails. https://chromiumcodereview.appspot.com/1264063002/diff/1/build/download_sdk_e... File build/download_sdk_extras.py (right): https://chromiumcodereview.appspot.com/1264063002/diff/1/build/download_sdk_e... build/download_sdk_extras.py:84: 'for Android, it may have errors.') On 2015/07/30 23:05:16, pgervais wrote: > On 2015/07/30 22:49:03, dnj wrote: > > This should return an error on failure. > > 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). 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.
I suggest at the least we make the failure to download smarter, so that it will error on failure *except* when we have an AccessDeniedException. (or we find a better solution) https://chromiumcodereview.appspot.com/1264063002/diff/1/build/download_sdk_e... File build/download_sdk_extras.py (right): https://chromiumcodereview.appspot.com/1264063002/diff/1/build/download_sdk_e... build/download_sdk_extras.py:84: 'for Android, it may have errors.') 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).
lgtm https://chromiumcodereview.appspot.com/1264063002/diff/1/build/download_sdk_e... File build/download_sdk_extras.py (right): https://chromiumcodereview.appspot.com/1264063002/diff/1/build/download_sdk_e... build/download_sdk_extras.py:84: 'for Android, it may have errors.') On 2015/07/30 23:40:39, navabi wrote: > 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). :(
sad lgtm from me too
The CQ bit was checked by pgervais@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1264063002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1264063002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
pgervais@chromium.org changed reviewers: + dpranke@google.com - iannucci@chromium.org
+dpranke for owners approval. ptal
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
https://chromiumcodereview.appspot.com/1264063002/diff/20001/build/download_s... File build/download_sdk_extras.py (right): https://chromiumcodereview.appspot.com/1264063002/diff/20001/build/download_s... build/download_sdk_extras.py:54: if not os.path.exists(local_file): it's confusing to me that a routine called download_package() doesn't unconditionally download something. Can you either pull the os.path.exists() check outside of the function, or rename this to download_package_if_needed() or some such? https://chromiumcodereview.appspot.com/1264063002/diff/20001/build/download_s... build/download_sdk_extras.py:77: return 0 # hiding downloading failures on purpose. I don't know what "hiding" means in this context, and this comment doesn't tell me *why* we want to do this. Can you be more verbose? https://chromiumcodereview.appspot.com/1264063002/diff/20001/build/download_s... build/download_sdk_extras.py:87: continue this continue is unnecessary, no?
New patchset uploaded. Thanks Dirk for the useful feedback.
lgtm, thanks!
The CQ bit was checked by pgervais@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dnj@chromium.org, navabi@google.com Link to the patchset: https://chromiumcodereview.appspot.com/1264063002/#ps40001 (title: "Addressed more review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1264063002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1264063002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c4d2114d3ca15736142f9ade1bbf273cc59f2e53 Cr-Commit-Position: refs/heads/master@{#341252} |
