Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1178733003:
Modify annotated_run to read factory configs from the slave checkout. (Closed)
|
Created:
5 years, 6 months ago by Dirk Pranke Modified:
5 years, 6 months ago CC:
Paweł Hajdan Jr., chromium-reviews, kjellander-cc_chromium.org, stip+watch_chromium.org, ghost stip (do not use) Target Ref:
refs/heads/master Project:
build Visibility:
Public. |
DescriptionModify annotated_run to read factory configs from the slave checkout.
Currently if you wish to change the recipe a builder uses, or
change the other factory properties associated with a builder,
you need to restart the master.
This change modifies annotated_run.py so that instead of taking
the information the master hands us, we ignore it and use the
copy of the master config we find in the slave checkout, which
should be up-to-date.
This should allow us to eliminate a decent number of master restarts.
(Though it would still be a good idea to restart the masters
periodically so that they don't get too stale).
R=iannucci@chromium.org, phajdan.jr@chromium.org, luqui@chromium.org
BUG=499071
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=295684
Patch Set 1 #
Total comments: 14
Patch Set 2 : update w/ minor fixes from review feedback #Patch Set 3 : rework to use scripts/tools/dump_master_cfg #Patch Set 4 : add --override-lookup to run_recipe and annotated_run #
Total comments: 7
Patch Set 5 : fix logic, rework code to be more readable (and rename new flag) #
Total comments: 11
Patch Set 6 : fix lint error, merge to r295681 #Messages
Total messages: 41 (8 generated)
https://codereview.chromium.org/1178733003/diff/1/scripts/slave/dump_factory_... File scripts/slave/dump_factory_properties.py (right): https://codereview.chromium.org/1178733003/diff/1/scripts/slave/dump_factory_... scripts/slave/dump_factory_properties.py:1: #!/usr/bin/python probably want to put this in scripts/tools https://codereview.chromium.org/1178733003/diff/1/scripts/slave/dump_factory_... scripts/slave/dump_factory_properties.py:37: result = 0 maybe 'ok' and make it a bool? https://codereview.chromium.org/1178733003/diff/1/scripts/slave/dump_factory_... scripts/slave/dump_factory_properties.py:48: }, sort_keys=2, indent=2) while 'sort_keys=2' technically works, you probably want True :) https://codereview.chromium.org/1178733003/diff/1/scripts/slave/dump_factory_... scripts/slave/dump_factory_properties.py:54: print output_str consider outfile = sys.stdout if args.output: outfile = open(args.output. 'w') with outfile: json.dump({...}, outfile) https://codereview.chromium.org/1178733003/diff/1/scripts/slave/dump_factory_... scripts/slave/dump_factory_properties.py:67: raise Err('master "%s" not found.' % mastername) tbh... I would probably just raise Exception (since you catch it anyway) https://codereview.chromium.org/1178733003/diff/1/scripts/slave/dump_factory_... scripts/slave/dump_factory_properties.py:85: from twisted.application import service dare I ask?
https://codereview.chromium.org/1178733003/diff/1/scripts/slave/dump_factory_... File scripts/slave/dump_factory_properties.py (right): https://codereview.chromium.org/1178733003/diff/1/scripts/slave/dump_factory_... scripts/slave/dump_factory_properties.py:1: #!/usr/bin/python On 2015/06/11 00:28:08, iannucci wrote: > probably want to put this in scripts/tools ok. https://codereview.chromium.org/1178733003/diff/1/scripts/slave/dump_factory_... scripts/slave/dump_factory_properties.py:37: result = 0 On 2015/06/11 00:28:08, iannucci wrote: > maybe 'ok' and make it a bool? Actually, looking at this now, it might be better to re-use more of the JSON format Paweł uses for tests ..., so 'message' would be replaced by 'failures': ['...'], and we'd get rid of the result key. https://codereview.chromium.org/1178733003/diff/1/scripts/slave/dump_factory_... scripts/slave/dump_factory_properties.py:48: }, sort_keys=2, indent=2) On 2015/06/11 00:28:08, iannucci wrote: > while 'sort_keys=2' technically works, you probably want True :) whoops, yes. https://codereview.chromium.org/1178733003/diff/1/scripts/slave/dump_factory_... scripts/slave/dump_factory_properties.py:54: print output_str On 2015/06/11 00:28:07, iannucci wrote: > consider > > outfile = sys.stdout > if args.output: > outfile = open(args.output. 'w') > > with outfile: > json.dump({...}, outfile) > That will close sys.stdout() at the end, which might technically correct but is also kinda weird. https://codereview.chromium.org/1178733003/diff/1/scripts/slave/dump_factory_... scripts/slave/dump_factory_properties.py:67: raise Err('master "%s" not found.' % mastername) On 2015/06/11 00:28:08, iannucci wrote: > tbh... I would probably just raise Exception (since you catch it anyway) Yeah, true, this is left over when I thought I would treat user-level errors differently from runtime errors like write failing. https://codereview.chromium.org/1178733003/diff/1/scripts/slave/dump_factory_... scripts/slave/dump_factory_properties.py:85: from twisted.application import service On 2015/06/11 00:28:08, iannucci wrote: > dare I ask? whoops, bad cut and paste, I think.
forgot to re-upload?
phajdan.jr@chromium.org changed reviewers: + phajdan.jr@chromium.org
https://codereview.chromium.org/1178733003/diff/1/scripts/slave/annotated_run.py File scripts/slave/annotated_run.py (right): https://codereview.chromium.org/1178733003/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:36: buildername = factory_properties['buildername'] How does this interact with e.g. running a recipe locally with run_recipe.py with a different mastername and buildername than in buildbot config (or just without these properties) for testing? https://codereview.chromium.org/1178733003/diff/1/scripts/slave/dump_factory_... File scripts/slave/dump_factory_properties.py (right): https://codereview.chromium.org/1178733003/diff/1/scripts/slave/dump_factory_... scripts/slave/dump_factory_properties.py:69: config = master_cfg_utils.LoadConfig(master_path, 'master.cfg') I think we have a very similar existing tool build/scripts/tools/dump_master_cfg.py Please consider extending it rather than writing yet another one.
On 2015/06/11 07:34:20, iannucci wrote: > forgot to re-upload? no, just haven't gotten to it yet :)
On 2015/06/11 08:38:19, Paweł Hajdan Jr. wrote: > https://codereview.chromium.org/1178733003/diff/1/scripts/slave/annotated_run.py > File scripts/slave/annotated_run.py (right): > > https://codereview.chromium.org/1178733003/diff/1/scripts/slave/annotated_run... > scripts/slave/annotated_run.py:36: buildername = > factory_properties['buildername'] > How does this interact with e.g. running a recipe locally with run_recipe.py > with a different mastername and buildername than in buildbot config (or just > without these properties) for testing? annotated_run calls run_recipe, so I would expect run_recipe to be unaffected. I will double-check that running buildbots locally w/ TESTING_MASTER_HOST etc. still works (I would expect it to, since that's just change mastername and buildername, which we still get from the master). > > https://codereview.chromium.org/1178733003/diff/1/scripts/slave/dump_factory_... > File scripts/slave/dump_factory_properties.py (right): > > https://codereview.chromium.org/1178733003/diff/1/scripts/slave/dump_factory_... > scripts/slave/dump_factory_properties.py:69: config = > master_cfg_utils.LoadConfig(master_path, 'master.cfg') > I think we have a very similar existing tool > build/scripts/tools/dump_master_cfg.py > > Please consider extending it rather than writing yet another one. I will take a look, thanks!
On 2015/06/11 08:38:19, Paweł Hajdan Jr. wrote: > https://codereview.chromium.org/1178733003/diff/1/scripts/slave/annotated_run.py > File scripts/slave/annotated_run.py (right): > > https://codereview.chromium.org/1178733003/diff/1/scripts/slave/annotated_run... > scripts/slave/annotated_run.py:36: buildername = > factory_properties['buildername'] > How does this interact with e.g. running a recipe locally with run_recipe.py > with a different mastername and buildername than in buildbot config (or just > without these properties) for testing? > I would guess that we'll only try the magic lookup if both mastername and buildername are set? We'll need to do a merge like: properties = passed_in_properties.update(magic_properties) So then you could do: run_recipe noexist mastername=something buildername=something_else and then it'll do exactly what that builder would do (sans build-properties). Maybe we should allow run_recipe to have the recipe name omitted if mastername+buildername are both specified?
On 2015/06/11 16:41:14, iannucci wrote: > On 2015/06/11 08:38:19, Paweł Hajdan Jr. wrote: > > > https://codereview.chromium.org/1178733003/diff/1/scripts/slave/annotated_run.py > > File scripts/slave/annotated_run.py (right): > > > > > https://codereview.chromium.org/1178733003/diff/1/scripts/slave/annotated_run... > > scripts/slave/annotated_run.py:36: buildername = > > factory_properties['buildername'] > > How does this interact with e.g. running a recipe locally with run_recipe.py > > with a different mastername and buildername than in buildbot config (or just > > without these properties) for testing? > > > > I would guess that we'll only try the magic lookup if both mastername and > buildername are set? We'll need to do a merge like: > > properties = passed_in_properties.update(magic_properties) > > So then you could do: > > run_recipe noexist mastername=something buildername=something_else > > and then it'll do exactly what that builder would do (sans build-properties). > Maybe we should allow run_recipe to have the recipe name omitted if > mastername+buildername are both specified? Note that if we have explicit input signatures for recipes (as opposed to properties), we could also do things like display help for recipes stating what parameters are needed, and aborting early if you omit a required parameter.
On 2015/06/11 16:40:50, Dirk Pranke wrote: > annotated_run calls run_recipe, so I would expect run_recipe to be unaffected. Oh, actually, I see that it's the other way around. Hm. I need to think about iannucci's comments a bit more, then.
On 2015/06/11 17:03:32, Dirk Pranke wrote: > On 2015/06/11 16:40:50, Dirk Pranke wrote: > > annotated_run calls run_recipe, so I would expect run_recipe to be unaffected. > > Oh, actually, I see that it's the other way around. Hm. I need to think > about iannucci's comments a bit more, then. Seems like there's a couple options. One is to make mastername and buildername required, and make the recipe optional (or remove it completely) from run_recipe, but admittedly at that point the name would be kinda misleading (a run_recipe that doesn't take a recipe?) Another would be to add a flag to indicate that the recipe should be derived from the mastername and buildername, and have that flag by on by default when invoked on the slave (or, conversely, to have a flag to bypass the lookup). A third would be to refactor things between run_recipe and annotated run so that we have two different code paths (e.g., a "run_annotated_run" script that was different from "run_recipe" in some way. I kinda like the first idea the most; it's the most awkward for someone who wants to manually test a recipe (they have to change things in the master configs to test them), but it doesn't introduce a new code path and it does reflect what the bots would be doing. But, there is definitely some value in being able to test things directly and bypass the lookup. I don't think I have a strong opinion here, and I'm the least vested in this code. You guys figure out what you'd like to happen and I'll implement that?
On 2015/06/11 20:43:23, Dirk Pranke wrote:
> On 2015/06/11 17:03:32, Dirk Pranke wrote:
> > On 2015/06/11 16:40:50, Dirk Pranke wrote:
> > > annotated_run calls run_recipe, so I would expect run_recipe to be
> unaffected.
> >
> > Oh, actually, I see that it's the other way around. Hm. I need to think
> > about iannucci's comments a bit more, then.
>
> Seems like there's a couple options.
>
> One is to make mastername and buildername required, and make the recipe
optional
> (or remove it completely) from run_recipe,
> but admittedly at that point the name would be kinda misleading (a run_recipe
> that doesn't take a recipe?)
>
> Another would be to add a flag to indicate that the recipe should be derived
> from the mastername and buildername, and have
> that flag by on by default when invoked on the slave (or, conversely, to have
a
> flag to bypass the lookup).
>
> A third would be to refactor things between run_recipe and annotated run so
that
> we have two different code paths (e.g.,
> a "run_annotated_run" script that was different from "run_recipe" in some way.
>
> I kinda like the first idea the most; it's the most awkward for someone who
> wants to manually test a recipe (they have to
> change things in the master configs to test them), but it doesn't introduce a
> new code path and it does reflect what the
> bots would be doing.
>
> But, there is definitely some value in being able to test things directly and
> bypass the lookup.
>
> I don't think I have a strong opinion here, and I'm the least vested in this
> code. You guys figure out what you'd like to
> happen and I'll implement that?
So eventually ("soon") we won't have masters or builders :)
I think what I would prefer is option 2-ish
annotated_run --force-recipe <blah> --*-properties {...}
* do lookup (always). If mastername/buildername are not present but recipe
is, skip the lookup entirely
* if looked-up recipe != force-recipe, print a warning
run_recipe <recipe> ...
* turns into annotated_run --force-recipe <recipe> ...
run_recipe ...
* turns into an annotated_run ...
So:
* don't force mastername/buildername (as we're moving away from that
eventually)
* but if you do have mastername/buildername, do consistency check
* can do buildbot-like flow with run_recipe as well
* but can override if you want to do a 'what if' run
WDYT?
On 2015/06/11 21:03:28, iannucci wrote:
> On 2015/06/11 20:43:23, Dirk Pranke wrote:
> > On 2015/06/11 17:03:32, Dirk Pranke wrote:
> > > On 2015/06/11 16:40:50, Dirk Pranke wrote:
> > > > annotated_run calls run_recipe, so I would expect run_recipe to be
> > unaffected.
> > >
> > > Oh, actually, I see that it's the other way around. Hm. I need to think
> > > about iannucci's comments a bit more, then.
> >
> > Seems like there's a couple options.
> >
> > One is to make mastername and buildername required, and make the recipe
> optional
> > (or remove it completely) from run_recipe,
> > but admittedly at that point the name would be kinda misleading (a
run_recipe
> > that doesn't take a recipe?)
> >
> > Another would be to add a flag to indicate that the recipe should be derived
> > from the mastername and buildername, and have
> > that flag by on by default when invoked on the slave (or, conversely, to
have
> a
> > flag to bypass the lookup).
> >
> > A third would be to refactor things between run_recipe and annotated run so
> that
> > we have two different code paths (e.g.,
> > a "run_annotated_run" script that was different from "run_recipe" in some
way.
> >
> > I kinda like the first idea the most; it's the most awkward for someone who
> > wants to manually test a recipe (they have to
> > change things in the master configs to test them), but it doesn't introduce
a
> > new code path and it does reflect what the
> > bots would be doing.
> >
> > But, there is definitely some value in being able to test things directly
and
> > bypass the lookup.
> >
> > I don't think I have a strong opinion here, and I'm the least vested in this
> > code. You guys figure out what you'd like to
> > happen and I'll implement that?
>
> So eventually ("soon") we won't have masters or builders :)
>
> I think what I would prefer is option 2-ish
>
> annotated_run --force-recipe <blah> --*-properties {...}
> * do lookup (always). If mastername/buildername are not present but recipe
> is, skip the lookup entirely
> * if looked-up recipe != force-recipe, print a warning
> run_recipe <recipe> ...
> * turns into annotated_run --force-recipe <recipe> ...
> run_recipe ...
> * turns into an annotated_run ...
>
> So:
> * don't force mastername/buildername (as we're moving away from that
> eventually)
> * but if you do have mastername/buildername, do consistency check
> * can do buildbot-like flow with run_recipe as well
> * but can override if you want to do a 'what if' run
>
> WDYT?
Sounds reasonable. I will implement something and post it for review.
Okay, this is re-worked to use the --override-lookup approach as discussed, and to use the pre-existing scripts/tools/dump_master_cfg.py that I didn't know about earlier. I haven't yet tested this, but otherwise I think it should be more-or-less what we want. Please take another look? https://codereview.chromium.org/1178733003/diff/60001/scripts/slave/annotated... File scripts/slave/annotated_run.py (right): https://codereview.chromium.org/1178733003/diff/60001/scripts/slave/annotated... scripts/slave/annotated_run.py:49: (properties['recipe'], factory_properties['recipe'])) I don't warn about other factory properties also overriding, but I'm a little worried that people might see this and not realize that the other properties will *also* override. I know that it's worth enumerating all of the mismatches, but I could do that. We could also just print a generic message and not any of the details. Thoughts?
lg, though I don't think the new lookup logic will WAI https://codereview.chromium.org/1178733003/diff/60001/scripts/slave/annotated... File scripts/slave/annotated_run.py (right): https://codereview.chromium.org/1178733003/diff/60001/scripts/slave/annotated... scripts/slave/annotated_run.py:49: (properties['recipe'], factory_properties['recipe'])) On 2015/06/11 22:40:21, Dirk Pranke wrote: > I don't warn about other factory properties also overriding, but I'm a little > worried that people might see this and not realize that the > other properties will *also* override. > > I know that it's worth enumerating all of the mismatches, but I could do that. > > We could also just print a generic message and not any of the details. > > Thoughts? I'd err on the side of enumerating the mismatches just to avoid headscratchers. Should be able to do for key in (set(properties) | set(factory_properties)): etc. https://codereview.chromium.org/1178733003/diff/60001/scripts/slave/annotated... scripts/slave/annotated_run.py:81: builder_dict['factory'].properties.asList()) hm... isn't config json though? I don't think there's going to be any asList https://codereview.chromium.org/1178733003/diff/60001/scripts/slave/annotated... scripts/slave/annotated_run.py:120: 'properties in the master.cfg') IIRC, run_recipe passes all args as build properties, right?
https://codereview.chromium.org/1178733003/diff/60001/scripts/slave/annotated... File scripts/slave/annotated_run.py (right): https://codereview.chromium.org/1178733003/diff/60001/scripts/slave/annotated... scripts/slave/annotated_run.py:49: (properties['recipe'], factory_properties['recipe'])) Ok. It's easy enough to do. https://codereview.chromium.org/1178733003/diff/60001/scripts/slave/annotated... scripts/slave/annotated_run.py:81: builder_dict['factory'].properties.asList()) On 2015/06/11 23:01:49, iannucci wrote: > hm... isn't config json though? I don't think there's going to be any asList Yeah, that's probably true. I need to run this, because I'm not sure whether the properties will contain the third, 'source' parameter or not as part of the serialized info. Hopefully it doesn't, and we can use the dict as-is. (will verify). https://codereview.chromium.org/1178733003/diff/60001/scripts/slave/annotated... scripts/slave/annotated_run.py:120: 'properties in the master.cfg') On 2015/06/11 23:01:49, iannucci wrote: > IIRC, run_recipe passes all args as build properties, right? looks like run_recipe passes all args as both, just to be confusing :).
On 2015/06/11 23:22:47, Dirk Pranke wrote: > https://codereview.chromium.org/1178733003/diff/60001/scripts/slave/annotated... > File scripts/slave/annotated_run.py (right): > > https://codereview.chromium.org/1178733003/diff/60001/scripts/slave/annotated... > scripts/slave/annotated_run.py:49: (properties['recipe'], > factory_properties['recipe'])) > Ok. It's easy enough to do. > > https://codereview.chromium.org/1178733003/diff/60001/scripts/slave/annotated... > scripts/slave/annotated_run.py:81: builder_dict['factory'].properties.asList()) > On 2015/06/11 23:01:49, iannucci wrote: > > hm... isn't config json though? I don't think there's going to be any asList > > Yeah, that's probably true. I need to run this, because I'm not sure whether the > properties will contain the third, 'source' parameter or not as part of the > serialized info. Hopefully it doesn't, and we can use the dict as-is. > > (will verify). > > https://codereview.chromium.org/1178733003/diff/60001/scripts/slave/annotated... > scripts/slave/annotated_run.py:120: 'properties in the master.cfg') > On 2015/06/11 23:01:49, iannucci wrote: > > IIRC, run_recipe passes all args as build properties, right? > > looks like run_recipe passes all args as both, just to be confusing :). I think there's historical reasons there.... I think some properties are interpreted differently depending on if they come from fp or bp (to be compatible with the way buildbot does them). This is why api.properties is just one merged view so we don't have to keep doing that madness :P
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/1178733003/diff/120001/scripts/slave/annotate... File scripts/slave/annotated_run.py (right): https://codereview.chromium.org/1178733003/diff/120001/scripts/slave/annotate... scripts/slave/annotated_run.py:52: slave_properties[name]) If the master has a property set and the slave doesn't, should we use the master's version in the non-override case? it might have been removed in a newer version of the master.cfg. The code will currently use the master's version, i.e., it doesn't delete keys that the slave doesn't have. Obviously we would only want to do this for factory properties, not build properties, but in run_recipe.py, at least, we can't tell which is which, since we use the same dict for both. https://codereview.chromium.org/1178733003/diff/120001/scripts/slave/annotate... scripts/slave/annotated_run.py:77: master_path = path it's a bit lame to have to import master_cfg_utils and do this lookup; perhaps we should modify dump_master_cfg to do this instead?
Okay, tested and cleaned up. Please take another look?
https://codereview.chromium.org/1178733003/diff/120001/scripts/slave/annotate... File scripts/slave/annotated_run.py (right): https://codereview.chromium.org/1178733003/diff/120001/scripts/slave/annotate... scripts/slave/annotated_run.py:52: slave_properties[name]) On 2015/06/12 at 03:01:53, Dirk Pranke wrote: > If the master has a property set and the slave doesn't, should we use the master's version in the non-override case? it might have been removed in a newer version of the master.cfg. > > The code will currently use the master's version, i.e., it doesn't delete keys that the slave doesn't have. Obviously we would only want to do this for factory properties, not build properties, but in run_recipe.py, at least, we can't tell which is which, since we use the same dict for both. Good point. How about making this a _complete_ override, i.e. if --master-overrides-slave is passed, do not even attempt to read the values on the slave? Just take everything from the "master", in this case run_recipe.py command line. https://codereview.chromium.org/1178733003/diff/120001/scripts/slave/annotate... scripts/slave/annotated_run.py:77: master_path = path On 2015/06/12 at 03:01:53, Dirk Pranke wrote: > it's a bit lame to have to import master_cfg_utils and do this lookup; perhaps we should modify dump_master_cfg to do this instead? I'd be fine with that. https://codereview.chromium.org/1178733003/diff/120001/scripts/tools/run_reci... File scripts/tools/run_recipe.py (right): https://codereview.chromium.org/1178733003/diff/120001/scripts/tools/run_reci... scripts/tools/run_recipe.py:78: parser.add_argument('--master-overrides-slave', action='store_true') FWIW, don't we _always_ want this with run_recipe? What would be an example case where other behavior would be preferred or desirable?
If this basically looks good, I'd like to land this as-is so we can unblock the other things I need to be working on (switching the GN bots to the chromium recipes), and we can continue to iterate on this as we figure out how we'd really like things to work. Does that sound okay? https://codereview.chromium.org/1178733003/diff/120001/scripts/slave/annotate... File scripts/slave/annotated_run.py (right): https://codereview.chromium.org/1178733003/diff/120001/scripts/slave/annotate... scripts/slave/annotated_run.py:52: slave_properties[name]) On 2015/06/12 09:24:11, Paweł Hajdan Jr. wrote: > On 2015/06/12 at 03:01:53, Dirk Pranke wrote: > > If the master has a property set and the slave doesn't, should we use the > master's version in the non-override case? it might have been removed in a newer > version of the master.cfg. > > > > The code will currently use the master's version, i.e., it doesn't delete keys > that the slave doesn't have. Obviously we would only want to do this for factory > properties, not build properties, but in run_recipe.py, at least, we can't tell > which is which, since we use the same dict for both. > > Good point. How about making this a _complete_ override, i.e. if > --master-overrides-slave is passed, do not even attempt to read the values on > the slave? Just take everything from the "master", in this case run_recipe.py > command line. That's actually the opposite of what I'm worried about; I'm worried about how to have the slave override the master and delete a key. I'm not sure in what situation we'd want to do what you're suggesting; I think it would be only in the case where the slave's version sets a property value but you want to test what happens when it isn't set. I'm a bit concerned at this point that we're just guessing as to what we might need to do, and may end up overdesigning something. https://codereview.chromium.org/1178733003/diff/120001/scripts/slave/annotate... scripts/slave/annotated_run.py:77: master_path = path On 2015/06/12 09:24:11, Paweł Hajdan Jr. wrote: > On 2015/06/12 at 03:01:53, Dirk Pranke wrote: > > it's a bit lame to have to import master_cfg_utils and do this lookup; perhaps > we should modify dump_master_cfg to do this instead? > > I'd be fine with that. Okay. I think I'd like to make that change in a follow-up CL if that's okay. https://codereview.chromium.org/1178733003/diff/120001/scripts/tools/run_reci... File scripts/tools/run_recipe.py (right): https://codereview.chromium.org/1178733003/diff/120001/scripts/tools/run_reci... scripts/tools/run_recipe.py:78: parser.add_argument('--master-overrides-slave', action='store_true') On 2015/06/12 09:24:11, Paweł Hajdan Jr. wrote: > FWIW, don't we _always_ want this with run_recipe? What would be an example case > where other behavior would be preferred or desirable? If you want to reproduce what is actually happening on the bot, you don't want this flag, because you want the slave's checkout to override what the master passed in. If you want to test new changes, you probably do want this flag. I'm not sure which case is more common. If we modify the command that we print out in the build log to show the values we actually used rather than the values passed to run_recipe, then the first case probably goes away.
lgtm https://chromiumcodereview.appspot.com/1178733003/diff/120001/scripts/slave/a... File scripts/slave/annotated_run.py (right): https://chromiumcodereview.appspot.com/1178733003/diff/120001/scripts/slave/a... scripts/slave/annotated_run.py:41: buildername = master_properties['buildername'] these may not be set if we're doing run_recipe (i.e. use .get() ) https://chromiumcodereview.appspot.com/1178733003/diff/120001/scripts/slave/a... scripts/slave/annotated_run.py:52: slave_properties[name]) On 2015/06/12 15:42:37, Dirk Pranke wrote: > On 2015/06/12 09:24:11, Paweł Hajdan Jr. wrote: > > On 2015/06/12 at 03:01:53, Dirk Pranke wrote: > > > If the master has a property set and the slave doesn't, should we use the > > master's version in the non-override case? it might have been removed in a > newer > > version of the master.cfg. > > > > > > The code will currently use the master's version, i.e., it doesn't delete > keys > > that the slave doesn't have. Obviously we would only want to do this for > factory > > properties, not build properties, but in run_recipe.py, at least, we can't > tell > > which is which, since we use the same dict for both. > > > > Good point. How about making this a _complete_ override, i.e. if > > --master-overrides-slave is passed, do not even attempt to read the values on > > the slave? Just take everything from the "master", in this case run_recipe.py > > command line. > > That's actually the opposite of what I'm worried about; I'm worried about how > to have the slave override the master and delete a key. > > I'm not sure in what situation we'd want to do what you're suggesting; I think > it would be only in the case where the slave's version sets a property value but > you want to test what happens when it isn't set. > > I'm a bit concerned at this point that we're just guessing as to what we might > need to do, and may end up overdesigning something. It may be worth it to just move this logic to run_recipe then (in another CL). Have a --lookup option which gets passed through to annotated_run, and then we know that the properties on the command line are meant to be overrides of what's in the factory. Doing a key deletion could be specified by `key=`. https://chromiumcodereview.appspot.com/1178733003/diff/120001/scripts/slave/a... scripts/slave/annotated_run.py:77: master_path = path On 2015/06/12 15:42:37, Dirk Pranke wrote: > On 2015/06/12 09:24:11, Paweł Hajdan Jr. wrote: > > On 2015/06/12 at 03:01:53, Dirk Pranke wrote: > > > it's a bit lame to have to import master_cfg_utils and do this lookup; > perhaps > > we should modify dump_master_cfg to do this instead? > > > > I'd be fine with that. > > Okay. I think I'd like to make that change in a follow-up CL if that's okay. sgtm
LGTM
The CQ bit was checked by dpranke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1178733003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: build_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/build_presubmit...)
The CQ bit was checked by dpranke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from iannucci@chromium.org, phajdan.jr@chromium.org Link to the patchset: https://codereview.chromium.org/1178733003/#ps140001 (title: "fix lint error, merge to r295681")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1178733003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: build_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/build_presubmit...)
Message was sent while issue was closed.
Committed patchset #6 (id:140001) manually as 295684.
Message was sent while issue was closed.
On 2015/06/15 21:33:38, Dirk Pranke wrote: > Committed patchset #6 (id:140001) manually as 295684. (bypassing the broken presubmit check caused by https://codereview.chromium.org/1185693002 ).
Message was sent while issue was closed.
On 2015/06/15 21:34:59, Dirk Pranke wrote: > On 2015/06/15 21:33:38, Dirk Pranke wrote: > > Committed patchset #6 (id:140001) manually as 295684. > > (bypassing the broken presubmit check caused by > https://codereview.chromium.org/1185693002 ). I suspect it broke the world, e.g. https://build.chromium.org/p/chromium.infra.cron/builders/gnumbd/builds/7547/...
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:140001) has been created in https://codereview.chromium.org/1183223004/ by dpranke@chromium.org. The reason for reverting is: Reverting, probably broke things..
Message was sent while issue was closed.
@vadimsh: I believe you're right; I'm on it.
Message was sent while issue was closed.
On 2015/06/15 21:45:51, Dirk Pranke wrote: > @vadimsh: I believe you're right; I'm on it. I thought I had fixed that issue; I will take another look. |
