[telemetry] Add @RunOnBuildMastersNamed support

[telemetry] Add @RunOnBuildMastersNamed support

Allows tests to customize what build master they run on. This paves the way
for in-situ specification of what tests to run where, rather than that
customization having to be done in the buildbot factory files.

BUG=256451
R=dtu@chromium.org

[dtu, 2013-07-03 01:25:43]

https://codereview.chromium.org/18576004/diff/1/tools/telemetry/telemetry/tes...
File tools/telemetry/telemetry/test.py (right):

https://codereview.chromium.org/18576004/diff/1/tools/telemetry/telemetry/tes...
tools/telemetry/telemetry/test.py:9: __all__ = ["Test", "RunOnMastersNamed"]
single quotes

https://codereview.chromium.org/18576004/diff/1/tools/telemetry/telemetry/tes...
File tools/telemetry/telemetry/test_runner.py (left):

https://codereview.chromium.org/18576004/diff/1/tools/telemetry/telemetry/tes...
tools/telemetry/telemetry/test_runner.py:70: 'enabled': test_class.enabled,
There are two tests in content/test/gpu/gpu_tests that i added "enabled" to,
please remove those and add decorators instead :)

https://codereview.chromium.org/18576004/diff/1/tools/telemetry/telemetry/tes...
File tools/telemetry/telemetry/test_runner.py (right):

https://codereview.chromium.org/18576004/diff/1/tools/telemetry/telemetry/tes...
tools/telemetry/telemetry/test_runner.py:57: def GetShortDescriptionOf(thing):
_GetShortDescriptionOf

https://codereview.chromium.org/18576004/diff/1/tools/telemetry/telemetry/tes...
tools/telemetry/telemetry/test_runner.py:86: if t['description']:
If GetShortDescriptionOf() returns '', you don't need guards here.

[tonyg, 2013-07-03 02:15:08]

https://codereview.chromium.org/18576004/diff/1/tools/perf/benchmarks/netsim_...
File tools/perf/benchmarks/netsim_top25.py (right):

https://codereview.chromium.org/18576004/diff/1/tools/perf/benchmarks/netsim_...
tools/perf/benchmarks/netsim_top25.py:9:
@test.RunOnBuildMastersNamed('chromium.perf')
Does this need to be platform specific? For instance, we run top_mobile_sites on
android and top_25 on desktop?

In any case, I'd recommend renaming this "test.RunOnBuildMaster"

[nduca, 2013-07-03 05:18:58]

> Does this need to be platform specific? For instance, we run top_mobile_sites
on
> android and top_25 on desktop?
I can add that. How would you specify this to the script?
--builder-platform=android?

> In any case, I'd recommend renaming this "test.RunOnBuildMaster"
It takes multiple, hence the plural name. And we give --build-master-name, so I
wanted to use the word name throughout.

[tonyg, 2013-07-03 15:24:24]

On 2013/07/03 05:18:58, nduca wrote:
> > Does this need to be platform specific? For instance, we run
top_mobile_sites
> on
> > android and top_25 on desktop?
> I can add that. How would you specify this to the script?
> --builder-platform=android?

SGTM, we should also make sure the default is all platforms (without having to
list them).

Thinking about this more, I also realized another wrinkle. Currently in the
buildbot scripts, we don't just specify "run on this waterfall" we also specify
"run on this slave". This is because we shard the tests across multiple slaves
and try to balance them out by cycle time so that the slaves will cycle evenly.
So I'm not quite sure how to resolve this. We could specify
test.RunOnBuildSlave(), but that seems to couple us too closely with the
buildbot code. We could also have the bots shard the tests across slaves by
number of tests, but I'm not sure how well that would work because test run time
can range from like 1m (in the case of the dozen dromaeo tests), to like 30m (in
the case of spaceport).

Concretely, here is the current sharding for linux:

9 tests:
http://build.chromium.org/p/chromium.perf/builders/Linux%20Perf%20%284%29/bui...
5 tests:
http://build.chromium.org/p/chromium.perf/builders/Linux%20Perf%20%283%29/bui...
24 tests:
http://build.chromium.org/p/chromium.perf/builders/Linux%20Perf%20%282%29/bui...
6 tests:
http://build.chromium.org/p/chromium.perf/builders/Linux%20Perf%20%281%29/bui...

> 
> > In any case, I'd recommend renaming this "test.RunOnBuildMaster"
> It takes multiple, hence the plural name. And we give --build-master-name, so
I
> wanted to use the word name throughout.

I think terser is better in this case, but don't feel strongly. Feel free to
ignore.

[tonyg, 2013-07-03 16:20:15]

On 2013/07/03 15:24:24, tonyg wrote:
> On 2013/07/03 05:18:58, nduca wrote:
> > > Does this need to be platform specific? For instance, we run
> top_mobile_sites
> > on
> > > android and top_25 on desktop?
> > I can add that. How would you specify this to the script?
> > --builder-platform=android?
> 
> SGTM, we should also make sure the default is all platforms (without having to
> list them).
> 
> Thinking about this more, I also realized another wrinkle. Currently in the
> buildbot scripts, we don't just specify "run on this waterfall" we also
specify
> "run on this slave". This is because we shard the tests across multiple slaves
> and try to balance them out by cycle time so that the slaves will cycle
evenly.
> So I'm not quite sure how to resolve this. We could specify
> test.RunOnBuildSlave(), but that seems to couple us too closely with the
> buildbot code. We could also have the bots shard the tests across slaves by
> number of tests, but I'm not sure how well that would work because test run
time
> can range from like 1m (in the case of the dozen dromaeo tests), to like 30m
(in
> the case of spaceport).
> 
> Concretely, here is the current sharding for linux:
> 
> 9 tests:
>
http://build.chromium.org/p/chromium.perf/builders/Linux%2520Perf%2520%25284%...
> 5 tests:
>
http://build.chromium.org/p/chromium.perf/builders/Linux%2520Perf%2520%25283%...
> 24 tests:
>
http://build.chromium.org/p/chromium.perf/builders/Linux%2520Perf%2520%25282%...
> 6 tests:
>
http://build.chromium.org/p/chromium.perf/builders/Linux%2520Perf%2520%25281%...
> 

Thinking about this some more. A lot of test frameworks have a concept of test
size, e.g. @BigTest, @MediumTest, @SmallTest. Maybe we could add an annotation
like that and then the master could use that to decide how to shard (ie. in
terms of cycle time 1 BigTest == 2 MediumTest == 10 SmallTest).

> > 
> > > In any case, I'd recommend renaming this "test.RunOnBuildMaster"
> > It takes multiple, hence the plural name. And we give --build-master-name,
so
> I
> > wanted to use the word name throughout.
> 
> I think terser is better in this case, but don't feel strongly. Feel free to
> ignore.

[dtu, 2013-07-03 20:21:47]

On 2013/07/03 16:20:15, tonyg wrote:
> On 2013/07/03 15:24:24, tonyg wrote:
> > On 2013/07/03 05:18:58, nduca wrote:
> > > > Does this need to be platform specific? For instance, we run
> > top_mobile_sites
> > > on
> > > > android and top_25 on desktop?
> > > I can add that. How would you specify this to the script?
> > > --builder-platform=android?
> > 
> > SGTM, we should also make sure the default is all platforms (without having
to
> > list them).
> > 
> > Thinking about this more, I also realized another wrinkle. Currently in the
> > buildbot scripts, we don't just specify "run on this waterfall" we also
> specify
> > "run on this slave". This is because we shard the tests across multiple
slaves
> > and try to balance them out by cycle time so that the slaves will cycle
> evenly.
> > So I'm not quite sure how to resolve this. We could specify
> > test.RunOnBuildSlave(), but that seems to couple us too closely with the
> > buildbot code. We could also have the bots shard the tests across slaves by
> > number of tests, but I'm not sure how well that would work because test run
> time
> > can range from like 1m (in the case of the dozen dromaeo tests), to like 30m
> (in
> > the case of spaceport).
> > 
> > Concretely, here is the current sharding for linux:
> > 
> > 9 tests:
> >
>
http://build.chromium.org/p/chromium.perf/builders/Linux%252520Perf%252520%25...
> > 5 tests:
> >
>
http://build.chromium.org/p/chromium.perf/builders/Linux%252520Perf%252520%25...
> > 24 tests:
> >
>
http://build.chromium.org/p/chromium.perf/builders/Linux%252520Perf%252520%25...
> > 6 tests:
> >
>
http://build.chromium.org/p/chromium.perf/builders/Linux%252520Perf%252520%25...
> > 
> 
> Thinking about this some more. A lot of test frameworks have a concept of test
> size, e.g. @BigTest, @MediumTest, @SmallTest. Maybe we could add an annotation
> like that and then the master could use that to decide how to shard (ie. in
> terms of cycle time 1 BigTest == 2 MediumTest == 10 SmallTest).
> 
> > > 
> > > > In any case, I'd recommend renaming this "test.RunOnBuildMaster"
> > > It takes multiple, hence the plural name. And we give --build-master-name,
> so
> > I
> > > wanted to use the word name throughout.
> > 
> > I think terser is better in this case, but don't feel strongly. Feel free to
> > ignore.

How is the sharding currently done? If you generate the sharding with a script,
then the tests could rearrange themselves on the slaves arbitrarily, making it
harder to track the results.

[tonyg, 2013-07-03 20:49:18]

> How is the sharding currently done?

We manually specific the set of tests that are associated with each slave.

> If you generate the sharding with a script,
> then the tests could rearrange themselves on the slaves arbitrarily, making it
> harder to track the results.

Good point, not sure what to do about that.

[nduca, 2013-07-08 22:26:37]

On 2013/07/03 20:49:18, tonyg wrote:
> > How is the sharding currently done?

Okay, after some thought: the thing we're agonizing about here is what exact
@RunOnBlahBLah restrictions we need. I had taken the approach of strongly typing
these --- but as I'm discovering, that makes it very hard to get consensus.

Buildbot typically passes stuff like --factory-properties=<big blob o json> to
the runtest script. This has the nice property of leaving the command line api
out of things. I think I may take a pass on this patch that does that as well.
Then, we dont have to agonize as much about the command line api, and new kinds
of restrictions can just be made inside the decorator function rather than
sprawling through a few files.

I'm thinking I"ll call it --bot-properties=... but still keep the basic idea.

For now, I'm going to keep it just to listening for master name, in this patch.
Parallel to this patch, xusydoc and Marcus are busily hacking on a sharded tests
script. With this basic capability in place, I think they can come back and make
small incremental edits to it to add slave name restrictions etc, as needed.

[bulach, 2013-07-09 10:07:31]

just some thoughts given the recent scars:
(please note that this entirely independent to the android sharding discussion:
that happens on yet-another-level, and as long as this produces a dict mapping
{"name": "command"}, the android side will be happy...)

the general point here is that maintaining the config of "bot": [tests] (or
"test": [bots]) is unavoidable.. if we could keep that as separate config file
and self-contained to test-runner, it'd probably be simpler..

https://codereview.chromium.org/18576004/diff/14001/tools/perf/benchmarks/net...
File tools/perf/benchmarks/netsim_top25.py (right):

https://codereview.chromium.org/18576004/diff/14001/tools/perf/benchmarks/net...
tools/perf/benchmarks/netsim_top25.py:23: return bot_properties.master_name in
['default', 'chromium.perf']
IMHO, sprinkling this "CanRunOnBot" makes it difficult to do the "slave-level
sharding": it's going to be hard to check that benchmarks A, B and C are running
in slave X, but D and E are running in slave Y..

I'd prefer to see this as pure data, like a json config saying something like:

{
  "BENCHMARK_A": {"slaves": [MAC_SLAVE_1, LINUX_SLAVE_1]},
  "BENCHMARK_B": {"slaves": [MAC_SLAVE_2, LINUX_SLAVE_2]},
  "BENCHMARK_C": {"master": FOO_MASTER},
}

I don't have strong feelings whether the keys should be BENCHMARK or SLAVE,
whatever makes it more compact...

but my point is that the benchmarks themselves should not know about bot: that's
configuration data for the test_runner itself to deal with.. wdyt?

https://codereview.chromium.org/18576004/diff/14001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/test_runner.py (right):

https://codereview.chromium.org/18576004/diff/14001/tools/telemetry/telemetry...
tools/telemetry/telemetry/test_runner.py:94: test_list = []
so this would be the only place to do something like:

if options.bot_properties:
  # Running on a bot, return the filtered list of tests.
  return self.GetFilteredTests(self.ReadBotConfig())
# Return all available tests. 
...

the common use-cases I'm thinking is:
- add a new test to the bot.
- load-balance existing tests.

if we have a centralized data config, it'll be much easier to check to which bot
the test should be added, or which ones need to be moved around.. if the
decision is sprinkled all over the place, it'd be potentially harder..