Modify annotated_run to read factory configs from the slave checkout.

scripts/slave/annotated_run.py

Index: scripts/slave/annotated_run.py
diff --git a/scripts/slave/annotated_run.py b/scripts/slave/annotated_run.py
index 136e84bde1fd9c6e37f5af71d4a00c1f94b58d0c..f748d2ba2e6b89dd604c5ad2eaaaa1bb1ba8952c 100755
--- a/scripts/slave/annotated_run.py
+++ b/scripts/slave/annotated_run.py
@@ -3,6 +3,7 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import json
 import optparse
 import os
 import subprocess
@@ -15,44 +16,105 @@ sys.path.append(os.path.join(BUILD_ROOT, 'third_party'))
 
 from common import annotator
 from common import chromium_utils
+from common import master_cfg_utils
 from slave import recipe_universe
 
 from recipe_engine import main as recipe_main
 
 
-def get_recipe_properties(factory_properties, build_properties):
+def get_recipe_properties(factory_properties, build_properties,
+                          master_overrides_slave):
   """Constructs the recipe's properties from buildbot's properties.
 
-  This merges factory_properties and build_properties.  Furthermore, it
-  tries to reconstruct the 'recipe' property from builders.pyl if it isn't
-  already there, and in that case merges in properties form builders.pyl.
+  This retrieves the current factory properties from the master_config
+  in the slave's checkout (the factory properties handed to us from the
+  master might be out of date), and merges in the build properties.
+
+  Using the values from the checkout allows us to do things like change
+  the recipe and other factory properties for a builder without needing
+  a master restart.
   """
-  properties = factory_properties.copy()
-  properties.update(build_properties)
-
-  # Try to reconstruct the recipe from builders.pyl if not given.
-  if 'recipe' not in properties:
-    mastername = properties['mastername']
-    buildername = properties['buildername']
-
-    master_path = chromium_utils.MasterPath(mastername)
-    builders_file = os.path.join(master_path, 'builders.pyl')
-    if os.path.isfile(builders_file):
-      builders = chromium_utils.ReadBuildersFile(builders_file)
-      assert buildername in builders['builders'], (
-        'buildername %s is not listed in %s' % (buildername, builders_file))
-      builder = builders['builders'][buildername]
-
-      # Update properties with builders.pyl data.
-      properties['recipe'] = builder['recipe']
-      properties.update(builder.get('properties', {}))
-    else:
-      raise LookupError('Cannot find recipe for %s on %s' %
-                        (build_properties['buildername'],
-                        build_properties['mastername']))
+  master_properties = factory_properties.copy()
+  master_properties.update(build_properties)
+
+  mastername = master_properties['mastername']
+  buildername = master_properties['buildername']

[iannucci, 2015/06/12 17:46:59]

these may not be set if we're doing run_recipe (i.e. use .get() )

+  if mastername and buildername:
+    slave_properties = get_factory_properties_from_disk(mastername, buildername)
+  else:
+    slave_properties = {}
+
+  properties = master_properties.copy()
+  conflicting_properties = {}
+  for name in slave_properties:
+    if master_properties.get(name) != slave_properties[name]:
+      conflicting_properties[name] = (master_properties.get(name),
+                                   slave_properties[name])

[Dirk Pranke, 2015/06/12 03:01:53]

If the master has a property set and the slave doesn't, should we use the
master's version in the non-override case? it might have been removed in a newer
version of the master.cfg.

The code will currently use the master's version, i.e., it doesn't delete keys
that the slave doesn't have. Obviously we would only want to do this for factory
properties, not build properties, but in run_recipe.py, at least, we can't tell
which is which, since we use the same dict for both.

[Paweł Hajdan Jr., 2015/06/12 09:24:11]

Good point. How about making this a _complete_ override, i.e. if
--master-overrides-slave is passed, do not even attempt to read the values on
the slave? Just take everything from the "master", in this case run_recipe.py
command line.

[Dirk Pranke, 2015/06/12 15:42:37]

That's actually the opposite of what I'm worried about; I'm worried about how
to have the slave override the master and delete a key.

I'm not sure in what situation we'd want to do what you're suggesting; I think
it would be only in the case where the slave's version sets a property value but
you want to test what happens when it isn't set.

I'm a bit concerned at this point that we're just guessing as to what we might
need to do, and may end up overdesigning something. 

[iannucci, 2015/06/12 17:46:59]

It may be worth it to just move this logic to run_recipe then (in another CL).
Have a --lookup option which gets passed through to annotated_run, and then we
know that the properties on the command line are meant to be overrides of what's
in the factory. Doing a key deletion could be specified by `key=`.

+
+  if conflicting_properties:
+    print >> sys.stderr, (
+        'The following build properties differ between master and slave:')
+    for name, (master_value, slave_value) in conflicting_properties.items():
+      print >> sys.stderr, '  "%s": master: "%s", slave: "%s"' % (
+          name,
+          "<unset>" if (master_value is None) else master_value,
+          slave_value)
+    print >> sys.stderr, ("Using the values from the %s." %
+                          ("master" if master_overrides_slave else "slave"))
+
+  if not master_overrides_slave:
+    for name, (_, slave_value) in conflicting_properties.items():
+      properties[name] = slave_value
+
   return properties
 
 
+def get_factory_properties_from_disk(mastername, buildername):
+  master_list = master_cfg_utils.GetMasters()
+  master_path = None
+  for name, path in master_list:
+    if name == mastername:
+      master_path = path

[Dirk Pranke, 2015/06/12 03:01:53]

it's a bit lame to have to import master_cfg_utils and do this lookup; perhaps
we should modify dump_master_cfg to do this instead?

[Paweł Hajdan Jr., 2015/06/12 09:24:11]

I'd be fine with that.

[Dirk Pranke, 2015/06/12 15:42:37]

Okay. I think I'd like to make that change in a follow-up CL if that's okay.

[iannucci, 2015/06/12 17:46:59]

sgtm

+
+  if not master_path:
+    raise LookupError('master "%s" not found.' % mastername)
+
+  script_path = os.path.join(BUILD_ROOT, 'scripts', 'tools',
+                             'dump_master_cfg.py')
+  dump_cmd = [sys.executable,
+              script_path,
+              master_path, '-']
+  proc = subprocess.Popen(dump_cmd, cwd=BUILD_ROOT, stdout=subprocess.PIPE)
+  out, _ = proc.communicate()
+  exit_code = proc.returncode
+
+  if exit_code:
+    raise LookupError('Failed to get the master config; dump_master_cfg %s'
+                      'returned %d):\n%s\n'% (
+                      mastername, exit_code, out))
+
+  config = json.loads(out)
+
+  # Now extract just the factory properties for the requested builder
+  # from the master config.
+  props = {}
+  for builder_dict in config['builders']:
+    if builder_dict['name'] == buildername:
+      factory_properties = builder_dict['factory']['properties']
+      for name, (value, source) in factory_properties.items():
+        props[name] = value
+
+  if not props:
+    raise LookupError('builder "%s" not found on in master "%s"' %
+                      (buildername, mastername))
+
+  if 'recipe' not in props:
+    raise LookupError('Cannot find recipe for %s on %s' %
+                      (buildername, mastername))
+
+  return props
+
+
 def get_args(argv):
   """Process command-line arguments."""
 
@@ -76,6 +138,9 @@ def get_args(argv):
                     help='factory properties in b64 gz JSON format')
   parser.add_option('--keep-stdin', action='store_true', default=False,
                     help='don\'t close stdin when running recipe steps')
+  parser.add_option('--master-overrides-slave', action='store_true',
+                    help='use the property values given on the command line '
+                         'from the master, not the ones looked up on the slave')
   return parser.parse_args(argv)
 
 
@@ -113,7 +178,8 @@ def update_scripts():
 def main(argv):
   opts, _ = get_args(argv)
   properties = get_recipe_properties(
-      opts.factory_properties, opts.build_properties)
+      opts.factory_properties, opts.build_properties,
+      opts.master_overrides_slave)
   stream = annotator.StructuredAnnotationStream()
   ret = recipe_main.run_steps(properties, stream,
                               universe=recipe_universe.get_universe())