Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1350673004:
gerrit_util: Add GCE metadata server auth. (Closed)
|
Created:
5 years, 3 months ago by dnj Modified:
5 years, 2 months ago CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
Descriptiongerrit_util: Add GCE metadata server auth.
BUG=chromium:532318
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=296930
Patch Set 1 #Patch Set 2 : Little cleanup. #
Total comments: 2
Patch Set 3 : Cleanup, better layout. #
Total comments: 11
Patch Set 4 : Fix token error, cleanup, retry on 500+. #
Total comments: 4
Patch Set 5 : Log retries at INFO. #Messages
Total messages: 25 (7 generated)
dnj@google.com changed reviewers: + dnj@google.com, sergiyb@chromium.org, smut@google.com
PTAL sergiyb@: I copied a lot of this from your Twisted implementation.
Generally seems okay % aesthetic comment, but wait for Sergiy. https://codereview.chromium.org/1350673004/diff/20001/gerrit_util.py File gerrit_util.py (right): https://codereview.chromium.org/1350673004/diff/20001/gerrit_util.py#newcode133 gerrit_util.py:133: return '%(token_type)s %(access_token)s' % token_dict Move classes down (at least below module-level constant on line 138, but preferably below GetConnectionClass (defined starting at 163) because that function is referenced on line 111. Easier to read top-down if GetConnectionClass has already been defined by the time we get to its usage.
https://codereview.chromium.org/1350673004/diff/20001/gerrit_util.py File gerrit_util.py (right): https://codereview.chromium.org/1350673004/diff/20001/gerrit_util.py#newcode133 gerrit_util.py:133: return '%(token_type)s %(access_token)s' % token_dict On 2015/09/18 22:32:43, smut wrote: > Move classes down (at least below module-level constant on line 138, but > preferably below GetConnectionClass (defined starting at 163) because that > function is referenced on line 111. Easier to read top-down if > GetConnectionClass has already been defined by the time we get to its usage. Done.
https://codereview.chromium.org/1350673004/diff/40001/gerrit_util.py File gerrit_util.py (right): https://codereview.chromium.org/1350673004/diff/40001/gerrit_util.py#newcode124 gerrit_util.py:124: class GceAuthenticator(Authenticator): I recall writing this code. Can you please reference the source so that I can compare? I couldn't find it myself: https://code.google.com/p/chromium/codesearch#search/&q=GceAuthenticator&sq=p...
On 2015/09/22 11:49:49, Sergiy Byelozyorov wrote: > https://codereview.chromium.org/1350673004/diff/40001/gerrit_util.py > File gerrit_util.py (right): > > https://codereview.chromium.org/1350673004/diff/40001/gerrit_util.py#newcode124 > gerrit_util.py:124: class GceAuthenticator(Authenticator): > I recall writing this code. Can you please reference the source so that I can > compare? I couldn't find it myself: > https://code.google.com/p/chromium/codesearch#search/&q=GceAuthenticator&sq=p... https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/commo...
(ping)
On 2015/09/23 16:20:13, dnj (Google) wrote: > (ping) Re-ping?
https://codereview.chromium.org/1350673004/diff/40001/gerrit_util.py File gerrit_util.py (right): https://codereview.chromium.org/1350673004/diff/40001/gerrit_util.py#newcode140 gerrit_util.py:140: cls._cache_is_gce = cls._test_is_gce() Please add 5 retries inside out outside this call. Metadata endpoint may flake with 500 just like AppEngine does. https://codereview.chromium.org/1350673004/diff/40001/gerrit_util.py#newcode165 gerrit_util.py:165: return cls._token_cache You actually return the token only if it expires within the next 25 seconds... and otherwise you always refresh it. Change: s/>/</ https://codereview.chromium.org/1350673004/diff/40001/gerrit_util.py#newcode167 gerrit_util.py:167: resp = cls._get(cls._ACQUIRE_URL, headers=cls._ACQUIRE_HEADERS) Please add 5 retries around this. Metadata endpoint may flake with 500 just like AppEngine does. https://codereview.chromium.org/1350673004/diff/40001/gerrit_util.py#newcode170 gerrit_util.py:170: cls._token_cache = token_dict = json.load(resp) why not just assign to cls._token_cache and use that in the next statement?
https://codereview.chromium.org/1350673004/diff/40001/gerrit_util.py File gerrit_util.py (right): https://codereview.chromium.org/1350673004/diff/40001/gerrit_util.py#newcode124 gerrit_util.py:124: class GceAuthenticator(Authenticator): On 2015/09/22 11:49:49, Sergiy Byelozyorov wrote: > I recall writing this code. Can you please reference the source so that I can > compare? I couldn't find it myself: > https://code.google.com/p/chromium/codesearch#search/&q=GceAuthenticator&sq=p... This is standalone, so I don't think referencing the Twisted one in "build/" is useful here. https://codereview.chromium.org/1350673004/diff/40001/gerrit_util.py#newcode140 gerrit_util.py:140: cls._cache_is_gce = cls._test_is_gce() On 2015/09/25 15:36:20, Sergiy Byelozyorov wrote: > Please add 5 retries inside out outside this call. Metadata endpoint may flake > with 500 just like AppEngine does. I'm fairly reluctant to do 5 retries in all circumstances, but I'll throw in some logic to retry on 500s. https://codereview.chromium.org/1350673004/diff/40001/gerrit_util.py#newcode165 gerrit_util.py:165: return cls._token_cache On 2015/09/25 15:36:20, Sergiy Byelozyorov wrote: > You actually return the token only if it expires within the next 25 seconds... > and otherwise you always refresh it. Change: s/>/</ lol good catch https://codereview.chromium.org/1350673004/diff/40001/gerrit_util.py#newcode167 gerrit_util.py:167: resp = cls._get(cls._ACQUIRE_URL, headers=cls._ACQUIRE_HEADERS) On 2015/09/25 15:36:20, Sergiy Byelozyorov wrote: > Please add 5 retries around this. Metadata endpoint may flake with 500 just like > AppEngine does. Done. https://codereview.chromium.org/1350673004/diff/40001/gerrit_util.py#newcode170 gerrit_util.py:170: cls._token_cache = token_dict = json.load(resp) On 2015/09/25 15:36:20, Sergiy Byelozyorov wrote: > why not just assign to cls._token_cache and use that in the next statement? It avoids one more dictionary getattr hit *shrug*. Doesn't really matter though, so I'll simplify it since you think it's useful.
lgtm w/ comment https://codereview.chromium.org/1350673004/diff/40001/gerrit_util.py File gerrit_util.py (right): https://codereview.chromium.org/1350673004/diff/40001/gerrit_util.py#newcode140 gerrit_util.py:140: cls._cache_is_gce = cls._test_is_gce() On 2015/09/25 17:40:55, dnj wrote: > On 2015/09/25 15:36:20, Sergiy Byelozyorov wrote: > > Please add 5 retries inside out outside this call. Metadata endpoint may flake > > with 500 just like AppEngine does. > > I'm fairly reluctant to do 5 retries in all circumstances, but I'll throw in > some logic to retry on 500s. If you think 5 is too much, feel free to reduce it to 3 or 2, but not retrying at all is also not good. Also AFAIU this will run on bots, so users won't be annoyed too much by this. https://codereview.chromium.org/1350673004/diff/60001/gerrit_util.py File gerrit_util.py (right): https://codereview.chromium.org/1350673004/diff/60001/gerrit_util.py#newcode160 gerrit_util.py:160: next_delay_sec) I wonder if this delay would be visible to the user? Unlikely since this authenticator only makes sense on GCE, but if it would, then we should probably provide some feedback before making a script sleep for 16 seconds. https://codereview.chromium.org/1350673004/diff/60001/gerrit_util.py#newcode169 gerrit_util.py:169: if resp.status < httplib.INTERNAL_SERVER_ERROR: why < and not !=?
https://codereview.chromium.org/1350673004/diff/60001/gerrit_util.py File gerrit_util.py (right): https://codereview.chromium.org/1350673004/diff/60001/gerrit_util.py#newcode160 gerrit_util.py:160: next_delay_sec) On 2015/09/25 20:06:34, Sergiy Byelozyorov wrote: > I wonder if this delay would be visible to the user? Unlikely since this > authenticator only makes sense on GCE, but if it would, then we should probably > provide some feedback before making a script sleep for 16 seconds. I'll update to `info` level. https://codereview.chromium.org/1350673004/diff/60001/gerrit_util.py#newcode169 gerrit_util.py:169: if resp.status < httplib.INTERNAL_SERVER_ERROR: On 2015/09/25 20:06:34, Sergiy Byelozyorov wrote: > why < and not !=? b/c I want to catch the full range of 500+ errors, since they're all internal server errors.
Patchset #5 (id:80001) has been deleted
lgtm
The CQ bit was checked by dnj@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1350673004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1350673004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: depot_tools_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/depot_tools_pre...)
dnj@google.com changed reviewers: + agable@chromium.org, pgervais@chromium.org
+agable@, pgervais@ for depot_tools owners.
On 2015/09/29 15:55:43, dnj (Google) wrote: > +agable@, pgervais@ for depot_tools owners. rs-lgtm
The CQ bit was checked by dnj@chromium.org
The CQ bit was checked by dnj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1350673004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1350673004/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=296930 |
