Add support for "additional_mirrors" property in trybot configs.

Add support for "additional_mirrors" property in trybot configs.

This allows a single trybot to mirror multiple waterfall bots, so that
it can build the binaries once, but spawn all of the tests (typically
via swarming) that each of those waterfall bots would run.

This will be used to pull in the tests from the chromium.gpu waterfall
onto a few trybot configurations (linux_chromium_rel_ng,
mac_chromium_rel_ng, win_chromium_rel_ng), which is currently being done
manually in the chromium_trybot recipe.

BUG=573337

[Ken Russell (switch to Gerrit), 2016-01-05 04:52:52]

kbr@chromium.org changed reviewers:
+ phajdan.jr@chromium.org, sergeyberezin@chromium.org, sergiyb@chromium.org,
tandrii@chromium.org

[Ken Russell (switch to Gerrit), 2016-01-05 04:52:52]

Please provide your feedback on the general direction of this CL. I haven't
checked the recipe simulation changes in detail so it's very possible I broke
things.

The change to scripts/slave/recipe_modules/chromium_tests/trybots.py shows the
high-level intent.

Thanks.

[luqui, 2016-01-05 23:02:35]

luqui@chromium.org changed reviewers:
+ luqui@chromium.org

[luqui, 2016-01-05 23:02:36]

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/api.py (right):

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/api.py:138:
self.start_test_run_for_bisect(api, update_step, master_dicts[0],
This [0] is suspicious.  Is it clear during construction that the first element
is distinguished in this way (or does it not matter for some reason)?

[Ken Russell (switch to Gerrit), 2016-01-06 00:38:24]

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/api.py (right):

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/api.py:138:
self.start_test_run_for_bisect(api, update_step, master_dicts[0],
On 2016/01/05 23:02:35, luqui wrote:
> This [0] is suspicious.  Is it clear during construction that the first
element
> is distinguished in this way (or does it not matter for some reason)?

No, I didn't do a good job documenting the return values from
api.chromium_tests.prepare_checkout. I agree this approach is pretty hacky.
Basically, I found it necessary to return multiple master_dicts and test_specs
from prepare_checkout, so both return values are arrays now. The first entry in
each array is what would previously have been returned in master_dict and
test_spec. Subsequent entries correspond to the "additional_mirrors" entry in
trybots.py.

I may have missed something major in the design here, but it does seem to me
that it's necessary to return both multiple master_dict and test_spec instances.
However, api.chromium_tests.get_master_dict_with_dynamic_tests does some work
which makes me wonder if all the data in test_spec is actually embedded already
in the returned master_dict instance.

[Sergey Berezin, 2016-01-06 01:50:56]

I have a comment on the design - see below. Also, if you can wait, I'd let Pawel
review it too when he's back.

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/api.py (right):

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/api.py:138:
self.start_test_run_for_bisect(api, update_step, master_dicts[0],
On 2016/01/06 00:38:24, Ken Russell wrote:
> On 2016/01/05 23:02:35, luqui wrote:
> > This [0] is suspicious.  Is it clear during construction that the first
> element
> > is distinguished in this way (or does it not matter for some reason)?
> 
> No, I didn't do a good job documenting the return values from
> api.chromium_tests.prepare_checkout. I agree this approach is pretty hacky.
> Basically, I found it necessary to return multiple master_dicts and test_specs
> from prepare_checkout, so both return values are arrays now. The first entry
in
> each array is what would previously have been returned in master_dict and
> test_spec. Subsequent entries correspond to the "additional_mirrors" entry in
> trybots.py.
> 
> I may have missed something major in the design here, but it does seem to me
> that it's necessary to return both multiple master_dict and test_spec
instances.
> However, api.chromium_tests.get_master_dict_with_dynamic_tests does some work
> which makes me wonder if all the data in test_spec is actually embedded
already
> in the returned master_dict instance.

I suspect that bisect would only run waterfall builders, which would only have
one entry in master_dicts. I feel it'd be cleaner to modify
start_test_run_for_bisect() to take master_dicts, but that would probably
trigger too much other code change... 

I'm actually not sure what would be a simple good solution here.

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/chromium_tests/api.py (right):

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/api.py:154: for c in
config.get('chromium_apply_config', []):
nit: Does the order matter? If not, you can shorten it to:

for c in set(config.get('chromium_apply_config', [])):
  ...

and skip the deduplication sets.

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/api.py:297: mastername, buildername,
test_specs[0], scripts_compile_targets)]
nit: indent args by 4 spaces

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/api.py:300: for mirror in
additional_trybot_mirrors:
nit: for i, mirror in enumerate(additional_trybot_mirrors, start=1):

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/api.py:302:
self.get_master_dict_with_dynamic_tests(
nit: indent args by 4 spaces, here and below

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/chromium_tests/trybots.py (right):

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/trybots.py:273:
'additional_mirrors': [
nit on the design: it might make sense to generalize the config, e.g. to:

  linux_chromium_rel_ng: [
   {
     'mastername': 'chromium.linux',
     'buildername': 'Linux Builder',
     'tester': 'Linux Tests',
   },
   {
      'mastername': 'chromium.gpu',
      'buildername': 'GPU Linux Builder',
      'tester': 'Linux Release (NVIDIA)',
    },
   ]

and allow a single dict as an option for backward compatibility, but interpreted
as a single element list. WDYT?

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipes/chrom...
File scripts/slave/recipes/chromium.py (right):

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipes/chrom...
scripts/slave/recipes/chromium.py:61: mastername, buildername, update_step,
master_dicts[0])
Is it guaranteed that master_dicts is always of size 1 here? I feel for sanity,
it might make sense to redefine tests_for_builder to take master_dicts.

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipes/chrom...
File scripts/slave/recipes/chromium_trybot.py (right):

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipes/chrom...
scripts/slave/recipes/chromium_trybot.py:263: for mirror in
additional_trybot_mirrors:
nit: if tests_for_builder() takes master_dicts and iterates over them, this code
wouldn't be necessary.

https://codereview.chromium.org/1554383003/diff/1/tests/masters_recipes_test.py
File tests/masters_recipes_test.py (right):

https://codereview.chromium.org/1554383003/diff/1/tests/masters_recipes_test....
tests/masters_recipes_test.py:183: set(WRONG_RECIPE_SUPPRESSIONS))
nit: 4 space indent for args

[Ken Russell (switch to Gerrit), 2016-01-06 03:30:07]

Thanks for your review Sergey. I'm certainly going to wait for Pawel's review
before proceeding.

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/chromium_tests/api.py (right):

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/api.py:154: for c in
config.get('chromium_apply_config', []):
On 2016/01/06 01:50:56, Sergey Berezin wrote:
> nit: Does the order matter? If not, you can shorten it to:
> 
> for c in set(config.get('chromium_apply_config', [])):
>   ...
> 
> and skip the deduplication sets.

After talking with iannucci@, the order does matter. Still, if there's a more
terse way to express this please tell me.

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/chromium_tests/trybots.py (right):

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/trybots.py:273:
'additional_mirrors': [
On 2016/01/06 01:50:56, Sergey Berezin wrote:
> nit on the design: it might make sense to generalize the config, e.g. to:
> 
>   linux_chromium_rel_ng: [
>    {
>      'mastername': 'chromium.linux',
>      'buildername': 'Linux Builder',
>      'tester': 'Linux Tests',
>    },
>    {
>       'mastername': 'chromium.gpu',
>       'buildername': 'GPU Linux Builder',
>       'tester': 'Linux Release (NVIDIA)',
>     },
>    ]
> 
> and allow a single dict as an option for backward compatibility, but
interpreted
> as a single element list. WDYT?

That's certainly feasible. I don't know whether it'd be preferable. Currently in
this CL I tried to minimize the potential impact to the existing trybot
configurations, code, etc. by making the additional_mirrors "mixin" optional.
Happy to change it (or even change all of these trybots' configurations into
lists of dictionaries, and change the required format).

[Sergey Berezin, 2016-01-06 19:12:35]

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/chromium_tests/api.py (right):

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/api.py:154: for c in
config.get('chromium_apply_config', []):
On 2016/01/06 03:30:07, Ken Russell wrote:
> On 2016/01/06 01:50:56, Sergey Berezin wrote:
> > nit: Does the order matter? If not, you can shorten it to:
> > 
> > for c in set(config.get('chromium_apply_config', [])):
> >   ...
> > 
> > and skip the deduplication sets.
> 
> After talking with iannucci@, the order does matter. Still, if there's a more
> terse way to express this please tell me.

Acknowledged.

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/chromium_tests/trybots.py (right):

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/trybots.py:273:
'additional_mirrors': [
On 2016/01/06 03:30:07, Ken Russell wrote:
> On 2016/01/06 01:50:56, Sergey Berezin wrote:
> > nit on the design: it might make sense to generalize the config, e.g. to:
> > 
> >   linux_chromium_rel_ng: [
> >    {
> >      'mastername': 'chromium.linux',
> >      'buildername': 'Linux Builder',
> >      'tester': 'Linux Tests',
> >    },
> >    {
> >       'mastername': 'chromium.gpu',
> >       'buildername': 'GPU Linux Builder',
> >       'tester': 'Linux Release (NVIDIA)',
> >     },
> >    ]
> > 
> > and allow a single dict as an option for backward compatibility, but
> interpreted
> > as a single element list. WDYT?
> 
> That's certainly feasible. I don't know whether it'd be preferable. Currently
in
> this CL I tried to minimize the potential impact to the existing trybot
> configurations, code, etc. by making the additional_mirrors "mixin" optional.
> Happy to change it (or even change all of these trybots' configurations into
> lists of dictionaries, and change the required format).

To be clear, what I'm proposing is backward-compatible to other configs (no
changes necessary), and it'll likely simplify your code.

Specifically, if you add logic to interpret 'builder': { ... } as 'builder': [{
... }] early on, you only need the code you already have for
'additional_mirrors', and can kill the special handling of the original fields.

But I'll leave it up to you and Pawel for a final form. My only concern is the
already non-trivial configuration grows yet another special flag, and it doesn't
really have to.

[Paweł Hajdan Jr., 2016-01-07 09:14:27]

Thanks. Mostly high-level issues. WDYT?

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/chromium_tests/api.py (right):

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/api.py:71: def
_get_bot_config_internal(self, mastername, buildername):
"internal" is a bit vague. How about renaming to _get_mutable_bot_config to make
the "danger" clear?

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/api.py:86: # gclient_config.
Otherwise, unexpected results are likely.
Whoa. Thanks for thinking about this. I agree it's an issue.

Now, with this code being already quite complex (have you talked to Luke and
some other folks?), I'd really even go out of my way to keep it as simple as
possible.

Would it work for you if we e.g. moved GPU bots over to the same waterfall as
chromium bots, and make both use the same builders? My understanding is that
it'd help avoid at least some of the additional logic from this patch.

In fact, we had code to support multiple testers that use the same builder in
the past, and it was quite simple IMO.

[Ken Russell (switch to Gerrit), 2016-01-07 10:19:24]

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/chromium_tests/api.py (right):

https://codereview.chromium.org/1554383003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/api.py:86: # gclient_config.
Otherwise, unexpected results are likely.
On 2016/01/07 09:14:27, Paweł Hajdan Jr. (OOO) wrote:
> Whoa. Thanks for thinking about this. I agree it's an issue.
> 
> Now, with this code being already quite complex (have you talked to Luke and
> some other folks?), I'd really even go out of my way to keep it as simple as
> possible.

I haven't talked with Luke about this patch. I mentioned the idea briefly to
Robbie.

> Would it work for you if we e.g. moved GPU bots over to the same waterfall as
> chromium bots, and make both use the same builders? My understanding is that
> it'd help avoid at least some of the additional logic from this patch.
> 
> In fact, we had code to support multiple testers that use the same builder in
> the past, and it was quite simple IMO.

We can discuss it, but my initial reaction is that I would rather not scatter
the chromium.gpu bots over multiple waterfalls. It would make it harder to
sheriff the GPU bots and to correctly maintain the tests. (Adding a new test
would then involve touching chromium.linux.json, chromium.mac.json,
chromium.win.json, and chromium.gpu.fyi.json, instead of just chromium.gpu.json
and chromium.gpu.fyi.json.) The chromium.gpu.fyi bots would no longer mirror the
chromium.gpu bots closely. Do these points resonate with you, or do you really
think the high-level approach in this CL should be abandoned? I think the
approach here is more generally useful.

[Paweł Hajdan Jr., 2016-01-07 16:28:55]

On 2016/01/07 at 10:19:24, kbr wrote:
> I haven't talked with Luke about this patch. I mentioned the idea briefly to
Robbie.

To clarify, I was thinking about concerns about code complexity in the
chromium_tests recipe module in general. I'm worried this patch is adding even
more logic there.

> > Would it work for you if we e.g. moved GPU bots over to the same waterfall
as
> > chromium bots, and make both use the same builders? My understanding is that
> > it'd help avoid at least some of the additional logic from this patch.
> > 
> > In fact, we had code to support multiple testers that use the same builder
in
> > the past, and it was quite simple IMO.
> 
> We can discuss it, but my initial reaction is that I would rather not scatter
the chromium.gpu bots over multiple waterfalls. It would make it harder to
sheriff the GPU bots and to correctly maintain the tests. (Adding a new test
would then involve touching chromium.linux.json, chromium.mac.json,
chromium.win.json, and chromium.gpu.fyi.json, instead of just chromium.gpu.json
and chromium.gpu.fyi.json.) The chromium.gpu.fyi bots would no longer mirror the
chromium.gpu bots closely. Do these points resonate with you, or do you really
think the high-level approach in this CL should be abandoned? I think the
approach here is more generally useful.

Acknowledged. There's definitely a tradeoff here. I'd like to explore it.

Note that in general, with LUCI and maybe even earlier with recipes on swarming
(bypassing buildbot), we'd be moving away from the concept of a waterfall. Most
drawbacks you listed already apply to non-GPU tests if I understand correctly.
chromium.gpu.fyi mirroring chromium.gpu should be solvable - it's not clear to
me why moving bots between waterfalls would make a difference for that.

Also to clarify my general approach to design of this code, "more generally
useful" isn't necessarily what I use to evaluate options. All else being equal,
sure, more general seems better. However, if I can find a way to simplify things
based on how they're actually used (and what use cases are truly necessary and
which ones just seem nice to have or cool), I'd try to go the simpler, not the
more generic way. Simpler is often easier to change as well.

[Ken Russell (switch to Gerrit), 2016-01-08 07:34:39]

Description was changed from

==========
Add support for "additional_mirrors" property in trybot configs.

This allows a single trybot to mirror multiple waterfall bots, so that
it can build the binaries once, but spawn all of the tests (typically
via swarming) that each of those waterfall bots would run.

This will be used to pull in the tests from the chromium.gpu waterfall
onto a few trybot configurations (linux_chromium_rel_ng,
mac_chromium_rel_ng, win_chromium_rel_ng), which is currently being done
manually in the chromium_trybot recipe.

BUG=573337
==========

to

==========
Add support for "additional_mirrors" property in trybot configs.

This allows a single trybot to mirror multiple waterfall bots, so that
it can build the binaries once, but spawn all of the tests (typically
via swarming) that each of those waterfall bots would run.

This will be used to pull in the tests from the chromium.gpu waterfall
onto a few trybot configurations (linux_chromium_rel_ng,
mac_chromium_rel_ng, win_chromium_rel_ng), which is currently being done
manually in the chromium_trybot recipe.

BUG=573337
==========

[Paweł Hajdan Jr., 2016-01-08 16:04:42]

After a VC, please don't consider this blocked on me.

LGTM after taking into account issues flagged by other reviewers (or when
they're fine with this).

Feel free to have this (and future related changes) only reviewed by US-based
reviewers. You can point people to this note if needed. This recipe code will
see major changes anyway, and there's big win from GPU unification. Thanks for
working on it!

[Ken Russell (switch to Gerrit), 2016-01-09 06:30:44]

Thanks Pawel and everyone for taking a look. After more work on this, going to
pursue the alternate approach in https://codereview.chromium.org/1565113003 and
a coming dependent CL. Closing.