Add concept of bot config and test spec database (BotConfigAndTestDB).

Add concept of bot config and test spec database (BotConfigAndTestDB).

This class abstracts the (master_dict, test_spec) pair that is returned
from api.chromium_tests.prepare_checkout. It will allow the database to
support multiple waterfalls (masternames) while avoiding changing most
callers' code.

BUG=542370
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=298207

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

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

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

All: please review.

For context: after an offline discussion between sergeyberezin, iannucci, luqui
and I today, sergeyberezin and I pair programmed most of this CL in preparation
for https://codereview.chromium.org/1554383003/ . It is a pure refactoring which
abstracts the pair of (master_dict, test_spec) return arguments from
api.chromium_tests.prepare_checkout into a first-class object.

The other CL will be much simpler and change far fewer callers with this CL in
place. It will be implemented by changing api.chromium_tests.prepare_checkout to
take a list of (mastername, buildername) pairs. Most callers will still pass
just one pair, but chromium_trybot will be changed to support passing more than
one.

There's a longstanding TODO with the configuration of enable_swarming in
_get_master_dict_with_dynamic_tests that needs to be fixed after this lands in
order to make the bot_db work in the general case. I am hoping that that bug can
be fixed during the European workday today (Friday) so that I can proceed with
redoing the other CL on top of this one.

Thanks.

[Sergiy Byelozyorov, 2016-01-08 10:35:58]

LGTM - I see no problem with this refactoring. Changes that are needed to make a
trybot able to mirror multiple waterfall builders may require further
discussion, but this is outside of scope of this CL.

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

https://codereview.chromium.org/1565113003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/bot_config_and_test_db.py:7: class
BotConfigAndTestDB(object):
IMHO, this should be named BuilderConfigAndTestDB, because 'bot' is typically a
word for a specific machine on swarming ('slave' is used for machines used by
Buildbot). For example, get_bot_config below actually returns a builder config,
which also why it is passed 'buildername' parameter. Probably what was meant
here is a short version of 'trybot' that is used for builders used by CQ, but
since this module is used by both waterfall and trybots, I suggest using
'builder' term.

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

LGTM w/nit

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

https://codereview.chromium.org/1565113003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/bot_config_and_test_db.py:18: pass
nit: This line shouldn't be needed now.

[Sergey Berezin, 2016-01-08 18:54:14]

LGTM + a few nits. Thanks!

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

https://codereview.chromium.org/1565113003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/bot_config_and_test_db.py:5: import
collections
Not used, remove.

https://codereview.chromium.org/1565113003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/bot_config_and_test_db.py:8: """An
immutable database of bot configurations and test specifications.
nit: s/bot/builder/ in the docstring as well - agreed with Sergiy's comment on
terminology.

https://codereview.chromium.org/1565113003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/bot_config_and_test_db.py:23: 
nit: remove the empty line.

https://codereview.chromium.org/1565113003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/bot_config_and_test_db.py:38: return
self._db[mastername]['master_dict'].get('builders', {}).get(
nit: if mastername is not in the _db, this will raise an exception. It's fine if
we expect the tests to catch all these errors. But otherwise I'd use
_db.get(mastername, {'master_dict': {}}).

https://codereview.chromium.org/1565113003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/bot_config_and_test_db.py:42: return
self._db[mastername]['master_dict'].get('settings', {})
nit: same as above.

https://codereview.chromium.org/1565113003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/bot_config_and_test_db.py:48: 
nit: remove the empty line.

https://codereview.chromium.org/1565113003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/bot_config_and_test_db.py:56: return
self._db[mastername]['test_spec'].get(buildername, {})
nit: same as above.

[Ken Russell (switch to Gerrit), 2016-01-08 23:02:45]

FYI, I'm going to wait to CQ this until I've reimplemented
https://codereview.chromium.org/1554383003/ on top of it to make sure it looks
good -- hopefully ready in an hour or two.

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

https://codereview.chromium.org/1565113003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/bot_config_and_test_db.py:5: import
collections
On 2016/01/08 18:54:13, Sergey Berezin wrote:
> Not used, remove.

Done.

https://codereview.chromium.org/1565113003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/bot_config_and_test_db.py:7: class
BotConfigAndTestDB(object):
On 2016/01/08 10:35:57, Sergiy Byelozyorov wrote:
> IMHO, this should be named BuilderConfigAndTestDB, because 'bot' is typically
a
> word for a specific machine on swarming ('slave' is used for machines used by
> Buildbot). For example, get_bot_config below actually returns a builder
config,
> which also why it is passed 'buildername' parameter. Probably what was meant
> here is a short version of 'trybot' that is used for builders used by CQ, but
> since this module is used by both waterfall and trybots, I suggest using
> 'builder' term.

Sergiy and I talked about this during a Hangout this morning and I think
'builder' would be more confusing than 'bot'. 'builder' has a specific meaning
in this code (as opposed to 'tester'), but 'bot' is already used throughout the
code to describe both kinds of machines. We are in agreement to leave the naming
as is.

https://codereview.chromium.org/1565113003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/bot_config_and_test_db.py:8: """An
immutable database of bot configurations and test specifications.
On 2016/01/08 18:54:13, Sergey Berezin wrote:
> nit: s/bot/builder/ in the docstring as well - agreed with Sergiy's comment on
> terminology.

Please see above -- I think the term 'bot' should be used here, not 'builder'.

https://codereview.chromium.org/1565113003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/bot_config_and_test_db.py:18: pass
On 2016/01/08 16:00:42, Paweł Hajdan Jr. wrote:
> nit: This line shouldn't be needed now.

Done.

https://codereview.chromium.org/1565113003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/bot_config_and_test_db.py:23: 
On 2016/01/08 18:54:13, Sergey Berezin wrote:
> nit: remove the empty line.

Done.

https://codereview.chromium.org/1565113003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/bot_config_and_test_db.py:38: return
self._db[mastername]['master_dict'].get('builders', {}).get(
On 2016/01/08 18:54:13, Sergey Berezin wrote:
> nit: if mastername is not in the _db, this will raise an exception. It's fine
if
> we expect the tests to catch all these errors. But otherwise I'd use
> _db.get(mastername, {'master_dict': {}}).

There should be strong guarantees that a dictionary for this mastername should
already be in the database. I think it's better to raise an exception here if
the lookup fails, because the consequence is that compile targets or tests
silently disappear if there's a bug, rather than the recipe failing visibly.

https://codereview.chromium.org/1565113003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/bot_config_and_test_db.py:42: return
self._db[mastername]['master_dict'].get('settings', {})
On 2016/01/08 18:54:13, Sergey Berezin wrote:
> nit: same as above.

Same reply.

https://codereview.chromium.org/1565113003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/bot_config_and_test_db.py:48: 
On 2016/01/08 18:54:13, Sergey Berezin wrote:
> nit: remove the empty line.

Done.

https://codereview.chromium.org/1565113003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/bot_config_and_test_db.py:56: return
self._db[mastername]['test_spec'].get(buildername, {})
On 2016/01/08 18:54:13, Sergey Berezin wrote:
> nit: same as above.

Same reply.

[Sergey Berezin, 2016-01-08 23:31:59]

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

https://codereview.chromium.org/1565113003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/bot_config_and_test_db.py:7: class
BotConfigAndTestDB(object):
On 2016/01/08 23:02:44, Ken Russell wrote:
> On 2016/01/08 10:35:57, Sergiy Byelozyorov wrote:
> > IMHO, this should be named BuilderConfigAndTestDB, because 'bot' is
typically
> a
> > word for a specific machine on swarming ('slave' is used for machines used
by
> > Buildbot). For example, get_bot_config below actually returns a builder
> config,
> > which also why it is passed 'buildername' parameter. Probably what was meant
> > here is a short version of 'trybot' that is used for builders used by CQ,
but
> > since this module is used by both waterfall and trybots, I suggest using
> > 'builder' term.
> 
> Sergiy and I talked about this during a Hangout this morning and I think
> 'builder' would be more confusing than 'bot'. 'builder' has a specific meaning
> in this code (as opposed to 'tester'), but 'bot' is already used throughout
the
> code to describe both kinds of machines. We are in agreement to leave the
naming
> as is.

Acknowledged.

https://codereview.chromium.org/1565113003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/bot_config_and_test_db.py:38: return
self._db[mastername]['master_dict'].get('builders', {}).get(
On 2016/01/08 23:02:44, Ken Russell wrote:
> On 2016/01/08 18:54:13, Sergey Berezin wrote:
> > nit: if mastername is not in the _db, this will raise an exception. It's
fine
> if
> > we expect the tests to catch all these errors. But otherwise I'd use
> > _db.get(mastername, {'master_dict': {}}).
> 
> There should be strong guarantees that a dictionary for this mastername should
> already be in the database. I think it's better to raise an exception here if
> the lookup fails, because the consequence is that compile targets or tests
> silently disappear if there's a bug, rather than the recipe failing visibly.

Ack. Agreed, let's fail with exception here.

[Ken Russell (switch to Gerrit), 2016-01-11 22:03:23]

Since it sounds like the dependent CL https://codereview.chromium.org/1574433004
will make progress, I'm going to CQ this now.

[Ken Russell (switch to Gerrit), 2016-01-11 22:03:28]

The CQ bit was checked by kbr@chromium.org

[Ken Russell (switch to Gerrit), 2016-01-11 22:03:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyberezin@chromium.org, phajdan.jr@chromium.org, sergiyb@chromium.org
Link to the patchset: https://codereview.chromium.org/1565113003/#ps40001
(title: "Rebased. (Probably unnecessary.)")

[commit-bot: I haz the power, 2016-01-11 22:03:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1565113003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1565113003/40001

[commit-bot: I haz the power, 2016-01-11 22:06:51]

Description was changed from

==========
Add concept of bot config and test spec database (BotConfigAndTestDB).

This class abstracts the (master_dict, test_spec) pair that is returned
from api.chromium_tests.prepare_checkout. It will allow the database to
support multiple waterfalls (masternames) while avoiding changing most
callers' code.

BUG=542370
==========

to

==========
Add concept of bot config and test spec database (BotConfigAndTestDB).

This class abstracts the (master_dict, test_spec) pair that is returned
from api.chromium_tests.prepare_checkout. It will allow the database to
support multiple waterfalls (masternames) while avoiding changing most
callers' code.

BUG=542370

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=298207
==========

[commit-bot: I haz the power, 2016-01-11 22:06:52]

Committed patchset #3 (id:40001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=298207

[Ken Russell (switch to Gerrit), 2016-01-12 03:47:12]

On 2016/01/11 22:06:52, commit-bot: I haz the power wrote:
> Committed patchset #3 (id:40001) as
> http://src.chromium.org/viewvc/chrome?view=rev&revision=298207

Note: luqui pointed out that this CL broke the internal recipes. I've prepared
two CLs, one in tools/build and one in build_internal, fixing the breakage. My
preference would be to land those two CLs rather than revert this one, but if
you feel it's necessary, please revert this. Thanks.