Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1501663002:
annotated_run.py: Add LogDog bootstrapping. (Closed)
Descriptionannotated_run.py: Add LogDog bootstrapping.
BUG=chromium:550673
TEST=`./scripts/slave/unittests/annotated_run_test.py`
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=298289
Patch Set 1 #
Total comments: 24
Patch Set 2 : Cleanup, fixes. #
Total comments: 2
Patch Set 3 : Added some unit testing, fixed some things. #Patch Set 4 : Add CIPD bootstrapping. #Patch Set 5 : Update service account path now that it exists: https://chromereviews.googleplex.com/341937013 #
Total comments: 8
Patch Set 6 : Fixes, remove exception assert. #Patch Set 7 : Update LogDog commands, generate prefix. #Patch Set 8 : Updated, actually works. #
Total comments: 21
Patch Set 9 : Comments. #
Messages
Total messages: 24 (10 generated)
dnj@google.com changed reviewers: + dnj@google.com, estaab@chromium.org, iannucci@chromium.org, martiniss@chromium.org
Updated, PTAL. Migrated from https://chromiumcodereview.appspot.com/1468053008/ .
looks alright! Any chance of adding tests for the code you added here? Not sure if it's worth it, but I always like more tests :) https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run.py File scripts/slave/annotated_run.py (right): https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:71: 'credential_paths': ( Logdog is linux only for now? https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:96: 'logdog_butler_streamserver_gen', A bit hard for me to parse this name. https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:204: def _run_command(cmd, **kwargs): Duplicates? Line 134 https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:230: combination of plaetform and runtime values used in the setup and execution of typo https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:258: attrs.update(kw) Why not just call get_config ? https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:274: except Exception: just 'except'? https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:280: def __getattr__(self, key): Why don't we just set the attributes we want to support in the constructor? This would make it much easier for someone to read through this class and understand what data it stores. https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:304: This method probes the local environemnt and returns a (possibly empty) list typo https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:370: logdog_verbose.extend('-log_level=info') Don't you want append? https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:385: # Dump Annotee command to JSON. Clarify a bit? If you can think of a better way. Makes sense when you understand the below code, but takes a bit of thought. Maybe.... "Annotee wants a json file of the command it runs."? https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:393: ] + logdog_verbose + service_account_args + [ why are you mixing in-line lists and concatenating lists. Would prefer to pick one for readability. https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:797: LOGGER.info('Not using LogDog. Invoking `annotated_run.py` directly.') Invoking recipes.py directly
Thanks for the review pass. I'm definitely going to write some tests. https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run.py File scripts/slave/annotated_run.py (right): https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:71: 'credential_paths': ( On 2015/12/04 18:42:30, martiniss wrote: > Logdog is linux only for now? Yep. It can build/run on Windows, but I'm going to do an incremental rollout starting with Linux. https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:96: 'logdog_butler_streamserver_gen', On 2015/12/04 18:42:30, martiniss wrote: > A bit hard for me to parse this name. I'll add a comment. https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:204: def _run_command(cmd, **kwargs): On 2015/12/04 18:42:31, martiniss wrote: > Duplicates? Line 134 Oh yes, true that. https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:230: combination of plaetform and runtime values used in the setup and execution of On 2015/12/04 18:42:30, martiniss wrote: > typo Ack, this class is actually obsolete and shouldn't have made it in. Sorry about that. https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:258: attrs.update(kw) On 2015/12/04 18:42:31, martiniss wrote: > Why not just call get_config ? Obsolete, sorry. https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:274: except Exception: On 2015/12/04 18:42:30, martiniss wrote: > just 'except'? Obsolete, sorry. https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:280: def __getattr__(self, key): On 2015/12/04 18:42:30, martiniss wrote: > Why don't we just set the attributes we want to support in the constructor? This > would make it much easier for someone to read through this class and understand > what data it stores. Obsolete, sorry. https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:304: This method probes the local environemnt and returns a (possibly empty) list On 2015/12/04 18:42:30, martiniss wrote: > typo Done. https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:370: logdog_verbose.extend('-log_level=info') On 2015/12/04 18:42:31, martiniss wrote: > Don't you want append? Yep. https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:385: # Dump Annotee command to JSON. On 2015/12/04 18:42:30, martiniss wrote: > Clarify a bit? If you can think of a better way. Makes sense when you understand > the below code, but takes a bit of thought. > > Maybe.... "Annotee wants a json file of the command it runs."? Done. https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:393: ] + logdog_verbose + service_account_args + [ On 2015/12/04 18:42:31, martiniss wrote: > why are you mixing in-line lists and concatenating lists. Would prefer to pick > one for readability. Done. https://codereview.chromium.org/1501663002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:797: LOGGER.info('Not using LogDog. Invoking `annotated_run.py` directly.') On 2015/12/04 18:42:30, martiniss wrote: > Invoking recipes.py directly Done.
Looking pretty good! https://codereview.chromium.org/1501663002/diff/20001/scripts/slave/annotated... File scripts/slave/annotated_run.py (right): https://codereview.chromium.org/1501663002/diff/20001/scripts/slave/annotated... scripts/slave/annotated_run.py:213: def ensure_directory(*path): Duplicate with line 129. https://codereview.chromium.org/1501663002/diff/20001/scripts/slave/annotated... scripts/slave/annotated_run.py:349: raise LogDogNotBootstrapped('Required mastername/buildername is not set.') Maybe just a value error? Seems a bit weird to say that "Logdog is not bootstrapped" when really a value isn't set. 359 about not being whitelisted looks ok to me though.
Dan, looks like this one is back on you. (reverse ping)
Description was changed from ========== annotated_run.py: Add LogDog bootstrapping. BUG=chromium:550673 TEST=`./scripts/slave/unittests/annotated_run_test.py` ========== to ========== annotated_run.py: Add LogDog bootstrapping. BUG=chromium:550673 TEST=`./scripts/slave/unittests/annotated_run_test.py` ==========
estaab@chromium.org changed reviewers: - estaab@chromium.org
dnj@chromium.org changed reviewers: + vadimsh@chromium.org
PTAL +vadimsh@ for CIPD stuffs.
On 2016/01/08 at 23:54:33, dnj wrote: > PTAL > > +vadimsh@ for CIPD stuffs. lgtm, yay tests! Thanks for writing those. Should wait for vadim for CIPD though.
Patchset #5 (id:80001) has been deleted
lgtm to avoid blocking butler roll out not a fan of refetching everything all the time But it doesn't matter for initial roll out on a small number of machines. I'm now more convinces that we should just use puppet for deploying butler runtime. --- Also how does it fit with 'kitchen' stuff that Nodir wrote? Or this is only for legacy buildbot stuff? https://codereview.chromium.org/1501663002/diff/100001/scripts/slave/annotate... File scripts/slave/annotated_run.py (right): https://codereview.chromium.org/1501663002/diff/100001/scripts/slave/annotate... scripts/slave/annotated_run.py:325: bootstrap_dir = ensure_directory(tempdir, 'logdog_bootstrap') it will run bootstrap from scratch each time? :( They aren't very large files, but when >2000 bots fetch them all the time, using single inbound pipe, it adds up. Why not cache them locally between runs? https://codereview.chromium.org/1501663002/diff/100001/scripts/slave/annotate... scripts/slave/annotated_run.py:749: _assert_logdog_whitelisted(properties.get('mastername'), I was confused by this... Do not use exception as a normal flow control. Exceptions should be exceptional. e.g. change it to is_logdog_whitelisted. https://codereview.chromium.org/1501663002/diff/100001/scripts/slave/cipd.py File scripts/slave/cipd.py (right): https://codereview.chromium.org/1501663002/diff/100001/scripts/slave/cipd.py#... scripts/slave/cipd.py:129: help='Add a <package:version> to the CIPD manifest.') wrong help https://codereview.chromium.org/1501663002/diff/100001/scripts/slave/gce.py File scripts/slave/gce.py (right): https://codereview.chromium.org/1501663002/diff/100001/scripts/slave/gce.py#n... scripts/slave/gce.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. most of code in this file is unused. Why is it here?
Agreed that deploying to 2000+ slaves from raw CIPD is not a good idea. Let's see how deployment goes. In the long run this will be Isolated as part of the job description. This sort of emulates that so I would like to at least initially deploy with this sort of setup.' Also FYI since this is annotated_run and Friday, I will not be deploying this CL until Monday :) https://codereview.chromium.org/1501663002/diff/100001/scripts/slave/annotate... File scripts/slave/annotated_run.py (right): https://codereview.chromium.org/1501663002/diff/100001/scripts/slave/annotate... scripts/slave/annotated_run.py:325: bootstrap_dir = ensure_directory(tempdir, 'logdog_bootstrap') On 2016/01/09 01:25:13, Vadim Sh. wrote: > it will run bootstrap from scratch each time? :( They aren't very large files, > but when >2000 bots fetch them all the time, using single inbound pipe, it adds > up. Why not cache them locally between runs? No specific reason other than this is nice and hermetic. In future-land this will probably be isolated (as opposed to machine-native) so less pressure. This sort of emulates isolate setup circumstances it in the meantime. https://codereview.chromium.org/1501663002/diff/100001/scripts/slave/annotate... scripts/slave/annotated_run.py:749: _assert_logdog_whitelisted(properties.get('mastername'), On 2016/01/09 01:25:13, Vadim Sh. wrote: > I was confused by this... Do not use exception as a normal flow control. > Exceptions should be exceptional. e.g. change it to is_logdog_whitelisted. Done. https://codereview.chromium.org/1501663002/diff/100001/scripts/slave/cipd.py File scripts/slave/cipd.py (right): https://codereview.chromium.org/1501663002/diff/100001/scripts/slave/cipd.py#... scripts/slave/cipd.py:129: help='Add a <package:version> to the CIPD manifest.') On 2016/01/09 01:25:13, Vadim Sh. wrote: > wrong help Hah so it is. https://codereview.chromium.org/1501663002/diff/100001/scripts/slave/gce.py File scripts/slave/gce.py (right): https://codereview.chromium.org/1501663002/diff/100001/scripts/slave/gce.py#n... scripts/slave/gce.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. On 2016/01/09 01:25:13, Vadim Sh. wrote: > most of code in this file is unused. Why is it here? iannucci@ suggested I tear this out from depot_tools's gerrit_util.py in an earlier code review *shrug*
Patchset #8 (id:150001) has been deleted
Patchset #8 (id:170001) has been deleted
PTAL, review PS#7 and #8. I had to do some updates to get it to actually work locally, but it does now :) https://codereview.chromium.org/1501663002/diff/190001/scripts/slave/annotate... File scripts/slave/annotated_run.py (right): https://codereview.chromium.org/1501663002/diff/190001/scripts/slave/annotate... scripts/slave/annotated_run.py:341: def _build_logdog_prefix(properties): This translates the master, builder, and build number into a LogDog path. Spec is here: https://github.com/luci/luci-go/blob/master/common/logdog/types/streamname.go... https://codereview.chromium.org/1501663002/diff/190001/scripts/tools/gzjsondu... File scripts/tools/gzjsondump.py (right): https://codereview.chromium.org/1501663002/diff/190001/scripts/tools/gzjsondu... scripts/tools/gzjsondump.py:1: #!/usr/bin/env python I wrote this b/c I am sick of writing one-off scripts to decode build properties GZ. It seems generally useful and low-risk to add, so here it is. I used this a lot when testing the CL.
lgtm https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/slave/a... File scripts/slave/annotated_run.py (right): https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/slave/a... scripts/slave/annotated_run.py:36: LOGDOG_ERROR_RETURNCODES = ( gross :( https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/slave/a... scripts/slave/annotated_run.py:61: )) Join previous line? https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/slave/a... scripts/slave/annotated_run.py:262: return os.path.isfile(path) and os.access(path, os.X_OK) why not just os.access(path, os.X_OK) and catch the exception? It'd be a bit less racy... though this function is probably racy by nature for that reason. https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/slave/a... scripts/slave/annotated_run.py:436: streamserver_uri = _logdog_get_streamserver_uri(rt, plat.streamserver) what cleans this socket file up? https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/slave/a... scripts/slave/annotated_run.py:471: '-tee', this tees everything currently, right? Later we could make it only tee annotations, or something like that? https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/slave/a... scripts/slave/annotated_run.py:834: status = _logdog_bootstrap(rt, opts, tdir, config, properties, cmd) can this ever return None even after running the recipe? https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/slave/c... File scripts/slave/cipd.py (right): https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/slave/c... scripts/slave/cipd.py:26: common.env.Install() can we with this? Just want to make sure it doesn't leak into subprocesses https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/slave/g... File scripts/slave/gce.py (right): https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/slave/g... scripts/slave/gce.py:6: Utilities for interfacing with Google Compute Engine. is this stripped-down? No reason to have code here that's not used. If we need to add something, we can add it later. If you want it to be an exact copy, then let's put it in third_party. https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/slave/g... scripts/slave/gce.py:77: resp = cls._get(cls._ACQUIRE_URL, headers=cls._ACQUIRE_HEADERS) may want to log ("refreshing token") or something https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/tools/g... File scripts/tools/gzjsondump.py (right): https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/tools/g... scripts/tools/gzjsondump.py:1: #!/usr/bin/env python On 2016/01/14 at 22:50:28, dnj wrote: > I wrote this b/c I am sick of writing one-off scripts to decode build properties GZ. It seems generally useful and low-risk to add, so here it is. I used this a lot when testing the CL. k
https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/slave/a... File scripts/slave/annotated_run.py (right): https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/slave/a... scripts/slave/annotated_run.py:36: LOGDOG_ERROR_RETURNCODES = ( On 2016/01/15 04:18:17, iannucci wrote: > gross :( I can't think of a better way to do this part. I suppose I could drop files and delete them? https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/slave/a... scripts/slave/annotated_run.py:61: )) On 2016/01/15 04:18:18, iannucci wrote: > Join previous line? Done. https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/slave/a... scripts/slave/annotated_run.py:262: return os.path.isfile(path) and os.access(path, os.X_OK) On 2016/01/15 04:18:18, iannucci wrote: > why not just os.access(path, os.X_OK) and catch the exception? It'd be a bit > less racy... though this function is probably racy by nature for that reason. Actually apparently nothing uses this anymore. https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/slave/a... scripts/slave/annotated_run.py:436: streamserver_uri = _logdog_get_streamserver_uri(rt, plat.streamserver) On 2016/01/15 04:18:17, iannucci wrote: > what cleans this socket file up? The Butler should delete it, but it's also built within a Runtime-managed temporary directory, so it will be cleaned up on annotated_run.py exit regardless. https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/slave/a... scripts/slave/annotated_run.py:471: '-tee', On 2016/01/15 04:18:17, iannucci wrote: > this tees everything currently, right? Later we could make it only tee > annotations, or something like that? Exactly. https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/slave/a... scripts/slave/annotated_run.py:834: status = _logdog_bootstrap(rt, opts, tdir, config, properties, cmd) On 2016/01/15 04:18:18, iannucci wrote: > can this ever return None even after running the recipe? It currently only raises Exceptions or returns the Butler's return value, so I don't think so. https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/slave/c... File scripts/slave/cipd.py (right): https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/slave/c... scripts/slave/cipd.py:26: common.env.Install() On 2016/01/15 04:18:18, iannucci wrote: > can we with this? Just want to make sure it doesn't leak into subprocesses This is a standalone executable, so this usage should be safe. Actually just scanned through. It looks like nothing is using "chromium_utils" anymore, so I'm going to nuke the path setup. https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/slave/g... File scripts/slave/gce.py (right): https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/slave/g... scripts/slave/gce.py:6: Utilities for interfacing with Google Compute Engine. On 2016/01/15 04:18:18, iannucci wrote: > is this stripped-down? No reason to have code here that's not used. If we need > to add something, we can add it later. If you want it to be an exact copy, then > let's put it in third_party. Stripped it down a bit. https://chromiumcodereview.appspot.com/1501663002/diff/190001/scripts/slave/g... scripts/slave/gce.py:77: resp = cls._get(cls._ACQUIRE_URL, headers=cls._ACQUIRE_HEADERS) On 2016/01/15 04:18:18, iannucci wrote: > may want to log ("refreshing token") or something This was removed as part of the strip.
The CQ bit was checked by dnj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from martiniss@chromium.org, vadimsh@chromium.org, iannucci@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1501663002/#ps210001 (title: "Comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1501663002/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1501663002/210001
Message was sent while issue was closed.
Description was changed from ========== annotated_run.py: Add LogDog bootstrapping. BUG=chromium:550673 TEST=`./scripts/slave/unittests/annotated_run_test.py` ========== to ========== annotated_run.py: Add LogDog bootstrapping. BUG=chromium:550673 TEST=`./scripts/slave/unittests/annotated_run_test.py` Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=298289 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:210001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=298289 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
