Allow a single trybot to mirror multiple waterfall bots.

scripts/slave/recipe_modules/chromium_tests/api.py

Index: scripts/slave/recipe_modules/chromium_tests/api.py
diff --git a/scripts/slave/recipe_modules/chromium_tests/api.py b/scripts/slave/recipe_modules/chromium_tests/api.py
index 25292663aeffa3ee9c0fba4d56c06ae1ccdb9c03..588b5b4c892421fd03b16f177456b6c86894d9e0 100644
--- a/scripts/slave/recipe_modules/chromium_tests/api.py
+++ b/scripts/slave/recipe_modules/chromium_tests/api.py
@@ -18,7 +18,6 @@ from . import builders
 from . import steps
 from . import trybots
 
-

[Sergey Berezin, 2016/01/12 01:44:23]

nit: please keep 2 blank lines: we separate most top-level entries in Python
files by 2 blank lines.

[Ken Russell (switch to Gerrit), 2016/01/12 05:35:51]

Sorry, that was an accident.

 MB_CONFIG_FILENAME = ['tools', 'mb', 'mb_config.pyl']
 
 
@@ -69,62 +68,131 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
     """Adds builders to our builder map"""
     self._builders.update(builders)
 
-  def _get_bot_config(self, mastername, buildername):
+  def _get_mutable_bot_config(self, mastername, buildername):
     master_dict = self.builders.get(mastername, {})
-    return freeze(master_dict.get('builders', {}).get(buildername))
+    return master_dict.get('builders', {}).get(buildername)
 
-  def configure_build(self, mastername, buildername, override_bot_type=None):
-    master_dict = self.builders.get(mastername, {})
-    bot_config = master_dict.get('builders', {}).get(buildername)
+  def _get_bot_config(self, mastername, buildername):
+    return freeze(self._get_mutable_bot_config(mastername, buildername))
+
+  def _get_bot_configs(self, bot_descs):
+    bot_configs = []
+    for bot_desc in bot_descs:
+      bot_configs.append(self._get_mutable_bot_config(bot_desc['mastername'],
+                                                      bot_desc['buildername']))
+    return bot_configs
+
+  def normalize_bot_descs(self, bot_descs):

[Paweł Hajdan Jr., 2016/01/11 18:41:11]

Why do we need this? It's not a very strong preference, but in general I dislike
APIs that accept different formats and "guess" things or try to provide
convenience. WDYT?

[Ken Russell (switch to Gerrit), 2016/01/11 22:54:01]

chromium_trybot.py fetches the entry for a tryserver from trybots.py's
dictionaries, and that entry might be either a dictionary containing
"mastername" and "buildername" properties, or a list of such dictionaries.

We could of course change the required syntax in trybots.py, but that seems like
a fairly "noisy" change not adding a lot of value. It seems to me that if both
syntaxes have to be required, then we might as well add a helper function which
takes care of it. I don't have a strong opinion though.

(There's also an issue when trybots.py's data structures are frozen, and needing
to discover that the lists/dictionaries are still valid inputs.)

[Sergey Berezin, 2016/01/12 01:44:23]

This is per my request in the original CL to keep the API backwards compatible.
This code would allow keeping all the 1.5 gazillion configs as they are.
Migrating them all to the new format can be delayed to later CLs, if at all
necessary.

+    """Gets the bot_descs into a uniform format. It should be an array of
+    dictionary-like objects containing the properties 'mastername' and
+    'buildername'.
+    """
+    if not isinstance(bot_descs, (list, tuple)):
+      return [bot_descs]
+    return bot_descs
+
+  def _verify_property_is_same_across_bot_configs(self, bot_configs, property):
+    if len(bot_configs) < 2:
+      return
+    # This code path is only taken for trybots that mirror more than
+    # one waterfall bot. Verify that all of the other builders this
+    # trybot is supposed to mirror all use the same key properties
+    # like chromium_config and gclient_config. Otherwise, unexpected
+    # results are likely.
+    value = bot_configs[0].get(property)
+    for bot_config in bot_configs[1:]:
+      if value != bot_config.get(property):
+        # TODO(kbr): add a test configuration which triggers this error.
+        # Unclear how difficult it will be.
+        raise ValueError(
+            '%s was different between two bots a trybot is mirroring (%s vs %s)' %
+            (property, value, bot_config.get(value)))  # pragma: no cover
+
+  def create_bot_desc(self, mastername, buildername):
+    """Creates a valid "bot_desc" argument for the various APIs below from
+    the given mastername and buildername. A dictionary containing the
+    properties "mastername" and "buildername" is a suitable argument.
+    An array of such dictionaries is also suitable, in which case tests
+    will be pulled from multiple testers.

[Sergey Berezin, 2016/01/12 01:44:23]

The docstring doesnt' make sense to me. It appears you are describing the return
result options, but it doesn't really apply to this method.

Also, if we are standardizing on a list of dicts, why not return a normalized
form?

[Ken Russell (switch to Gerrit), 2016/01/12 05:35:51]

That's true. I should have documented the "bot_desc" argument in the subsequent
APIs instead; however, several of them didn't have docs in the first place and I
wasn't sure how to document them.
That's a fair point.

+    """
+    return {'mastername': mastername, 'buildername': buildername}
+
+  def configure_build(self, bot_desc, override_bot_type=None):
+    bot_desc = self.normalize_bot_descs(bot_desc)
+    bot_configs = self._get_bot_configs(bot_desc)
 
     # Get the buildspec version. It can be supplied as a build property or as
     # a recipe config value.
     buildspec_version = (self.m.properties.get('buildspec_version') or
-                         bot_config.get('buildspec_version'))
+                         bot_configs[0].get('buildspec_version'))

[Paweł Hajdan Jr., 2016/01/11 18:41:11]

Shouldn't we check that 'buildspec_version' is consistent across bot configs?

Here's an idea: we can do that while getting the value, i.e. instead of
verifying things and then manually dereferencing [0], have the getter also
verify consistency. Example: get_property_from_bot_configs(bot_configs,
'buildspec_version'), and it'd also internally verify consistency. WDYT?

Main rationale is to make it difficult to forget verifying consistency.

[Ken Russell (switch to Gerrit), 2016/01/11 22:54:01]

Probably, or at least possibly. I didn't know what buildspec_version did.
That sounds like a good idea.

When doing this, though, keep in mind that not all of the properties will be the
same across all of the bot_configs. chromium_apply_config and
gclient_apply_config are two that are important to accumulate across the
bot_configs.

+
+    self._verify_property_is_same_across_bot_configs(bot_configs,
+                                                     'chromium_config')
 
     self.m.chromium.set_config(
-        bot_config.get('chromium_config'),
-        **bot_config.get('chromium_config_kwargs', {}))
+        bot_configs[0].get('chromium_config'),
+        **bot_configs[0].get('chromium_config_kwargs', {}))
 
     # Set GYP_DEFINES explicitly because chromium config constructor does
     # not support that.
-    self.m.chromium.c.gyp_env.GYP_DEFINES.update(
-        bot_config.get('GYP_DEFINES', {}))
-    if bot_config.get('use_isolate'):
-      self.m.isolate.set_isolate_environment(self.m.chromium.c)
+    for bot_config in bot_configs:
+      self.m.chromium.c.gyp_env.GYP_DEFINES.update(
+          bot_config.get('GYP_DEFINES', {}))
+      if bot_config.get('use_isolate'):
+        self.m.isolate.set_isolate_environment(self.m.chromium.c)
+
+    self._verify_property_is_same_across_bot_configs(bot_configs,
+                                                     'gclient_config')
 
     self.m.gclient.set_config(
-        bot_config.get('gclient_config'),
+        bot_configs[0].get('gclient_config'),
         PATCH_PROJECT=self.m.properties.get('patch_project'),
         BUILDSPEC_VERSION=buildspec_version,
-        **bot_config.get('gclient_config_kwargs', {}))
+        **bot_configs[0].get('gclient_config_kwargs', {}))

[Sergey Berezin, 2016/01/12 01:44:23]

Should *_kwargs properties also be checked for being the same, since you'd get
them only from configs[0]?

[Ken Russell (switch to Gerrit), 2016/01/12 05:35:50]

Good point. Yes, I think so. (Is this property even used any more? It doesn't
show up in code search.)

 
-    if 'android_config' in bot_config:
+    if 'android_config' in bot_configs[0]:

[Sergey Berezin, 2016/01/12 01:44:23]

nit: perhaps, more generally:

  if any('android_config' in bc for bc in bot_configs):
    # Move this up, since 'android_config' may not be present in [0].
    assert len(bot_configs) == 1
    # the rest of the code

[Ken Russell (switch to Gerrit), 2016/01/12 05:35:50]

Good point.

       self.m.chromium_android.configure_from_properties(
-          bot_config['android_config'],
-          **bot_config.get('chromium_config_kwargs', {}))
-
-    if 'amp_config' in bot_config:
-      self.m.amp.set_config(bot_config['amp_config'])
-
-    for c in bot_config.get('chromium_apply_config', []):
-      self.m.chromium.apply_config(c)
-
-    for c in bot_config.get('gclient_apply_config', []):
-      self.m.gclient.apply_config(c)
+          bot_configs[0]['android_config'],
+          **bot_configs[0].get('chromium_config_kwargs', {}))
+      # This code path hasn't been generalized yet for a trybot that

[Paweł Hajdan Jr., 2016/01/11 18:41:11]

Why not a TODO? Also applies to other places which haven't been converted.

[Ken Russell (switch to Gerrit), 2016/01/11 22:54:01]

Yes, these should have TODOs.

+      # mirrors multiple bots.
+      assert len(bot_configs) == 1
+
+    if 'amp_config' in bot_configs[0]:

[Sergey Berezin, 2016/01/12 01:44:23]

nit: same generalization as above.

[Ken Russell (switch to Gerrit), 2016/01/12 05:35:51]

Agree.

+      self.m.amp.set_config(bot_configs[0]['amp_config'])
+      # This code path hasn't been generalized yet for a trybot that
+      # mirrors multiple bots.
+      assert len(bot_configs) == 1
+
+    chromium_applied_configs = set()
+    gclient_applied_configs = set()
+    for bot_config in bot_configs:
+      for c in bot_config.get('chromium_apply_config', []):
+        if not c in chromium_applied_configs:
+          self.m.chromium.apply_config(c)
+          chromium_applied_configs.add(c)
+      for c in bot_config.get('gclient_apply_config', []):
+        if not c in gclient_applied_configs:
+          self.m.gclient.apply_config(c)
+          gclient_applied_configs.add(c)
 
     # WARNING: src-side runtest.py is only tested with chromium CQ builders.
     # Usage not covered by chromium CQ is not supported and can break
     # without notice.
+    master_dict = self.builders.get(bot_desc[0]['mastername'], {})

[Paweł Hajdan Jr., 2016/01/11 18:41:11]

Why don't we also ensure consistency here?

[Ken Russell (switch to Gerrit), 2016/01/11 22:54:01]

No good reason not to. This query would be a little different from the others,
since it'd be ensuring consistency across the master_dictionaries, rather than
the bot_configs.

     if master_dict.get('settings', {}).get('src_side_runtest_py'):
       self.m.chromium.c.runtest_py.src_side = True
 
-    if bot_config.get('goma_canary'):
-      self.m.goma.update_goma_canary(buildername)
+    if bot_configs[0].get('goma_canary'):

[Sergey Berezin, 2016/01/12 01:44:23]

nit: same generalization as above.

[Ken Russell (switch to Gerrit), 2016/01/12 05:35:51]

Acknowledged.

+      self.m.goma.update_goma_canary(bot_desc[0]['buildername'])
+      # This code path hasn't been generalized yet for a trybot that
+      # mirrors multiple bots.
+      assert len(bot_configs) == 1
 
-    bot_type = override_bot_type or bot_config.get('bot_type', 'builder_tester')
+    bot_type = override_bot_type or bot_configs[0].get('bot_type',

[Sergey Berezin, 2016/01/12 01:44:23]

nit: move inside the 'if'. This var is only used inside the conditional, and now
it also doesn't make sense outside without the check for len(bot_configs) == 1. 

[Ken Russell (switch to Gerrit), 2016/01/12 05:35:51]

Good point.

+                                                       'builder_tester')
 
-    if bot_config.get('set_component_rev'):
+    if bot_configs[0].get('set_component_rev'):

[Sergey Berezin, 2016/01/12 01:44:23]

nit: same generalization as above.

[Ken Russell (switch to Gerrit), 2016/01/12 05:35:51]

Acknowledged.

       # If this is a component build and the main revision is e.g. blink,
       # webrtc, or v8, the custom deps revision of this component must be
       # dynamically set to either:
@@ -137,8 +205,11 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
       if bot_type == 'tester':
         component_rev = self.m.properties.get(
             'parent_got_revision', component_rev)
-      dep = bot_config.get('set_component_rev')
+      dep = bot_configs[0].get('set_component_rev')
       self.m.gclient.c.revisions[dep['name']] = dep['rev_str'] % component_rev
+      # This code path hasn't been generalized yet for a trybot that
+      # mirrors multiple bots.
+      assert len(bot_configs) == 1
 
   def ensure_checkout(self, mastername, buildername,
                       root_solution_revision=None):
@@ -179,7 +250,8 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
     else:
       self.m.chromium.runhooks()
 
-  def get_test_spec(self, mastername, buildername):
+  def get_test_spec(self, mastername, buildername,
+                    annotate_master_name=False):
     bot_config = self._get_bot_config(mastername, buildername)
 
     test_spec_file = bot_config.get('testing', {}).get(
@@ -188,7 +260,9 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
     # TODO(phajdan.jr): Bots should have no generators instead.
     if bot_config.get('disable_tests'):
       return {}
-    return self.read_test_spec(self.m, test_spec_file)
+    return self.read_test_spec(self.m, test_spec_file,
+                               annotate_master_name=annotate_master_name,
+                               master_name_for_annotation=mastername)
 
   def _get_master_dict_with_dynamic_tests(
       self, mastername, buildername, test_spec, scripts_compile_targets):
@@ -217,43 +291,63 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
     bot_db._add_master_dict_and_test_spec(mastername, master_dict, {})
     return bot_db
 
-  def prepare_checkout(self, mastername, buildername,
-                       root_solution_revision=None):
-    bot_config = self._get_bot_config(mastername, buildername)
+  def prepare_checkout(self, bot_desc, root_solution_revision=None):
+    bot_desc = self.normalize_bot_descs(bot_desc)
+    bot_configs = self._get_bot_configs(bot_desc)
+
+    # Use the first bot's mastername and buildername for several of
+    # the setup steps in this routine.
+    mastername = bot_desc[0]['mastername']
+    buildername = bot_desc[0]['buildername']
 
-    update_step = self.ensure_checkout(mastername, buildername,
+    update_step = self.ensure_checkout(mastername,
+                                       buildername,
                                        root_solution_revision)
     # TODO(robertocn): Remove this hack by the end of Q1/2016.
     if (mastername == 'tryserver.chromium.perf'
-        and bot_config.get('bot_type') == 'builder'
+        and bot_configs[0].get('bot_type') == 'builder'

[Sergey Berezin, 2016/01/12 01:44:23]

nit: perhaps, also generalize as above?

[Ken Russell (switch to Gerrit), 2016/01/12 05:35:50]

Maybe. Since this is a hack it might be best to touch it as little as possible.

         and buildername.endswith('builder')):
       force_legacy_compile = self.should_force_legacy_compiling(
           mastername, buildername)
       if force_legacy_compile:
         self.m.chromium.c.project_generator.tool = 'gyp'
+      # This code path hasn't been generalized yet for a trybot that
+      # mirrors multiple bots.
+      assert len(bot_configs) == 1
 
     self.set_up_swarming(mastername, buildername)
     self.runhooks(update_step)
 
-    test_spec = self.get_test_spec(mastername, buildername)
+    bot_db = bdb_module.BotConfigAndTestDB()
 
-    # TODO(phajdan.jr): Bots should have no generators instead.
-    if bot_config.get('disable_tests'):
-      scripts_compile_targets = {}
-    else:
-      scripts_compile_targets = \
-          self.get_compile_targets_for_scripts().json.output
+    scripts_compile_targets = {}
+    first_iteration = True
+
+    for bot in bot_desc:
+      test_spec = self.get_test_spec(
+          bot['mastername'], bot['buildername'],
+          annotate_master_name=not first_iteration)
+
+      # This is done this way to reduce test expectation changes.
+      if first_iteration:
+        # TODO(phajdan.jr): Bots should have no generators instead.
+        if bot_configs[0].get('disable_tests'):
+          scripts_compile_targets = {}
+        else:
+          scripts_compile_targets = \
+              self.get_compile_targets_for_scripts().json.output
+        first_iteration = False
 
-    master_dict = self._get_master_dict_with_dynamic_tests(
-        mastername, buildername, test_spec, scripts_compile_targets)
+      master_dict = self._get_master_dict_with_dynamic_tests(
+          bot['mastername'], bot['buildername'], test_spec,
+          scripts_compile_targets)
+      bot_db._add_master_dict_and_test_spec(
+          bot['mastername'], master_dict, test_spec)
 
     if self.m.chromium.c.lto and \
         not self.m.chromium.c.env.LLVM_FORCE_HEAD_REVISION:
       self.m.chromium.download_lto_plugin()
 
-    bot_db = bdb_module.BotConfigAndTestDB()
-    bot_db._add_master_dict_and_test_spec(mastername, master_dict, test_spec)
-
     return update_step, bot_db
 
   def generate_tests_from_test_spec(self, api, test_spec, builder_dict,
@@ -269,11 +363,15 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
           tests)
     return tests
 
-  def read_test_spec(self, api, test_spec_file):
+  def read_test_spec(self, api, test_spec_file, annotate_master_name=False,
+                     master_name_for_annotation=None):
     test_spec_path = api.path['checkout'].join('testing', 'buildbot',
                                                test_spec_file)
+    step_name = 'read test spec'
+    if annotate_master_name:
+        step_name += ' (%s)' % master_name_for_annotation
     test_spec_result = api.json.read(
-        'read test spec',
+        step_name,
         test_spec_path,
         step_test_data=lambda: api.json.test_api.output({}))
     test_spec_result.presentation.step_text = 'path: %s' % test_spec_path
@@ -332,6 +430,33 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
     return test_runner
 
   def get_compile_targets_and_tests(
+      self, bot_desc, bot_db, override_bot_type=None, override_tests=None):
+    bot_desc = self.normalize_bot_descs(bot_desc)
+    """Returns a tuple: list of compile targets and list of tests.
+
+    The list of tests includes ones on the triggered testers."""
+    all_compile_targets = set()
+    all_tests = []
+    for bot in bot_desc:
+      compile_targets, tests = self._get_single_bot_compile_targets_and_tests(
+          bot['mastername'], bot['buildername'], bot_db,
+          override_bot_type=override_bot_type, override_tests=override_tests)
+      all_compile_targets.update(compile_targets)
+      # It doesn't seem feasible to de-duplicate tests in the case where
+      # a tryserver mirrors multiple waterfall bots. (This is the only
+      # case where there's more than one entry in bot_configs.) Just add
+      # them all in the order returned.
+      all_tests.extend(tests)
+      # The incoming 'override_tests' must be reset during the second

[Paweł Hajdan Jr., 2016/01/11 18:41:11]

Why don't we override tests at the end?

The current logic doesn't seem to _add_ tests, but _replace_ them.

[Ken Russell (switch to Gerrit), 2016/01/11 22:54:01]

Good point; that's probably the correct thing to do according to how this code
works.

+      # and subsequent loop iterations, or trybots which mirror multiple
+      # bots will add the same set of tests multiple times.
+      # TODO(kbr): it isn't clear to me what the correct behavior is here.
+      # What should happen with the second bot's 'tests' property?
+      override_tests = None
+
+    return sorted(all_compile_targets), all_tests
+
+  def _get_single_bot_compile_targets_and_tests(
       self, mastername, buildername, bot_db,
       override_bot_type=None, override_tests=None):
     """Returns a tuple: list of compile targets and list of tests.
@@ -391,19 +516,19 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
       command(lambda name: name)
 
 
-  def compile(self, mastername, buildername, update_step, bot_db,
+  def compile(self, bot_desc, update_step, bot_db,
               mb_mastername=None, mb_buildername=None):
-    """Runs compile and related steps for given builder."""
+    """Runs compile and related steps for builder(s) described in bot_desc."""
+    bot_desc = self.normalize_bot_descs(bot_desc)
     assert isinstance(bot_db, bdb_module.BotConfigAndTestDB), \
         "bot_db argument %r was not a BotConfigAndTestDB" % (bot_db)
     compile_targets, tests_including_triggered = \
         self.get_compile_targets_and_tests(
-            mastername,
-            buildername,
+            bot_desc,
             bot_db)
     self.compile_specific_targets(
-        mastername, buildername, update_step, bot_db,
-        compile_targets, tests_including_triggered,
+        bot_desc[0]['mastername'], bot_desc[0]['buildername'],

[Sergey Berezin, 2016/01/12 01:44:23]

Shouldn't we compile for all bot_descs? E.g. this would miss GPU builder,
wouldn't it?

[Ken Russell (switch to Gerrit), 2016/01/12 05:35:51]

At first glance it didn't seem necessary to generalize compile_specific_targets
in that way -- the compile_targets have already been assembled, and it looked
like the only thing the mastername and buildername were being used for was
constructing the URL for archiving the build. I might be wrong about that,
though.

+        update_step, bot_db, compile_targets, tests_including_triggered,
         mb_mastername=mb_mastername, mb_buildername=mb_buildername)
 
   def compile_specific_targets(