Add support for "additional_mirrors" property in trybot configs.

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 a02be4c40e17e00e09ba909bda7c88fd9a4a5801..7eba380f53c2af47c61a863ac4c53957c7fb57a3 100644
--- a/scripts/slave/recipe_modules/chromium_tests/api.py
+++ b/scripts/slave/recipe_modules/chromium_tests/api.py
@@ -68,13 +68,44 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
     """Adds builders to our builder map"""
     self._builders.update(builders)
 
-  def get_bot_config(self, mastername, buildername):
+  def _get_bot_config_internal(self, mastername, buildername):

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

"internal" is a bit vague. How about renaming to _get_mutable_bot_config to make
the "danger" clear?

     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_bot_config_internal(mastername, buildername))
+
+  def _verify_additional_trybot_mirror_property(self, mastername, buildername,
+                                                additional_trybot_mirrors,
+                                                property):
+    if not additional_trybot_mirrors:
+      return
+    bot_config = self._get_bot_config_internal(mastername, buildername)
+    # 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.

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

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]

I haven't talked with Luke about this patch. I mentioned the idea briefly to
Robbie.
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.

+    for mirror in additional_trybot_mirrors:
+      mirror_config = self._get_bot_config_internal(mirror['mastername'],
+                                                    mirror['buildername'])
+      if not mirror_config:
+        raise '%s not found for master %s' % (
+          mirror['mastername'], mirror['buildername'])  # pragma: no cover
+      if mirror_config.get(property) != bot_config.get(property):
+        raise ('%s was different between (%s, %s) and ' +
+               'additional trybot mirror (%s, %s)' %
+               (property, mastername, buildername, mirror['mastername'],
+                mirror['buildername']))  # pragma: no cover
+
+  def configure_build(self, mastername, buildername,
+                      additional_trybot_mirrors=None,
+                      override_bot_type=None):
+    bot_config = self._get_bot_config_internal(mastername, buildername)
+    all_bot_configs = [bot_config]
+    if additional_trybot_mirrors:
+      for mirror in additional_trybot_mirrors:
+        mirror_config = self._get_bot_config_internal(mirror['mastername'],
+                                                      mirror['buildername'])
+        all_bot_configs.append(mirror_config)
 
     # Get the buildspec version. It can be supplied as a build property or as
     # a recipe config value.
@@ -85,12 +116,18 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
         bot_config.get('chromium_config'),
         **bot_config.get('chromium_config_kwargs', {}))
 
+    self._verify_additional_trybot_mirror_property(
+      mastername, buildername, additional_trybot_mirrors, 'chromium_config')
+
     # 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 config in all_bot_configs:
+      self.m.chromium.c.gyp_env.GYP_DEFINES.update(
+        config.get('GYP_DEFINES', {}))
+    for config in all_bot_configs:
+      if config.get('use_isolate'):
+        self.m.isolate.set_isolate_environment(self.m.chromium.c)
+        break
 
     self.m.gclient.set_config(
         bot_config.get('gclient_config'),
@@ -98,28 +135,41 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
         BUILDSPEC_VERSION=buildspec_version,
         **bot_config.get('gclient_config_kwargs', {}))
 
+    self._verify_additional_trybot_mirror_property(
+      mastername, buildername, additional_trybot_mirrors, 'gclient_config')
+
     if 'android_config' in bot_config:
       self.m.chromium_android.configure_from_properties(
           bot_config['android_config'],
           **bot_config.get('chromium_config_kwargs', {}))
+      assert additional_trybot_mirrors == None
 
     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)
+      assert additional_trybot_mirrors == None
+
+    chromium_applied_configs = set()
+    gclient_applied_configs = set()
+    for config in all_bot_configs:
+      for c in config.get('chromium_apply_config', []):

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

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.

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

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

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

Acknowledged.

+        if not c in chromium_applied_configs:
+          self.m.chromium.apply_config(c)
+          chromium_applied_configs.add(c)
+      for c in 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(mastername, {})
     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)
+      assert additional_trybot_mirrors == None
 
     bot_type = override_bot_type or bot_config.get('bot_type', 'builder_tester')
 
@@ -208,10 +258,10 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
               bot_config.get('enable_swarming', False),
               scripts_compile_targets, builder_dict.get('test_generators', [])
           ))
-
     return freeze(master_dict)
 
   def prepare_checkout(self, mastername, buildername,
+                       additional_trybot_mirrors=None,
                        root_solution_revision=None):
     bot_config = self.get_bot_config(mastername, buildername)
 
@@ -229,7 +279,11 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
     self.set_up_swarming(mastername, buildername)
     self.runhooks(update_step)
 
-    test_spec = self.get_test_spec(mastername, buildername)
+    test_specs = [self.get_test_spec(mastername, buildername)]
+    if additional_trybot_mirrors:
+      for mirror in additional_trybot_mirrors:
+        test_specs.append(self.get_test_spec(mirror['mastername'],
+                                             mirror['buildername']))
 
     # TODO(phajdan.jr): Bots should have no generators instead.
     if bot_config.get('disable_tests'):
@@ -238,14 +292,23 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
       scripts_compile_targets = \
           self.get_compile_targets_for_scripts().json.output
 
-    master_dict = self.get_master_dict_with_dynamic_tests(
-        mastername, buildername, test_spec, scripts_compile_targets)
+    master_dicts = [
+      self.get_master_dict_with_dynamic_tests(
+        mastername, buildername, test_specs[0], scripts_compile_targets)]

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

nit: indent args by 4 spaces

+    if additional_trybot_mirrors:
+      i = 1
+      for mirror in additional_trybot_mirrors:

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

nit: for i, mirror in enumerate(additional_trybot_mirrors, start=1):

+        master_dicts.append(
+          self.get_master_dict_with_dynamic_tests(

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

nit: indent args by 4 spaces, here and below

+            mirror['mastername'], mirror['buildername'], test_specs[i],
+            scripts_compile_targets))
+        i += 1
 
     if self.m.chromium.c.lto and \
         not self.m.chromium.c.env.LLVM_FORCE_HEAD_REVISION:
       self.m.chromium.download_lto_plugin()
 
-    return update_step, master_dict, test_spec
+    return update_step, master_dicts, test_specs
 
   def generate_tests_from_test_spec(self, api, test_spec, builder_dict,
       buildername, mastername, enable_swarming, scripts_compile_targets,
@@ -323,13 +386,14 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
     return test_runner
 
   def get_compile_targets_and_tests(
-      self, mastername, buildername, master_dict, test_spec,
+      self, mastername, buildername, master_dicts,
+      additional_trybot_mirrors, test_specs,
       override_bot_type=None, override_tests=None):
     """Returns a tuple: list of compile targets and list of tests.
 
     The list of tests includes ones on the triggered testers."""
 
-    bot_config = master_dict.get('builders', {}).get(buildername)
+    bot_config = master_dicts[0].get('builders', {}).get(buildername)
     bot_type = override_bot_type or bot_config.get('bot_type', 'builder_tester')
 
     tests = [copy.deepcopy(t) for t in bot_config.get('tests', [])]
@@ -341,7 +405,7 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
 
     compile_targets = set(bot_config.get('compile_targets', []))
     tests_including_triggered = list(tests)
-    for _, builder_dict in master_dict.get('builders', {}).iteritems():
+    for _, builder_dict in master_dicts[0].get('builders', {}).iteritems():
       if builder_dict.get('parent_buildername') == buildername:
         tests_including_triggered.extend(builder_dict.get('tests', []))
 
@@ -356,8 +420,17 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
       compile_targets.add('crash_service')
 
     # Lastly, add any targets the checkout-side test spec told us to use.
-    compile_targets.update(test_spec.get(buildername, {}).get(
-        'additional_compile_targets', []))
+    for i in xrange(len(test_specs)):
+      # The first element in the test_specs array always corresponds
+      # to the mastername/buildername. Others come from the additional
+      # mirrors, which are only used by trybots.
+      test_spec = test_specs[i]
+      if i == 0:
+        current_builder = buildername
+      else:
+        current_builder = additional_trybot_mirrors[i-1]['buildername']
+      compile_targets.update(test_spec.get(current_builder, {}).get(
+          'additional_compile_targets', []))
 
     return sorted(compile_targets), tests_including_triggered
 
@@ -380,21 +453,24 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
       command(lambda name: name)
 
 
-  def compile(self, mastername, buildername, update_step, master_dict,
-              test_spec, mb_mastername=None, mb_buildername=None):
+  def compile(self, mastername, buildername, update_step, master_dicts,
+              additional_trybot_mirrors,
+              test_specs, mb_mastername=None, mb_buildername=None):
     """Runs compile and related steps for given builder."""
     compile_targets, tests_including_triggered = \
         self.get_compile_targets_and_tests(
             mastername,
             buildername,
-            master_dict, test_spec)
+            master_dicts,
+            additional_trybot_mirrors,
+            test_specs)
     self.compile_specific_targets(
-        mastername, buildername, update_step, master_dict,
+        mastername, buildername, update_step, master_dicts,
         compile_targets, tests_including_triggered,
         mb_mastername=mb_mastername, mb_buildername=mb_buildername)
 
   def compile_specific_targets(
-      self, mastername, buildername, update_step, master_dict,
+      self, mastername, buildername, update_step, master_dicts,
       compile_targets, tests_including_triggered,
       mb_mastername=None, mb_buildername=None, override_bot_type=None):
     """Runs compile and related steps for given builder.
@@ -413,8 +489,8 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
     (mb_mastername, mb_buildername) = (mastername, buildername) to exactly match
     a given continuous builder."""
 
-    bot_config = master_dict.get('builders', {}).get(buildername)
-    master_config = master_dict.get('settings', {})
+    bot_config = master_dicts[0].get('builders', {}).get(buildername)
+    master_config = master_dicts[0].get('settings', {})
     bot_type = override_bot_type or bot_config.get('bot_type', 'builder_tester')
 
     self.m.chromium.cleanup_temp()
@@ -493,7 +569,7 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
                 self.m.chromium.c.HOST_PLATFORM == 'mac'),
         )
 
-      for loop_buildername, builder_dict in sorted(master_dict.get(
+      for loop_buildername, builder_dict in sorted(master_dicts[0].get(
           'builders', {}).iteritems()):
         if builder_dict.get('parent_buildername') == buildername:
           trigger_spec = {
@@ -525,11 +601,11 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
     self.m.chromium.compile(compile_targets, name='compile%s' % name_suffix)
 
   def download_and_unzip_build(self, mastername, buildername, update_step,
-                               master_dict, build_archive_url=None,
+                               master_dicts, build_archive_url=None,
                                build_revision=None, override_bot_type=None):
     # We only want to do this for tester bots (i.e. those which do not compile
     # locally).
-    bot_type = override_bot_type or master_dict.get('builders', {}).get(
+    bot_type = override_bot_type or master_dicts[0].get('builders', {}).get(
         buildername, {}).get('bot_type')
     if bot_type != 'tester':
       return
@@ -553,7 +629,7 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
     build_archive_url = build_archive_url or self.m.properties.get(
         'parent_build_archive_url')
     if build_archive_url is None:
-      master_config = master_dict.get('settings', {})
+      master_config = master_dicts[0].get('settings', {})
       legacy_build_url = self._make_legacy_build_url(master_config, mastername)
 
     self.m.archive.download_and_unzip_build(