Add support for "additional_mirrors" property in trybot configs.

scripts/slave/recipe_modules/chromium_tests/api.py

 # Copyright 2014 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 import ast
 import collections
 import contextlib
 import copy
 import itertools
 import json
@@ skipping 50 matching lines @@
     return steps
 
   @property
   def trybots(self):
     return trybots.TRYBOTS
 
   def add_builders(self, builders):
     """Adds builders to our builder map"""
     self._builders.update(builders)
 
+  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 master_dict.get('builders', {}).get(buildername)
+
   def get_bot_config(self, mastername, buildername):
-    master_dict = self.builders.get(mastername, {})
-    return freeze(master_dict.get('builders', {}).get(buildername))
+    return freeze(self._get_bot_config_internal(mastername, 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 _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.
     buildspec_version = (self.m.properties.get('buildspec_version') or
                          bot_config.get('buildspec_version'))
 
     self.m.chromium.set_config(
         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'),
         PATCH_PROJECT=self.m.properties.get('patch_project'),
         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'])
+      assert additional_trybot_mirrors == None
 
-    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)
+    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')
 
     if bot_config.get('set_component_rev'):
       # 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:
       # (1) the revision of the builder if this is a tester,
       # (2) 'revision' from the waterfall, or
       # (3) 'HEAD' for forced builds with unspecified 'revision'.
@@ skipping 68 matching lines @@
           dict(builders[loop_buildername]))
       builders[loop_buildername]['tests'] = (
           self.generate_tests_from_test_spec(
               self.m, test_spec, builder_dict, loop_buildername, mastername,
               # TODO(phajdan.jr): Get enable_swarming value from builder_dict.
               # Above should remove the need to get bot_config and buildername
               # in this method.
               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)
 
     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 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'
 
     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'):
       scripts_compile_targets = {}
     else:
       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,
       generators):
     tests = builder_dict.get('tests', ())
     # TODO(phajdan.jr): Switch everything to scripts generators and simplify.
     for generator in generators:
       tests = (
           tuple(generator(api, mastername, buildername, test_spec,
                          enable_swarming=enable_swarming,
@@ skipping 57 matching lines @@
             raise
 
       if failed_tests:
         failed_tests_names = [t.name for t in failed_tests]
         raise self.m.step.StepFailure(
             '%d tests failed: %r' % (len(failed_tests), failed_tests_names))
 
     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', [])]
     if override_tests is not None:
       tests = override_tests
 
     if bot_type not in ['builder', 'builder_tester']:
       return [], []
 
     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', []))
 
     if bot_config.get('add_tests_as_compile_targets', True):
       for t in tests_including_triggered:
         compile_targets.update(t.compile_targets(self.m))
 
     # Only add crash_service when we have explicit compile targets.
     if (self.m.platform.is_win and
         compile_targets and
         'all' not in compile_targets):
       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
 
   def transient_check(self, update_step, command):
     """Runs command, checking for transience if this is a try job.
 
     * command is a function which takes an argument of type (str -> str),
       which is a test name transformation (it adds "with patch" or "without
       patch") and runs the command.
     * update_step is the bot_update step used for deapplying the patch.
     """
     if self.m.tryserver.is_tryserver:
       try:
         command(lambda name: '%s (with patch)' % name)
       except self.m.step.StepFailure:
         self.deapply_patch(update_step)
         command(lambda name: '%s (without patch)' % name)
         raise
     else:
       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.
 
     Allows finer-grained control about exact compile targets used.
 
     We don't use the given `mastername` and `buildername` to run MB, because
     they may be the values of the continuous builder the trybot may be
     configured to match; instead we need to use the actual mastername and
     buildername we're running on (Default to the "mastername" and
     "buildername" in the build properties -- self.m.properties, but could be
     overridden by `mb_mastername` and `mb_buildername`), because it may be
     configured with different MB settings.
 
     However, recipes used by Findit for culprit finding may still set
     (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()
     if self.m.chromium.c.TARGET_PLATFORM == 'android':
       self.m.chromium_android.clean_local_files()
       self.m.chromium_android.run_tree_truth()
 
     if bot_type in ['builder', 'builder_tester']:
       isolated_targets = [
           t.isolate_target(self.m)
@@ skipping 58 matching lines @@
             build_url=self._build_gs_archive_url(
                 mastername, master_config, buildername),
             build_revision=build_revision,
             cros_board=self.m.chromium.c.TARGET_CROS_BOARD,
             # TODO(machenbach): Make asan a configuration switch.
             package_dsym_files=(
                 self.m.chromium.c.gyp_env.GYP_DEFINES.get('asan') and
                 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 = {
             'builder_name': loop_buildername,
             'properties': {},
           }
           for name, value in update_step.presentation.properties.iteritems():
             if name.startswith('got_'):
               trigger_spec['properties']['parent_' + name] = value
           self.m.trigger(trigger_spec)
@@ skipping 11 matching lines @@
     if self.m.chromium.c.project_generator.tool == 'mb':
       mb_mastername = mb_mastername or self.m.properties['mastername']
       mb_buildername = mb_buildername or self.m.properties['buildername']
       self.m.chromium.run_mb(mb_mastername, mb_buildername,
                              isolated_targets=isolated_targets,
                              name='generate_build_files%s' % name_suffix)
 
     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
 
     # Protect against hard to debug mismatches between directory names
     # used to run tests from and extract build to. We've had several cases
     # where a stale build directory was used on a tester, and the extracted
     # build was not used at all, leading to confusion why source code changes
     # are not taking effect.
     #
     # The best way to ensure the old build directory is not used is to
     # remove it.
     self.m.file.rmtree(
       'build directory',
       self.m.chromium.c.build_dir.join(self.m.chromium.c.build_config_fs))
 
     legacy_build_url = None
     got_revision = update_step.presentation.properties['got_revision']
     build_revision = build_revision or self.m.properties.get(
         'parent_got_revision') or got_revision
     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(
       step_name='extract build',
       target=self.m.chromium.c.build_config_fs,
       build_url=legacy_build_url,
       build_revision=build_revision,
       build_archive_url=build_archive_url)
 
   def tests_for_builder(self, mastername, buildername, update_step, master_dict,
@@ skipping 308 matching lines @@
       result_text = 'MB is enabled for this builder at this revision.'
       log_name = 'Builder MB-ready'
       self.m.step.active_result.presentation.logs[log_name] = [result_text]
       return False
     except (self.m.step.StepFailure, KeyError):
       result_text = 'MB is not enabled for this builder at this revision.'
       log_name = 'Builder NOT MB-ready'
       self.m.step.active_result.presentation.logs[log_name] = [result_text]
       self.m.step.active_result.presentation.status = self.m.step.WARNING
       return True