Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1869053002:
kitchen_run: initial CL (Closed)
|
Created:
4 years, 8 months ago by Paweł Hajdan Jr. Modified:
4 years, 8 months ago CC:
chromium-reviews, infra-reviews+build_chromium.org, kjellander-cc_chromium.org Target Ref:
refs/heads/master Project:
build Visibility:
Public. |
Descriptionkitchen_run: initial CL
Depends on:
https://codereview.chromium.org/1868553006
https://codereview.chromium.org/1862163004
https://codereview.chromium.org/1884523002
https://codereview.chromium.org/1883503003
https://codereview.chromium.org/1879173003
https://codereview.chromium.org/1881123003
BUG=chromium:593999
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299924
Patch Set 1 #
Total comments: 46
Patch Set 2 : fixes #Patch Set 3 : presubmit #
Total comments: 27
Patch Set 4 : fixes #
Total comments: 15
Patch Set 5 : fixes #Patch Set 6 : fixes #
Messages
Total messages: 47 (15 generated)
phajdan.jr@chromium.org changed reviewers: + dnj@chromium.org, estaab@chromium.org, nodir@chromium.org
Hey folks, this is an early version of my kitchen_run CL. As you see, it has some local hacks I've used for testing. They'll be removed before final review and landing. Please consider this a heads up. Does the approach I've taken in this CL make sense to you? I may extract some parts of it, such as splitting update_scripts.py out of annotated_run.py, and land earlier separately, unless you have concerns. The rationale would be earlier testing and avoiding merge conflicts while iterating on this CL. Any comments are welcome - I'm very interested in your input, and flagging anything that might break.
estaab@chromium.org changed reviewers: + iannucci@chromium.org
[+iannucci] At a high level it looks ok and I had a couple of small comments. One of the others should definitely look too, though. https://codereview.chromium.org/1869053002/diff/1/masters/master.chromium.lin... File masters/master.chromium.linux/master_linux_cfg.py (right): https://codereview.chromium.org/1869053002/diff/1/masters/master.chromium.lin... masters/master.chromium.linux/master_linux_cfg.py:39: 'name': 'Linux Builder', Start with infra? Also separate kitchen_run implementation from configuring anything to use it so any potential rollback CL will be small. https://codereview.chromium.org/1869053002/diff/1/scripts/master/chromium_ste... File scripts/master/chromium_step.py (right): https://codereview.chromium.org/1869053002/diff/1/scripts/master/chromium_ste... scripts/master/chromium_step.py:1065: raise Exception(repr([ What's the format of the regular exception? If you need to override it can you format it so it's clear what the exception, name, and value each are?
Thanks for review, Erik! Both of the things you commented on are the hacks. I don't intend to migrate any builders in the final version (for reasons you've mentioned). FWIW, for deployment, I'd add some FYI builders/trybots in a separate CL.
some comments. I think this is doing too much in one CL. dnj, can you look at the cipd refactor suggestion? nodir, can you look at kitchen tmpfolder management question? https://codereview.chromium.org/1869053002/diff/1/masters/master.chromium.lin... File masters/master.chromium.linux/master_linux_cfg.py (right): https://codereview.chromium.org/1869053002/diff/1/masters/master.chromium.lin... masters/master.chromium.linux/master_linux_cfg.py:14: m_kitchen = kitchen_factory.KitchenFactory( Can we name this something more descriptive? annotator_factory was poorly named (should have been recipes_factory or something). Maybe hermetic_recipes_factory? https://codereview.chromium.org/1869053002/diff/1/masters/master.chromium.lin... masters/master.chromium.linux/master_linux_cfg.py:39: 'name': 'Linux Builder', On 2016/04/08 at 05:22:46, estaab wrote: > Start with infra? Also separate kitchen_run implementation from configuring anything to use it so any potential rollback CL will be small. +1 to both https://codereview.chromium.org/1869053002/diff/1/scripts/master/factory/kitc... File scripts/master/factory/kitchen_factory.py (right): https://codereview.chromium.org/1869053002/diff/1/scripts/master/factory/kitc... scripts/master/factory/kitchen_factory.py:19: self._common_properties = common_properties or {} hm... What do we need this for? This means we have 3 layers of properties: common_properties, factory_properties and build_properties. Could we just get by with factory_properties and a helper function like we do for other Factories? In fact, could we not have KitchenFactory be a class and just have it be a method (e.g. the body of BaseFactory)? So instead of F = KitchenFactory(some, options) buildFactory = F.BaseFactory(other options) It's just buildFactory = KitchenFactory(all, of, the, options) ? And if there are common options, just have a helper in the master def defaultKitchenFactory(some, of, the, options): return KitchenFactory(default, repo, with, some, of, the, options) https://codereview.chromium.org/1869053002/diff/1/scripts/slave/annotated_run.py File scripts/slave/annotated_run.py (right): https://codereview.chromium.org/1869053002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:30: from slave import update_scripts this is a big enough change that it should be a separate CL. https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.py File scripts/slave/kitchen_run.py (right): https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:31: # A CIPD binary description, including the package name and version, and relative hm... why copy+paste this but not copy+paste update scripts? Maybe factor this out into a cipd.py? https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:98: raise LogDogBootstrapError() especially since this is clearly raising copy/pasted exceptions. https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:107: return '/usr/local/google/home/phajdan/luci-go/kitchen' erm... what? https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:137: shutil.rmtree(tempdir) finally clauses like this are unreliable. tempfolder cleanup must happen at the beginning of this script (to clean up previous crashes). Doing it in finally will add another source of out-of-disk errors. It may make sense to build this folder management into the kitchen binary instead? I'm not sure.
First round of comments. https://codereview.chromium.org/1869053002/diff/1/scripts/master/chromium_ste... File scripts/master/chromium_step.py (right): https://codereview.chromium.org/1869053002/diff/1/scripts/master/chromium_ste... scripts/master/chromium_step.py:1065: raise Exception(repr([ On 2016/04/08 05:22:46, estaab wrote: > What's the format of the regular exception? If you need to override it can you > format it so it's clear what the exception, name, and value each are? Why is this here in the first place? Is this debugging? https://codereview.chromium.org/1869053002/diff/1/scripts/master/factory/kitc... File scripts/master/factory/kitchen_factory.py (right): https://codereview.chromium.org/1869053002/diff/1/scripts/master/factory/kitc... scripts/master/factory/kitchen_factory.py:35: #properties.update({'recipe': recipe}) Maybe delete this parameter, since we're not using it? https://codereview.chromium.org/1869053002/diff/1/scripts/slave/annotated_run.py File scripts/slave/annotated_run.py (right): https://codereview.chromium.org/1869053002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:30: from slave import update_scripts On 2016/04/11 18:07:28, iannucci wrote: > this is a big enough change that it should be a separate CL. I'm a bit worried about this, as it makes our fundamental ability to manage bots depend on PYTHONPATH. https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.py File scripts/slave/kitchen_run.py (right): https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:31: # A CIPD binary description, including the package name and version, and relative On 2016/04/11 18:07:28, iannucci wrote: > hm... why copy+paste this but not copy+paste update scripts? Maybe factor this > out into a cipd.py? +1 make these first-level cipd.py constructs. https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:44: if kwargs.pop('dry_run', False): You don't actually use this option. https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:97: LOGGER.exception('Failed to install LogDog CIPD packages.') nit: "Kitchen". https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:127: tempdir = tempfile.mkdtemp(prefix='kitchen_run') I would like you to use a scheme like I did with LogDog. You can borrow it from Runtime.tempdir. This should offer cleanup without accidentally having one run taint another. https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:134: '-properties', json.dumps(args.build_properties), Can we have this get passed to Kitchen as JSON blob? Windows bots have been known to hit command-line length limitations. https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:135: ], cwd=tempdir) Do we actually want to run Kitchen out of a temporary directory? https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:144: if False and update_scripts.update_scripts(): Want to remove "False", no?
https://codereview.chromium.org/1869053002/diff/1/scripts/slave/annotated_run.py File scripts/slave/annotated_run.py (right): https://codereview.chromium.org/1869053002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:30: from slave import update_scripts On 2016/04/11 18:17:15, dnj wrote: > On 2016/04/11 18:07:28, iannucci wrote: > > this is a big enough change that it should be a separate CL. > > I'm a bit worried about this, as it makes our fundamental ability to manage bots > depend on PYTHONPATH. Actually WDYT about making this a standalone executable that we run explicitly rather than a library?
Thanks for the review! Let me clarify - please read my initial reply to estaab - there's no intention of converting anything to kitchen_run in this CL. The master.chromium.linux change is here just for testing, as it seems cumbersome to apply and deapply things like that locally as I test the changes before upload. For the initial round, I was looking for high-level comments, and looks like there are some. Responses inline below - I'd like to understand your suggestions better so I can apply them. https://codereview.chromium.org/1869053002/diff/1/scripts/master/chromium_ste... File scripts/master/chromium_step.py (right): https://codereview.chromium.org/1869053002/diff/1/scripts/master/chromium_ste... scripts/master/chromium_step.py:1065: raise Exception(repr([ On 2016/04/11 at 18:17:15, dnj wrote: > On 2016/04/08 05:22:46, estaab wrote: > > What's the format of the regular exception? If you need to override it can you > > format it so it's clear what the exception, name, and value each are? > > Why is this here in the first place? Is this debugging? "As you see, it has some local hacks I've used for testing. They'll be removed before final review and landing." https://codereview.chromium.org/1869053002/diff/1/scripts/master/factory/kitc... File scripts/master/factory/kitchen_factory.py (right): https://codereview.chromium.org/1869053002/diff/1/scripts/master/factory/kitc... scripts/master/factory/kitchen_factory.py:19: self._common_properties = common_properties or {} On 2016/04/11 at 18:07:28, iannucci wrote: > hm... What do we need this for? This means we have 3 layers of properties: common_properties, factory_properties and build_properties. Could we just get by with factory_properties and a helper function like we do for other Factories? > > In fact, could we not have KitchenFactory be a class and just have it be a method (e.g. the body of BaseFactory)? So instead of > > F = KitchenFactory(some, options) > buildFactory = F.BaseFactory(other options) > > It's just > > buildFactory = KitchenFactory(all, of, the, options) > > ? > > > And if there are common options, just have a helper in the master > > def defaultKitchenFactory(some, of, the, options): > return KitchenFactory(default, repo, with, some, of, the, options) FWIW, this design mirrors AnnotatorFactory, and I see some value in consistency. A use case I had in mind for common_properties was e.g. setting path_config to kitchen. This can be done with the helper as well. I'm fine with changes and can do that in the next patchset. Please let me know if anybody wouldn't like it, so we avoid back-and-forth. https://codereview.chromium.org/1869053002/diff/1/scripts/master/factory/kitc... scripts/master/factory/kitchen_factory.py:35: #properties.update({'recipe': recipe}) On 2016/04/11 at 18:17:15, dnj wrote: > Maybe delete this parameter, since we're not using it? This line will go away in the final version of the CL. Note that we do use recipe parameter below, when invoking kitchen_run.py . https://codereview.chromium.org/1869053002/diff/1/scripts/slave/annotated_run.py File scripts/slave/annotated_run.py (right): https://codereview.chromium.org/1869053002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:30: from slave import update_scripts On 2016/04/11 at 18:18:28, dnj wrote: > On 2016/04/11 18:17:15, dnj wrote: > > On 2016/04/11 18:07:28, iannucci wrote: > > > this is a big enough change that it should be a separate CL. It will be - see my original comment, "I may extract some parts of it, such as splitting update_scripts.py out of annotated_run.py" > > I'm a bit worried about this, as it makes our fundamental ability to manage bots > > depend on PYTHONPATH. Good point. Doesn't it already depend on PYTHONPATH though? If any of these _existing_ imports fail, annotated_run.py will crash before running update_scripts. > Actually WDYT about making this a standalone executable that we run explicitly rather than a library? What do you guys think, especially wrt my above comment? I'm not sure if a separate executable has any advantages, but I could do that if there's a good reason. https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.py File scripts/slave/kitchen_run.py (right): https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:31: # A CIPD binary description, including the package name and version, and relative On 2016/04/11 at 18:17:15, dnj wrote: > On 2016/04/11 18:07:28, iannucci wrote: > > hm... why copy+paste this but not copy+paste update scripts? Maybe factor this > > out into a cipd.py? > > +1 make these first-level cipd.py constructs. Well let me know what you mean by that. Our interface to cipd.py is currently invoking it as a separate process. How would we pass named tuples? https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:44: if kwargs.pop('dry_run', False): On 2016/04/11 at 18:17:15, dnj wrote: > You don't actually use this option. Good point - will remove. https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:97: LOGGER.exception('Failed to install LogDog CIPD packages.') On 2016/04/11 at 18:17:15, dnj wrote: > nit: "Kitchen". Will do. https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:107: return '/usr/local/google/home/phajdan/luci-go/kitchen' On 2016/04/11 at 18:07:28, iannucci wrote: > erm... what? See "As you see, it has some local hacks I've used for testing. They'll be removed before final review and landing." in original message. This was needed before https://codereview.chromium.org/1868553006 and https://codereview.chromium.org/1862163004 landed. https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:127: tempdir = tempfile.mkdtemp(prefix='kitchen_run') On 2016/04/11 at 18:17:15, dnj wrote: > I would like you to use a scheme like I did with LogDog. You can borrow it from Runtime.tempdir. This should offer cleanup without accidentally having one run taint another. Could you explain more? How would one run taint another? Even though prefix is the same, each run gets a different temporary directory. https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:134: '-properties', json.dumps(args.build_properties), On 2016/04/11 at 18:17:15, dnj wrote: > Can we have this get passed to Kitchen as JSON blob? Windows bots have been known to hit command-line length limitations. Good point. Could you explain more? Pass path to a file containing JSON instead? Pass compressed JSON like we do for build properties? https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:135: ], cwd=tempdir) On 2016/04/11 at 18:17:15, dnj wrote: > Do we actually want to run Kitchen out of a temporary directory? I think so. That's what we do with swarmbucket. Why not? https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:137: shutil.rmtree(tempdir) On 2016/04/11 at 18:07:28, iannucci wrote: > finally clauses like this are unreliable. tempfolder cleanup must happen at the beginning of this script (to clean up previous crashes). Doing it in finally will add another source of out-of-disk errors. > > It may make sense to build this folder management into the kitchen binary instead? I'm not sure. Good point. I'll find some way to implement the cleanup. https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:144: if False and update_scripts.update_scripts(): On 2016/04/11 at 18:17:15, dnj wrote: > Want to remove "False", no? "As you see, it has some local hacks I've used for testing. They'll be removed before final review and landing."
https://codereview.chromium.org/1869053002/diff/1/scripts/master/factory/kitc... File scripts/master/factory/kitchen_factory.py (right): https://codereview.chromium.org/1869053002/diff/1/scripts/master/factory/kitc... scripts/master/factory/kitchen_factory.py:12: class KitchenFactory(object): please add a class docstring https://codereview.chromium.org/1869053002/diff/1/scripts/master/factory/kitc... scripts/master/factory/kitchen_factory.py:19: self._common_properties = common_properties or {} On 2016/04/11 20:44:39, Paweł Hajdan Jr. wrote: > On 2016/04/11 at 18:07:28, iannucci wrote: > > hm... What do we need this for? This means we have 3 layers of properties: > common_properties, factory_properties and build_properties. Could we just get by > with factory_properties and a helper function like we do for other Factories? > > > > In fact, could we not have KitchenFactory be a class and just have it be a > method (e.g. the body of BaseFactory)? So instead of > > > > F = KitchenFactory(some, options) > > buildFactory = F.BaseFactory(other options) > > > > It's just > > > > buildFactory = KitchenFactory(all, of, the, options) > > > > ? > > > > > > And if there are common options, just have a helper in the master > > > > def defaultKitchenFactory(some, of, the, options): > > return KitchenFactory(default, repo, with, some, of, the, options) > > FWIW, this design mirrors AnnotatorFactory, and I see some value in consistency. > > A use case I had in mind for common_properties was e.g. setting path_config to > kitchen. This can be done with the helper as well. > > I'm fine with changes and can do that in the next patchset. Please let me know > if anybody wouldn't like it, so we avoid back-and-forth. +1 to what Robbie said. This class has only one method. https://codereview.chromium.org/1869053002/diff/1/scripts/master/factory/kitc... scripts/master/factory/kitchen_factory.py:22: """ first line should be one sentence, then blank line, then the rest https://google.github.io/styleguide/pyguide.html#Comments https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.py File scripts/slave/kitchen_run.py (right): https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:77: if level <= logging.INFO: I'd use simpler == and `verbose = 1` and `verbose = 2` it would be more explicit and same amount of code https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:101: return tuple(pmap[p.package] for p in packages) You could compute path here, no need for pmap var https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:144: if False and update_scripts.update_scripts(): On 2016/04/11 20:44:40, Paweł Hajdan Jr. wrote: > On 2016/04/11 at 18:17:15, dnj wrote: > > Want to remove "False", no? > > "As you see, it has some local hacks I've used for testing. They'll be removed > before final review and landing." I guess it would be easier for reviewers (4) if the uploaded patchset does not include local hacks
Please keep local hacks local, e.g. in a separate branch
Description was changed from ========== kitchen_run: initial CL Depends on: https://codereview.chromium.org/1868553006 https://codereview.chromium.org/1862163004 BUG=chromium:593999 ========== to ========== kitchen_run: initial CL Depends on: https://codereview.chromium.org/1868553006 https://codereview.chromium.org/1862163004 https://codereview.chromium.org/1884523002 https://codereview.chromium.org/1883503003 BUG=chromium:593999 ==========
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1869053002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1869053002/20001
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1869053002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1869053002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Alright folks. This is now ready for the full review. Thanks Robbie for the tempfile suggestion. I didn't consider that, and it's indeed important that we automatically recover from stale temp files, without requiring trooper's intervention. Dan - yeah, I added you because the CIPD code here is similar to LogDog bootstrap code in annotated_run.py . I'm not sure if it's worth sharing much given how little code we have here, and that once we move off buildbot, this code will completely go away as well. I'd be happy to take suggestions - just make sure they're _specific_ enough so that I know how to translate that to changes in the code. Also consider timezone differences and aim at reducing number of round-trips I need to do (compared to how much will they improve code which will go away; consider conditional approval or deferring some changes to future CLs where possible). Thanks! https://codereview.chromium.org/1869053002/diff/1/masters/master.chromium.lin... File masters/master.chromium.linux/master_linux_cfg.py (right): https://codereview.chromium.org/1869053002/diff/1/masters/master.chromium.lin... masters/master.chromium.linux/master_linux_cfg.py:14: m_kitchen = kitchen_factory.KitchenFactory( On 2016/04/11 at 18:07:28, iannucci wrote: > Can we name this something more descriptive? annotator_factory was poorly named (should have been recipes_factory or something). Maybe hermetic_recipes_factory? Well, this runs kitchen so it makes sense for me to name this KitchenFactory. Given we're moving off buildbot, should we spend time bikeshedding on the name? FWIW, if you have a strong opinion, please give me a name and I'll update the CL.
buffering entire output is the main issue. It will OOM pretty quickly. https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... File scripts/slave/kitchen_run.py (right): https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:50: LOGGER.debug('Process [%s] returned [%d] with output:\n%s', First [%s] will render to something like [[cmd, args1, args2]]. Make it %s. https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:56: rv, stdout = _run_command(cmd, **kwargs) s/stdout/output because it includes stderr https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:56: rv, stdout = _run_command(cmd, **kwargs) nit: I assume rv stands for "return value". I think it is too abstract. I'd rename to exit_code or something more concrete than "return value" https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:62: def _install_cipd(path, *packages): rename to _cipd_install or _install_cipd_packages otherwise it sounds like its primary goal is to install cipd client https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:63: """Returns (list): The paths to the binaries in each of the packages. nit: first sentence should say what is the main thing this function does, which is install packages, not return paths https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:70: packages (CipdBinary): The set of CIPD binary packages to install. The type of the parameter is incorrect, should be (list of CipdBinary) https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:72: packages_path = os.path.join(path, 'packages.json') this file is not used. Either use (e.g. by logging its contents) or do not specify in --json-output https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:78: '--verbose', '--verbose', -vv https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:84: return tuple(os.path.join(path, p.relpath) for p in packages) doc says it returns a list, but in fact it returns a tuple. I prefer it to be a list because its length is variable https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:89: parser.add_argument('--repository') required=True https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:91: parser.add_argument('--recipe') required=True https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:112: rv, _ = _run_command([ _run_command buffers entire process output in main memory. Kitchen output size is on the order of hundreds of megabytes and it is not used here, so _run_command is not appropriate here. https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:118: '-properties', json.dumps(args.build_properties), I still see no reason why not to save to a file in this CL. It is not that hard. I am not comfortable approving CL that will quickly hit limits. https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:129: rv, _ = _run_command([sys.executable] + argv) _run_command is not appropriate for this because _run_command buffers output which is going to be enormous https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:136: logging.basicConfig(level=logging.INFO) this should set DEBUG if -verbose is passed, otherwise LOGGER.debug calls above are "local hacks"
https://codereview.chromium.org/1869053002/diff/1/scripts/slave/annotated_run.py File scripts/slave/annotated_run.py (right): https://codereview.chromium.org/1869053002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:30: from slave import update_scripts > Good point. Doesn't it already depend on PYTHONPATH though? If any of these > _existing_ imports fail, annotated_run.py will crash before running > update_scripts. It does. This is something I'm also not super happy about, especially w/ "chromium_utils" import. However, some of this stuff affects PYTHONPATH, meaning that an import here is *more* affected by PYTHONPATH than the simple loading of this script was. > > Actually WDYT about making this a standalone executable that we run explicitly > rather than a library? > > What do you guys think, especially wrt my above comment? I'm not sure if a > separate executable has any advantages, but I could do that if there's a good > reason. I still think this is better than a library. https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.py File scripts/slave/kitchen_run.py (right): https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:31: # A CIPD binary description, including the package name and version, and relative On 2016/04/11 20:44:39, Paweł Hajdan Jr. wrote: > On 2016/04/11 at 18:17:15, dnj wrote: > > On 2016/04/11 18:07:28, iannucci wrote: > > > hm... why copy+paste this but not copy+paste update scripts? Maybe factor > this > > > out into a cipd.py? > > > > +1 make these first-level cipd.py constructs. > > Well let me know what you mean by that. Our interface to cipd.py is currently > invoking it as a separate process. How would we pass named tuples? We'd define them in "cipd.py", then "import cipd" here and use it. "cipd.py" would be both a library and an executable. https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:134: '-properties', json.dumps(args.build_properties), On 2016/04/11 20:44:39, Paweł Hajdan Jr. wrote: > On 2016/04/11 at 18:17:15, dnj wrote: > > Can we have this get passed to Kitchen as JSON blob? Windows bots have been > known to hit command-line length limitations. > > Good point. Could you explain more? Pass path to a file containing JSON instead? > Pass compressed JSON like we do for build properties? Yep. 1) Create JSON blob as an array (or object w/ "args" array field, whatever). 2) Write blob to disk. 3) Run "kitchen", passing it that file. The important difference here, and the reason we zipped properties above, is b/c it did exceed the command-line length on Windows. You're decompressing them, then passing the uncompressed data to kitchen, which revives the problem. https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:135: ], cwd=tempdir) On 2016/04/11 20:44:39, Paweł Hajdan Jr. wrote: > On 2016/04/11 at 18:17:15, dnj wrote: > > Do we actually want to run Kitchen out of a temporary directory? > > I think so. That's what we do with swarmbucket. Why not? CWD means magic things for some processes. No specific reason, as I'm not sure what assumptions Kitchen or its subprocesses make about working directory. Just a note. https://codereview.chromium.org/1869053002/diff/1/scripts/slave/kitchen_run.p... scripts/slave/kitchen_run.py:144: if False and update_scripts.update_scripts(): On 2016/04/11 20:44:40, Paweł Hajdan Jr. wrote: > On 2016/04/11 at 18:17:15, dnj wrote: > > Want to remove "False", no? > > "As you see, it has some local hacks I've used for testing. They'll be removed > before final review and landing." Understood that you have local hacks, but when you don't note them it's difficult to identify which are hacks and which are oversights. Also, it's good to comment regardless in case you forget to remove a specific hack. https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... File scripts/slave/kitchen_run.py (right): https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:78: '--verbose', '--verbose', I don't think you should hardcode kitchen verbosity. If we want that by default, let's just enable that as the Kitchen default. https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:97: # Clean up possible leftover temporary directories (e.g. after a crash), I think this is inferior to the scheme used in "annotated_run.py", which is: 1) Pick a named temporary base (e.g., tempdir/"kitchen_run" 2) Purge (1) on startup. 3) Use mkdtemp to create a subdirectory underneath of (1). 4) Work out of that. 5) Purge (1). This means that any interrupted run will have its directory auto-purged by the next run, and no successive run will accidentally use a previous run's data. This is preferable to globbing because it is unambiguous and completely contained. https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:117: # TODO(phajdan.jr): check for command line length issues on Windows. This is a *known issue* that the GZIP properties were implemented in response to. I don't think it's a good idea to TODO its manifestation here. https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:121: chromium_utils.RemoveDirectory(tempdir) nit: Use a context manager for more elegant expression. with temp_kitchen_dir() as tempdir: # Do stuff. See annotated_run.py. https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:136: logging.basicConfig(level=logging.INFO) On 2016/04/12 17:02:06, nodir wrote: > this should set DEBUG if -verbose is passed, otherwise LOGGER.debug calls above > are "local hacks" IMO it should start as INFO (or even WARNING), then be modified to DEBUG upon command-line argument parsing.
correcting own comments because I was confused by calling stderr "stdout". lgtm % comments https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... File scripts/slave/kitchen_run.py (right): https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:46: kwargs.setdefault('stderr', subprocess.STDOUT) pass STDERR because it is not stdout https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:48: stdout, _ = proc.communicate() _, stderr = proc.communicate() because it is stderr stream https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:50: LOGGER.debug('Process [%s] returned [%d] with output:\n%s', s/output/stderr/ https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:56: rv, stdout = _run_command(cmd, **kwargs) On 2016/04/12 17:02:06, nodir wrote: > s/stdout/output > because it includes stderr s/stdout/stderr https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:59: return stdout do not return it, it is never used https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:78: '--verbose', '--verbose', On 2016/04/12 21:22:21, dnj wrote: > I don't think you should hardcode kitchen verbosity. If we want that by default, > let's just enable that as the Kitchen default. Consider adding -v flag
I mean, lgtm % (my and dnj's comments) because of time-zone differencies
https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... File scripts/slave/kitchen_run.py (right): https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:50: LOGGER.debug('Process [%s] returned [%d] with output:\n%s', On 2016/04/12 at 17:02:06, nodir wrote: > First [%s] will render to something like [[cmd, args1, args2]]. Make it %s. (I would actually change it to %r)
On 2016/04/12 at 21:45:41, iannucci wrote: > https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... > File scripts/slave/kitchen_run.py (right): > > https://codereview.chromium.org/1869053002/diff/40001/scripts/slave/kitchen_r... > scripts/slave/kitchen_run.py:50: LOGGER.debug('Process [%s] returned [%d] with output:\n%s', > On 2016/04/12 at 17:02:06, nodir wrote: > > First [%s] will render to something like [[cmd, args1, args2]]. Make it %s. > > (I would actually change it to %r) (but otherwise lgtm % dnj's and nodir's comments)
Description was changed from ========== kitchen_run: initial CL Depends on: https://codereview.chromium.org/1868553006 https://codereview.chromium.org/1862163004 https://codereview.chromium.org/1884523002 https://codereview.chromium.org/1883503003 BUG=chromium:593999 ========== to ========== kitchen_run: initial CL Depends on: https://codereview.chromium.org/1868553006 https://codereview.chromium.org/1862163004 https://codereview.chromium.org/1884523002 https://codereview.chromium.org/1883503003 https://codereview.chromium.org/1879173003 BUG=chromium:593999 ==========
Description was changed from ========== kitchen_run: initial CL Depends on: https://codereview.chromium.org/1868553006 https://codereview.chromium.org/1862163004 https://codereview.chromium.org/1884523002 https://codereview.chromium.org/1883503003 https://codereview.chromium.org/1879173003 BUG=chromium:593999 ========== to ========== kitchen_run: initial CL Depends on: https://codereview.chromium.org/1868553006 https://codereview.chromium.org/1862163004 https://codereview.chromium.org/1884523002 https://codereview.chromium.org/1883503003 https://codereview.chromium.org/1879173003 https://codereview.chromium.org/1881123003 BUG=chromium:593999 ==========
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1869053002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1869053002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I believe I've addressed your comments. Feel free to take another look and verify. If possible, please let me fix any remaining bits offline (either in this CL with conditional approval, or with a TODO + a follow-up CL). Thank you for your feedback!
Looks mostly good. A few remaining things. https://codereview.chromium.org/1869053002/diff/60001/scripts/slave/kitchen_r... File scripts/slave/kitchen_run.py (right): https://codereview.chromium.org/1869053002/diff/60001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:24: from slave import robust_tempdir This doesn't seem to exist. Where is it? https://codereview.chromium.org/1869053002/diff/60001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:83: _call(cmd) This does not check for success/fail status. It needs to fail if CIPD fails. Maybe you want "check_call"? https://codereview.chromium.org/1869053002/diff/60001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:95: tempdir = rt.tempdir(basedir) "robust_tempdir" should create/return the sub-tempdir automatically, handing you "tempdir". https://codereview.chromium.org/1869053002/diff/60001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:99: properties_file.flush() nit: "flush" is not needed, as it will automatically be flushed when the temporary file is closed. https://codereview.chromium.org/1869053002/diff/60001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:113: parser.add_argument('--repository', required=True) nit: please add help strings for these options. Even basic ones noting expected input type/format would be fine.
https://codereview.chromium.org/1869053002/diff/60001/scripts/slave/kitchen_r... File scripts/slave/kitchen_run.py (right): https://codereview.chromium.org/1869053002/diff/60001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:78: '-vv', pass -vv only if --verbose was passed to kitchen_run https://codereview.chromium.org/1869053002/diff/60001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:83: _call(cmd) why exit_code is not checked?
PTAL https://codereview.chromium.org/1869053002/diff/60001/scripts/slave/kitchen_r... File scripts/slave/kitchen_run.py (right): https://codereview.chromium.org/1869053002/diff/60001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:24: from slave import robust_tempdir On 2016/04/13 at 15:54:23, dnj wrote: > This doesn't seem to exist. Where is it? In scripts/slave. Please see https://codereview.chromium.org/1879173003 . https://codereview.chromium.org/1869053002/diff/60001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:78: '-vv', On 2016/04/13 at 16:01:17, nodir wrote: > pass -vv only if --verbose was passed to kitchen_run Done. https://codereview.chromium.org/1869053002/diff/60001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:83: _call(cmd) On 2016/04/13 at 16:01:18, nodir wrote: > why exit_code is not checked? Good catch. Fixed. https://codereview.chromium.org/1869053002/diff/60001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:95: tempdir = rt.tempdir(basedir) On 2016/04/13 at 15:54:23, dnj wrote: > "robust_tempdir" should create/return the sub-tempdir automatically, handing you "tempdir". Hm, this could be a nice improvement. However, RobustTempdir has been extracted out of annotated_run.py. I don't think changing its interface is in scope of this CL. If you insist, I could add a TODO, although I think there are places in annotated_run which call tempdir multiple times, so I'm not really sure how viable is this idea. https://codereview.chromium.org/1869053002/diff/60001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:99: properties_file.flush() On 2016/04/13 at 15:54:23, dnj wrote: > nit: "flush" is not needed, as it will automatically be flushed when the temporary file is closed. Please note if we let the context manager close it, it will also be deleted. Other places which use this pattern call flush(). I have yet to see how it behaves on Windows, so I may need to change it a bit. For now I believe this is OK. https://codereview.chromium.org/1869053002/diff/60001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:113: parser.add_argument('--repository', required=True) On 2016/04/13 at 15:54:23, dnj wrote: > nit: please add help strings for these options. Even basic ones noting expected input type/format would be fine. Done (copied straight from kitchen help cook).
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1869053002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1869053002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1869053002/diff/60001/scripts/slave/kitchen_r... File scripts/slave/kitchen_run.py (right): https://codereview.chromium.org/1869053002/diff/60001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:95: tempdir = rt.tempdir(basedir) On 2016/04/13 16:25:00, Paweł Hajdan Jr. wrote: > On 2016/04/13 at 15:54:23, dnj wrote: > > "robust_tempdir" should create/return the sub-tempdir automatically, handing > you "tempdir". > > Hm, this could be a nice improvement. > > However, RobustTempdir has been extracted out of annotated_run.py. > > I don't think changing its interface is in scope of this CL. > > If you insist, I could add a TODO, although I think there are places in > annotated_run which call tempdir multiple times, so I'm not really sure how > viable is this idea. Okay now I see the implementation. I hadn't sync'd. This implementation doesn't cover iannucci@'s concern that the tempdir should be cleaned before operating. This is currently a bug in `annoated_run.py`, but doesn't need to be one here, too. I see a TODO in "robust_tempdir", but iannucci@ explicitly named this concern in this CL, so it should be resolved before this CL lands. https://codereview.chromium.org/1869053002/diff/60001/scripts/slave/kitchen_r... scripts/slave/kitchen_run.py:99: properties_file.flush() On 2016/04/13 16:24:59, Paweł Hajdan Jr. wrote: > On 2016/04/13 at 15:54:23, dnj wrote: > > nit: "flush" is not needed, as it will automatically be flushed when the > temporary file is closed. > > Please note if we let the context manager close it, it will also be deleted. > Other places which use this pattern call flush(). > > I have yet to see how it behaves on Windows, so I may need to change it a bit. > For now I believe this is OK. Ah sorry I was off by an indent. One of the main purposes of robust_tempdir is that you don't have to use tempfiles within it. Here, you should just create a named file (e.g., os.path.join(tempdir, "kitchen_properties.json") and pass that to the Kitchen call. It will be cleaned up as part of RobustTempdir "with" cleanup. properties_file = os.path.join(tempdir, "kitchen_properties.json") with open(properties_file, 'w') as fd: json.dump(args.build_properties, fd) return _call([..., '-properties-file', properties_file])
lgtm w/ above comments
Thanks! I believe patchset #6 addresses these comments.
The CQ bit was checked by phajdan.jr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nodir@chromium.org, iannucci@chromium.org, dnj@chromium.org Link to the patchset: https://codereview.chromium.org/1869053002/#ps100001 (title: "fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1869053002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1869053002/100001
Message was sent while issue was closed.
Description was changed from ========== kitchen_run: initial CL Depends on: https://codereview.chromium.org/1868553006 https://codereview.chromium.org/1862163004 https://codereview.chromium.org/1884523002 https://codereview.chromium.org/1883503003 https://codereview.chromium.org/1879173003 https://codereview.chromium.org/1881123003 BUG=chromium:593999 ========== to ========== kitchen_run: initial CL Depends on: https://codereview.chromium.org/1868553006 https://codereview.chromium.org/1862163004 https://codereview.chromium.org/1884523002 https://codereview.chromium.org/1883503003 https://codereview.chromium.org/1879173003 https://codereview.chromium.org/1881123003 BUG=chromium:593999 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299924 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=299924 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
