git cl try-results: show buildbucket tryjobs.

git_cl.py

 #!/usr/bin/env python
 # Copyright (c) 2012 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.
 
 # Copyright (C) 2008 Evan Martin <martine@danga.com>
 
 """A git-command for integrating reviews on Rietveld."""
 
 from distutils.version import LooseVersion
 from multiprocessing.pool import ThreadPool
 import base64
 import collections
 import glob
 import httplib
 import json
 import logging
 import optparse
 import os
 import Queue
 import re
 import stat
 import sys
 import tempfile
 import textwrap
 import time
 import traceback
+import urllib
 import urllib2
 import urlparse
 import webbrowser
 import zlib
 
 try:
   import readline  # pylint: disable=F0401,W0611
 except ImportError:
   pass
 
@@ skipping 194 matching lines @@
   name, while the developers always use shortened master name
   (tryserver.chromium.linux) by stripping off the prefix 'master.'. This
   function does the conversion for buildbucket migration.
   """
   prefix = 'master.'
   if master.startswith(prefix):
     return master
   return '%s%s' % (prefix, master)
 
 
+def _buildbucket_retry(operation_name, http, *args, **kwargs):
+  """Retries requests to buildbucket service and returns parsed json content."""
+  for try_count in xrange(3):

[tandrii(chromium), 2016/02/25 17:59:33]

this is a copy of code from before.

+    response, content = http.request(*args, **kwargs)
+    try:
+      content_json = json.loads(content)
+    except ValueError:
+      content_json = None
+
+    # Buildbucket could return an error even if status==200.
+    if content_json and content_json.get('error'):
+      msg = 'Error in response. Code: %s. Reason: %s. Message: %s.' % (
+          content_json['error'].get('code', ''),

[nodir, 2016/02/25 17:14:52]

there is no code, only reason and message
https://docs.google.com/document/d/10BH6oSq_UEtdSdhrguEuV4hx-JxY7jzFcgs1OCHcn...

[tandrii(chromium), 2016/02/25 17:59:33]

ok, ok.

+          content_json['error'].get('reason', ''),
+          content_json['error'].get('message', ''))
+      raise BuildbucketResponseException(msg)
+
+    if response.status == 200:
+      if not content_json:
+        raise BuildbucketResponseException(
+            'Buildbucket returns invalid json content: %s.\n'
+            'Please file bugs at http://crbug.com, label "Infra-BuildBucket".' %
+            content)
+      return content_json
+    if response.status < 500 or try_count >= 2:

[nodir, 2016/02/25 17:14:52]

you have two checks for try_count value, here and in the loop header. I
recommend replacing the loop with `while True`

[tandrii(chromium), 2016/02/25 17:59:33]

DOne, but see https://codereview.chromium.org/1063103002/#ps320001 :)

+      raise httplib2.HttpLib2Error(content)
+
+    # status >= 500 means transient failures.
+    logging.debug('Transient errors when %s. Will retry.', operation_name)
+    time.sleep(0.5 + 1.5*try_count)
+  assert False, 'unreachable'
+
+
 def trigger_luci_job(changelist, masters, options):
   """Send a job to run on LUCI."""
   issue_props = changelist.GetIssueProperties()
   issue = changelist.GetIssue()
   patchset = changelist.GetMostRecentPatchset()
   for builders_and_tests in sorted(masters.itervalues()):
     # TODO(hinoka et al): add support for other properties.
     # Currently, this completely ignores testfilter and other properties.
     for builder in sorted(builders_and_tests):
       luci_trigger.trigger(
@@ skipping 46 matching lines @@
       }
       if tests:
         parameters['properties']['testfilter'] = tests
       if properties:
         parameters['properties'].update(properties)
       if options.clobber:
         parameters['properties']['clobber'] = True
       batch_req_body['builds'].append(
           {
               'bucket': bucket,
               'parameters_json': json.dumps(parameters),

[nodir, 2016/02/25 17:14:53]

Please set client_operation_id to a random string
See
https://docs.google.com/document/d/10BH6oSq_UEtdSdhrguEuV4hx-JxY7jzFcgs1OCHcn...

[tandrii(chromium), 2016/02/25 17:59:33]

outside of CL scope - this is a copy of old code.

That said, how does setting it to random string different from now? If it's
random, it's ~never same, hence always schedules new build.

[nodir, 2016/02/25 18:31:06]

it is the same in the retry loop, so if a build was created in datastore, but
client didn't receive the response and retries the request, the server will see
that client operation is the same and will not create a duplicate build

[tandrii(chromium), 2016/02/25 19:02:15]

but of course, you are perfectly right! Done.

               'tags': ['builder:%s' % builder,
                        'buildset:%s' % buildset,
                        'master:%s' % master,
                        'user_agent:git_cl_try']
           }
       )
 
-  for try_count in xrange(3):

[tandrii(chromium), 2016/02/25 17:59:33]

this is original - i didn't change this, except for the configurable request
args and logging message.

-    response, content = http.request(
-        buildbucket_put_url,
-        'PUT',
-        body=json.dumps(batch_req_body),
-        headers={'Content-Type': 'application/json'},
-    )
-    content_json = None
-    try:
-      content_json = json.loads(content)
-    except ValueError:
-      pass
-
-    # Buildbucket could return an error even if status==200.
-    if content_json and content_json.get('error'):
-      msg = 'Error in response. Code: %d. Reason: %s. Message: %s.' % (
-          content_json['error'].get('code', ''),
-          content_json['error'].get('reason', ''),
-          content_json['error'].get('message', ''))
-      raise BuildbucketResponseException(msg)
-
-    if response.status == 200:
-      if not content_json:
-        raise BuildbucketResponseException(
-            'Buildbucket returns invalid json content: %s.\n'
-            'Please file bugs at crbug.com, label "Infra-BuildBucket".' %
-            content)
-      break
-    if response.status < 500 or try_count >= 2:
-      raise httplib2.HttpLib2Error(content)
-
-    # status >= 500 means transient failures.
-    logging.debug('Transient errors when triggering tryjobs. Will retry.')
-    time.sleep(0.5 + 1.5*try_count)
-
+  _buildbucket_retry(
+      'triggering tryjobs',
+      http,
+      buildbucket_put_url,
+      'PUT',
+      body=json.dumps(batch_req_body),
+      headers={'Content-Type': 'application/json'}
+  )
   print '\n'.join(print_text)
 
 
+def fetch_try_jobs(auth_config, changelist, options):
+  """Fetches tryjobs from buildbucket.
+
+  Returns a map from build id to build info as json dictionary.
+  """
+  rietveld_url = settings.GetDefaultServerUrl()
+  rietveld_host = urlparse.urlparse(rietveld_url).hostname
+  authenticator = auth.get_authenticator_for_host(rietveld_host, auth_config)
+  http = authenticator.authorize(httplib2.Http())
+  http.force_exception_to_status_code = True
+
+  buildset = 'patch/rietveld/{hostname}/{issue}/{patch}'.format(
+      hostname=rietveld_host,
+      issue=changelist.GetIssue(),
+      patch=options.patchset)
+  params = {'tag': 'buildset:%s' % buildset}
+
+  login_required_error = None
+  builds = {}
+  while True:
+    url = 'https://{hostname}/_ah/api/buildbucket/v1/search?{params}'.format(
+        hostname=options.buildbucket_host,
+        params=urllib.urlencode(params))
+    try:
+      content = _buildbucket_retry('fetching tryjobs', http, url, 'GET')
+    except auth.LoginRequiredError as e:
+      if login_required_error:
+        # Avoid looping forever if login is required on this buildbucket host.
+        raise
+      print 'Warning: Some results might be missing because %s' % e
+      login_required_error = e
+      http = httplib2.Http()
+      http.force_exception_to_status_code = True

[nodir, 2016/02/25 17:14:52]

From what I understand reading this code, you first try with an authenticator in
case there are cached credentials. And if it fails with LoginRequiredError
(which means there are no cached credentials), you try without the authenticator

I'd recommend to be more explicit: use authenticator.has_cached_credentials
function to check if credentials are there. If it returns true, then call
authenticator.authoize. You won't have to write this catch clause

[tandrii(chromium), 2016/02/25 17:59:33]

Good idea! Done.

+      continue
+    for build in content.get('builds', []):
+      builds[build['id']] = build
+    if 'next_cursor' in content:
+      params['start_cursor'] = content['next_cursor']
+    else:
+      break
+  return builds
+
+
+def print_tryjobs(options, builds):
+  """Prints nicely result of fetch_try_jobs."""
+  if not builds:
+    print 'No tryjobs scheduled'
+    return
+
+  # Make a copy, because we'll be modifying builds dictionary.
+  builds = builds.copy()
+  def get_builder(b):

[nodir, 2016/02/25 17:14:52]

nit: blank line before func def

[tandrii(chromium), 2016/02/25 17:59:33]

Done.

+    for tag in b.get('tags', []):

[nodir, 2016/02/25 17:14:52]

Beware that someone may forget to set tags. The source of truth of builder is in
parameters_json

[tandrii(chromium), 2016/02/25 17:59:33]

Indeed, machenbach@ showed me exactly that case, and already fixed in PS5.

+      name_value = tag.split(':', 1)
+      if len(name_value) == 2 and name_value[0] == 'builder':
+        return name_value[1]
+    return None
+
+  def get_bucket(b):
+    bucket = b['bucket']
+    if bucket.startswith('master.'):
+      return bucket[len('master.'):]
+    return bucket
+
+  if options.print_master:
+    name_fmt = '%%-%ds %%-%ds' % (
+        max(len(str(get_bucket(b))) for b in builds.itervalues()),
+        max(len(str(get_builder(b))) for b in builds.itervalues()))
+    def get_name(b):
+      return name_fmt % (get_bucket(b), get_builder(b))
+  else:
+    name_fmt = '%%-%ds' % (
+        max(len(str(get_builder(b))) for b in builds.itervalues()))
+    def get_name(b):
+      return name_fmt % get_builder(b)
+
+  def sort_key(b):
+    return b['status'], b.get('result'), get_name(b), b.get('url')
+
+  def pop(title, f, color=None, **kwargs):
+    """Pop matching builds from `builds` dict and print them."""
+
+    if not sys.stdout.isatty() or color is None:
+      colorize = str
+    else:
+      colorize = lambda x: '%s%s%s' % (color, x, Fore.RESET)
+
+    result = []
+    for b in builds.values():
+      selected = True

[nodir, 2016/02/25 17:14:53]

possibly, using `any` may simplify this code

[tandrii(chromium), 2016/02/25 17:59:33]

s/any/all and done, good idea.

+      for k, v in kwargs.iteritems():
+        if b.get(k) != v:
+          selected = False
+          break
+      if selected:
+        builds.pop(b['id'])
+        result.append(b)
+    if result:
+      print colorize(title)
+      for b in sorted(result, key=sort_key):
+        print ' ', colorize('\t'.join(map(str, f(b))))
+
+  total = len(builds)
+  pop(status='COMPLETED', result='SUCCESS',
+      title='Successes:', color=Fore.GREEN,
+      f=lambda b: (get_name(b), b.get('url')))
+  pop(status='COMPLETED', result='FAILURE', failure_reason='INFRA_FAILURE',
+      title='Infra Failures:', color=Fore.MAGENTA,
+      f=lambda b: (get_name(b), b.get('url')))
+  pop(status='COMPLETED', result='FAILURE', failure_reason='BUILD_FAILURE',
+      title='Failures:', color=Fore.RED,
+      f=lambda b: (get_name(b), b.get('url')))
+  pop(status='COMPLETED', result='FAILURE',
+      failure_reason='INVALID_BUILD_DEFINITION',
+      title='Wrong master/builder name:', color=Fore.MAGENTA,
+      f=lambda b: (get_name(b),))
+  pop(status='COMPLETED', result='FAILURE',
+      title='Other failures:',
+      f=lambda b: (get_name(b), b.get('failure_reason'), b.get('url')))
+  pop(status='COMPLETED',
+      title='Other finished:',
+      f=lambda b: (get_name(b), b.get('result'), b.get('url')))

[nodir, 2016/02/25 17:14:53]

All cancelled builds will fall into this category. Maybe add a pop statement for
status='COMPLETED' result='CANCELED'?

[tandrii(chromium), 2016/02/25 17:59:33]

Done.

+  pop(status='STARTED',
+      title='Started:', color=Fore.YELLOW,
+      f=lambda b: (get_name(b), b.get('url')))
+  pop(status='SCHEDULED',
+      title='Scheduled:',
+      f=lambda b: (get_name(b), 'id=%s' % b['id']))
+  # The last section is just in case buildbucket API changes OR there is a bug.
+  pop(title='Other:',
+      f=lambda b: (get_name(b), 'id=%s' % b['id']))

[Michael Achenbach, 2016/02/25 15:20:29]

Maybe
assert len(builds) = 0
?

[tandrii(chromium), 2016/02/25 17:59:33]

Done.

+  print 'Total: %d tryjobs' % total
+
+
 def MatchSvnGlob(url, base_url, glob_spec, allow_wildcards):
   """Return the corresponding git ref if |base_url| together with |glob_spec|
   matches the full |url|.
 
   If |allow_wildcards| is true, |glob_spec| can contain wildcards (see below).
   """
   fetch_suburl, as_ref = glob_spec.split(':')
   if allow_wildcards:
     glob_match = re.match('(.+/)?(\*|{[^/]*})(/.+)?', fetch_suburl)
     if glob_match:
@@ skipping 2994 matching lines @@
 
     for (master, builders) in sorted(masters.iteritems()):
       if master:
         print 'Master: %s' % master
       length = max(len(builder) for builder in builders)
       for builder in sorted(builders):
         print '  %*s: %s' % (length, builder, ','.join(builders[builder]))
   return 0
 
 
+def CMDtry_results(parser, args):
+  group = optparse.OptionGroup(parser, "Try job results options")
+  group.add_option(
+      "-p", "--patchset", type=int, help="patchset number if not current.")
+  group.add_option(
+      "--print-master", action='store_true', help="print master name as well")
+  group.add_option(
+      "--buildbucket-host", default='cr-buildbucket.appspot.com',
+      help="Host of buildbucket. The default host is %default.")
+  parser.add_option_group(group)
+  auth.add_auth_options(parser)
+  options, args = parser.parse_args(args)
+  if args:
+    parser.error('Unrecognized args: %s' % ' '.join(args))
+
+  auth_config = auth.extract_auth_config_from_options(options)
+  cl = Changelist(auth_config=auth_config)
+  if not cl.GetIssue():
+    parser.error('Need to upload first')
+
+  if not options.patchset:
+    options.patchset = cl.GetMostRecentPatchset()
+    if options.patchset and options.patchset != cl.GetPatchset():
+      print(
+          '\nWARNING Mismatch between local config and server. Did a previous '
+          'upload fail?\ngit-cl try always uses latest patchset from rietveld. '
+          'Continuing using\npatchset %s.\n' % options.patchset)
+  try:
+    jobs = fetch_try_jobs(auth_config, cl, options)
+  except BuildbucketResponseException as ex:
+    print 'ERROR: %s' % ex

[nodir, 2016/02/25 17:14:53]

nit: 'Buildbucket error: %s'
so users direct their complaints more precisely

[tandrii(chromium), 2016/02/25 17:59:33]

Done.

+    return 1
+  except Exception as e:
+    stacktrace = (''.join(traceback.format_stack()) + traceback.format_exc())
+    print 'ERROR: Exception when trying to trigger tryjobs: %s\n%s' % (

[Michael Achenbach, 2016/02/25 15:20:29]

nit: trigger? Maybe adapt error message?

[tandrii(chromium), 2016/02/25 17:59:33]

Done.

+        e, stacktrace)
+    return 1
+  print_tryjobs(options, jobs)
+  return 0
+
+
 @subcommand.usage('[new upstream branch]')
 def CMDupstream(parser, args):
   """Prints or sets the name of the upstream branch, if any."""
   _, args = parser.parse_args(args)
   if len(args) > 1:
     parser.error('Unrecognized args: %s' % ' '.join(args))
 
   cl = Changelist()
   if args:
     # One arg means set upstream branch.
@@ skipping 382 matching lines @@
 if __name__ == '__main__':
   # These affect sys.stdout so do it outside of main() to simplify mocks in
   # unit testing.
   fix_encoding.fix_encoding()
   colorama.init()
   try:
     sys.exit(main(sys.argv[1:]))
   except KeyboardInterrupt:
     sys.stderr.write('interrupted\n')
     sys.exit(1)