Keep track of hashes triggered instead of builds.

scripts/slave/gatekeeper_ng.py

Index: scripts/slave/gatekeeper_ng.py
diff --git a/scripts/slave/gatekeeper_ng.py b/scripts/slave/gatekeeper_ng.py
index 99dd0c13d3153570309383cb34945b54852a9b87..596157f12ac37f08094e567404a5c7bd87d8fb5a 100755
--- a/scripts/slave/gatekeeper_ng.py
+++ b/scripts/slave/gatekeeper_ng.py
@@ -38,6 +38,10 @@ SCRIPTS_DIR = os.path.join(os.path.dirname(os.path.abspath(__file__)),
 SUCCESS, WARNINGS, FAILURE, SKIPPED, EXCEPTION, RETRY = range(6)
 
 
+# Bump each time there is an incompatible change in build_db.
+BUILD_DB_VERSION = 1
+
+
 def get_pwd(password_file):
   if os.path.isfile(password_file):
     return open(password_file, 'r').read().strip()
@@ -66,10 +70,10 @@ def get_root_json(master_url):
     return json.load(f)
 
 
-def find_new_builds(master_url, root_json, build_db):
+def find_new_builds(master_url, root_json, build_db, options):

[iannucci, 2014/02/20 03:30:55]

GREAT EVIL!!!

[ghost stip (do not use), 2014/02/22 10:03:07]

Done.

   """Given a dict of previously-seen builds, find new builds on each builder.
 
-  Note that we use the 'cachedBuilds here since it should be faster, and this
+  Note that we use the 'cachedBuilds' here since it should be faster, and this
   script is meant to be run frequently enough that it shouldn't skip any builds.
 
   'Frequently enough' means 1 minute in the case of Buildbot or cron, so the
@@ -80,39 +84,58 @@ def find_new_builds(master_url, root_json, build_db):
   """
   new_builds = {}
   build_db[master_url] = build_db.get(master_url, {})
+
+  last_finished_build = {}
+  for builder, builds in build_db[master_url].iteritems():
+    if any(b.get('finished') for b in builds):
+      last_finished_build[builder] = max(
+          b['build'] for b in builds if b.get('finished'))

[iannucci, 2014/02/20 03:30:55]

lame to iterate over builds twice, but still O(N) :P

[ghost stip (do not use), 2014/02/22 10:03:07]

Done.

+
   for buildername, builder in root_json['builders'].iteritems():
     candidate_builds = set(builder['cachedBuilds'] + builder['currentBuilds'])

[iannucci, 2014/02/20 03:30:55]

comment: cachedBuilds == finishedBuilds

[ghost stip (do not use), 2014/02/22 10:03:07]

Done.

-    if buildername in build_db[master_url]:
-      new_builds[buildername] = [x for x in candidate_builds
-                                 if x > build_db[master_url][buildername]]
+    if buildername in last_finished_build:
+      new_builds[buildername] = [{'build': x} for x in candidate_builds
+                                 if x > last_finished_build[buildername]]
     else:
-      new_builds[buildername] = candidate_builds
+      if (buildername in build_db[master_url] or
+          options.process_finished_builds_on_new_builder):
+        # Scan finished builds as well as unfinished.
+        new_builds[buildername] = [{'build': x} for x in candidate_builds]

[iannucci, 2014/02/20 03:30:55]

should have structs or namedtuples or classes or something for building up this
db

[ghost stip (do not use), 2014/02/22 10:03:07]

Done.

+      else:
+        # New builder or master, ignore past builds.
+        new_builds[buildername] = [
+            {'build': x} for x in builder['currentBuilds']]
 
-    # This is a heuristic, as currentBuilds may become completed by the time we
-    # scan them. The buildDB is fixed up later to account for this.
-    completed = set(builder['cachedBuilds']) - set(builder['currentBuilds'])
-    if completed:
-      build_db[master_url][buildername] = max(completed)
+    # Update build_db but don't duplicate builds already in there.
+    for build in new_builds.get(buildername, []):
+      build_db_builds = build_db[master_url].setdefault(buildername, [])
+      if not any(x['build'] == build['build'] for x in build_db_builds):
+        build_db_builds.append(build)
+
+    # Pull old + new unfinished builds from build_db.
+    new_builds[buildername] = [
+        b for b in build_db[master_url].setdefault(buildername, [])
+        if not b.get('finished')]
 
   return new_builds
 
 
-def find_new_builds_per_master(masters, build_db):
+def find_new_builds_per_master(masters, build_db, options):
   """Given a list of masters, find new builds and collect them under a dict."""
   builds = {}
   master_jsons = {}
   for master in masters:
     root_json = get_root_json(master)
     master_jsons[master] = root_json
-    builds[master] = find_new_builds(master, root_json, build_db)
+    builds[master] = find_new_builds(master, root_json, build_db, options)
   return builds, master_jsons
 
 
-def get_build_json(url_pair):
-  url, master = url_pair
+def get_build_json(url_tuple):
+  url, master, builder, build = url_tuple
   logging.debug('opening %s...' % url)
   with closing(urllib2.urlopen(url)) as f:
-    return json.load(f), master
+    return json.load(f), master, builder, build
 
 
 def get_build_jsons(master_builds, build_db, processes):
@@ -127,34 +150,52 @@ def get_build_jsons(master_builds, build_db, processes):
     for builder, new_builds in builder_dict.iteritems():
       for build in new_builds:
         safe_builder = urllib.quote(builder)
-        url = master + '/json/builders/%s/builds/%s' % (safe_builder, build)
-        url_list.append((url, master))
-  # The async/get is so that ctrl-c can interrupt the scans.
-  # See http://stackoverflow.com/questions/1408356/
-  # keyboard-interrupts-with-pythons-multiprocessing-pool
-  with chromium_utils.MultiPool(processes) as pool:
-    builds = filter(bool, pool.map_async(get_build_json, url_list).get(9999999))
-
-  for build_json, master in builds:
+        url = master + '/json/builders/%s/builds/%s' % (safe_builder,
+                                                        build['build'])
+        url_list.append((url, master, builder, build))
+  # Prevent map from hanging, see http://bugs.python.org/issue12157.
+  if url_list:
+    # The async/get is so that ctrl-c can interrupt the scans.
+    # See http://stackoverflow.com/questions/1408356/
+    # keyboard-interrupts-with-pythons-multiprocessing-pool
+    with chromium_utils.MultiPool(processes) as pool:
+      builds = filter(bool, pool.map_async(get_build_json, url_list).get(
+          9999999))
+  else:
+    builds = []
+
+  # Pools pickle and unpickle, which means the build object we use isn't the
+  # real build object. We recover it here so we can modify build_db later in the
+  # program.
+  def find_original_build(master, builder, build):
+    return next(b for b in build_db[master][builder]
+                if b['build'] == build['build'])
+  builds = [(u, m, find_original_build(m, bd, bl))
+            for u, m, bd, bl in builds]

[iannucci, 2014/02/20 03:30:55]

zip url_list with the rest of the data so that you don't need this fixup loop...
or just use the build number as the key of the hash.

[ghost stip (do not use), 2014/02/22 10:03:07]

Done.

+
+  # This is needed for the --sync-db option.
+  for build_json, master, build in builds:
     if build_json.get('results', None) is not None:
-      build_db[master][build_json['builderName']] = max(
-          build_json['number'],
-          build_db[master][build_json['builderName']])
+      build['finished'] = True
   return builds
 
 
-def check_builds(master_builds, master_jsons, build_db, gatekeeper_config):
+def check_builds(master_builds, master_jsons, gatekeeper_config):
   """Given a gatekeeper configuration, see which builds have failed."""
   failed_builds = []
-  for build_json, master_url in master_builds:
+  for build_json, master_url, build in master_builds:
     gatekeeper_sections = gatekeeper_config.get(master_url, [])
     for gatekeeper_section in gatekeeper_sections:
+      section_hash = gatekeeper_ng_config.gatekeeper_section_hash(
+          gatekeeper_section)
+
       if build_json['builderName'] in gatekeeper_section:
         gatekeeper = gatekeeper_section[build_json['builderName']]
       elif '*' in gatekeeper_section:
         gatekeeper = gatekeeper_section['*']
       else:
         gatekeeper = {}
+
       steps = build_json['steps']
       forgiving = set(gatekeeper.get('forgiving_steps', []))
       forgiving_optional = set(gatekeeper.get('forgiving_optional', []))
@@ -200,6 +241,7 @@ def check_builds(master_builds, master_jsons, build_db, gatekeeper_config):
 
       logging.debug('%sbuilders/%s/builds/%d ----', buildbot_url,
                     build_json['builderName'], build_json['number'])
+      logging.debug('  section hash: %s', section_hash)
       logging.debug('  build steps: %s', ', '.join(s['name'] for s in steps))
       logging.debug('  closing steps: %s', ', '.join(closing_steps))
       logging.debug('  closing optional steps: %s', ', '.join(closing_optional))
@@ -210,24 +252,27 @@ def check_builds(master_builds, master_jsons, build_db, gatekeeper_config):
       logging.debug('  unsatisfied steps: %s', ', '.join(unsatisfied_steps))
       logging.debug('  set to close tree: %s', close_tree)
       logging.debug('  build failed: %s', bool(unsatisfied_steps))
-      logging.debug('----')
 
 
       if unsatisfied_steps:
-        build_db[master_url][build_json['builderName']] = max(
-            build_json['number'],
-            build_db[master_url][build_json['builderName']])
-
-        failed_builds.append({'base_url': buildbot_url,
-                              'build': build_json,
-                              'close_tree': close_tree,
-                              'forgiving_steps': forgiving | forgiving_optional,
-                              'project_name': project_name,
-                              'sheriff_classes': sheriff_classes,
-                              'subject_template': subject_template,
-                              'tree_notify': tree_notify,
-                              'unsatisfied': unsatisfied_steps,
-                             })
+        if section_hash in build.get('triggered', []):
+          logging.debug('  section has already been triggered for this build, '
+                        'skipping...')
+        else:
+          build.setdefault('triggered', []).append(section_hash)
+
+          failed_builds.append({'base_url': buildbot_url,
+                                'build': build_json,
+                                'close_tree': close_tree,
+                                'forgiving_steps': (
+                                    forgiving | forgiving_optional),
+                                'project_name': project_name,
+                                'sheriff_classes': sheriff_classes,
+                                'subject_template': subject_template,
+                                'tree_notify': tree_notify,
+                                'unsatisfied': unsatisfied_steps,
+                               })
+      logging.debug('----')
 
   return failed_builds
 
@@ -406,18 +451,60 @@ def get_build_db(filename):
     with open(filename) as f:
       build_db = json.load(f)
 
-  return build_db or {}
+  if build_db and build_db.get('build_db_version', 0) != BUILD_DB_VERSION:
+    new_fn = '%s.old' % filename
+    logging.warn('%s is an older db version: %d (expecting %d). moving to '
+                 '%s' % (filename, build_db.get('build_db_version', 0),
+                         BUILD_DB_VERSION, new_fn))
+    chromium_utils.MoveFile(filename, new_fn)
+    build_db = None
+
+  if build_db and 'masters' in build_db:
+    return build_db['masters']
+  return {}
 
 
-def save_build_db(build_db, filename):
+def save_build_db(build_db_data, gatekeeper_config, filename):
   """Save the build_db file.
 
   build_db: dictionary to jsonize and store as build_db.
   filename: the filename of the build db.
+  gatekeeper_config: the gatekeeper config used for this pass.
   """
   print 'saving build_db to', filename
+
+  # Remove all but the last finished build.
+  for builders in build_db_data.values():
+    for builder in builders:
+      if any(b.get('finished') for b in builders[builder]):
+        last_finished_build = max(
+            b['build'] for b in builders[builder] if b.get('finished'))
+        builders[builder] = [
+            b for b in builders[builder] if (b['build'] == last_finished_build
+                                             or not b.get('finished'))]
+
+  build_db = {
+      'build_db_version': BUILD_DB_VERSION,
+      'masters': build_db_data,
+      'sections': {},
+  }
+
+  # Output the gatekeeper sections we're operating with, so a human reading the
+  # file can debug issues. This is discarded by the parser in get_build_db.
+  used_sections = set([])
+  for builders in build_db_data.values():
+    for builds in builders.values():
+      used_sections |= reduce(
+          lambda x, y: x | set(y.get('triggered', [])), builds, set([]))

[iannucci, 2014/02/20 03:30:55]

{ t 
  for b in builds
  for t in b.get('triggered', []) 
}

[ghost stip (do not use), 2014/02/22 10:03:07]

Done.

+  for master in gatekeeper_config.values():
+    for section in master:
+      section_hash = gatekeeper_ng_config.gatekeeper_section_hash(section)
+      if (section_hash in used_sections and
+          section_hash not in build_db['sections']):

[iannucci, 2014/02/20 03:30:55]

Use the sets, luke.

[ghost stip (do not use), 2014/02/22 10:03:07]

dude you just unlocked an entire set of lgtm sagas for luqui@ (see
https://chromiumcodereview.appspot.com/71553003/#msg11)

+        build_db['sections'][section_hash] = section
+
   with open(filename, 'wb') as f:
-    json.dump(build_db, f)
+    gatekeeper_ng_config.flatten_to_json(build_db, f)
 
 
 def get_options():
@@ -473,6 +560,10 @@ def get_options():
                     help='display flattened gatekeeper.json for debugging')
   parser.add_option('--no-hashes', action='store_true',
                     help='don\'t insert gatekeeper section hashes')
+  parser.add_option('--process-finished-builds-on-new-builder',
+                    action='store_true',
+                    help='when processing a new builder, process finished '
+                         'builds')
   parser.add_option('-v', '--verbose', action='store_true',
                     help='turn on extra debugging information')
 
@@ -535,17 +626,19 @@ def main():
 
   if options.clear_build_db:
     build_db = {}
-    save_build_db(build_db, options.build_db)
+    save_build_db(build_db, gatekeeper_config, options.build_db)
   else:
     build_db = get_build_db(options.build_db)
 
-  new_builds, master_jsons = find_new_builds_per_master(masters, build_db)
+  new_builds, master_jsons = find_new_builds_per_master(masters, build_db,
+                                                        options)
+  build_jsons = get_build_jsons(new_builds, build_db, options.parallelism)
+
   if options.sync_build_db:
-    save_build_db(build_db, options.build_db)
+    save_build_db(build_db, gatekeeper_config, options.build_db)
     return 0
-  build_jsons = get_build_jsons(new_builds, build_db, options.parallelism)
-  failed_builds = check_builds(build_jsons, master_jsons, build_db,
-                               gatekeeper_config)
+
+  failed_builds = check_builds(build_jsons, master_jsons, gatekeeper_config)
   if options.set_status:
     options.password = get_pwd(options.password_file)
 
@@ -557,7 +650,7 @@ def main():
                         options.disable_domain_filter)
 
   if not options.skip_build_db_update:
-    save_build_db(build_db, options.build_db)
+    save_build_db(build_db, gatekeeper_config, options.build_db)
 
   return 0