Fail build if triggering fails

scripts/master/chromium_step.py

 # 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.
 
 """Subclasses of various slave command classes."""
 
 from datetime import datetime
 import copy
 import errno
 import json
@@ skipping 641 matching lines @@
   def __init__(self, command=None, show_perf=False, perf_id=None,
                perf_report_url_suffix=None, target=None, active_master=None,
                *args, **kwargs):
     buildstep.LogLineObserver.__init__(self, *args, **kwargs)
     self.command = command
     self.sections = []
     self.annotate_status = builder.SUCCESS
     self.halt_on_failure = False
     self.honor_zero_return_code = False
     self.cursor = None
+    self.fatal_errors = []  # a list of fatal error descriptions
 
     self.show_perf = show_perf
     self.perf_id = perf_id
     self.perf_report_url_suffix = perf_report_url_suffix
     self.target = target
     self.active_master = active_master
     self.bb_triggering_service = None
 
+  def record_fatal_error(self, description):
+    self.fatal_errors.append(description)
+
   def initialSection(self):
     """Initializes the annotator's sections.
 
     Annotator uses a list of dictionaries which hold information stuch as status
     and logs for each added step. This method populates the section list with an
     entry referencing the original buildbot step."""
     if self.sections:
       return
     # Add a log section for output before the first section heading.
     preamble = self.command.addLog('preamble')
@@ skipping 188 matching lines @@
         self.outReceived(line)
       elif channel == interfaces.LOG_CHANNEL_STDERR:
         self.errReceived(line)
       elif channel == interfaces.LOG_CHANNEL_HEADER:
         self.headerReceived(line)
 
   def outReceived(self, data):
     buildstep.LogLineObserver.outReceived(self, data)
     if self.sections:
       self.ensureStepIsStarted(self.cursor)
-      self.cursor['log'].addStdout(data)
+      if self.cursor['log'].finished:
+        # This might happen if the section was finished with a premature error
+        # asynchronously.
+        pass
+      else:
+        self.cursor['log'].addStdout(data)
 
   def errReceived(self, data):
     buildstep.LogLineObserver.errReceived(self, data)
     if self.sections:
       self.ensureStepIsStarted(self.cursor)
       self.cursor['log'].addStderr(data)
 
   def headerReceived(self, data):
     if self.sections:
       preamble = self.sections[0]
@@ skipping 318 matching lines @@
 
     # Sort files.
     files = change.get('files')
     if files:
       change['files'] = sorted(files)
 
     return change
 
   def STEP_TRIGGER(self, spec):
     # Support: @@@STEP_TRIGGER <json spec>@@@ (trigger build(s)).
+    builder_names = ['<unknown>']
+
+    section = self.cursor
+    critical = True
+
+    def handle_exception(ex):
+      if not section['step'].isFinished():
+        self.finishStep(section, builder.EXCEPTION, ex)
+      if critical:
+        self.record_fatal_error(
+            'step %s: could not trigger builds %r: %s' %
+            (section['name'], builder_names, ex))
+
     try:
       spec = json.loads(spec)
+      critical = spec.get('critical', True)
       bucket = spec.get('bucket')
       builder_names = spec.get('builderNames')
       properties = spec.get('properties') or {}
       changes = spec.get('changes')
       tags = spec.get('tags') or []
       if changes:
         assert isinstance(changes, list)
         changes = map(self.normalizeChangeSpec, changes)
 
       if not builder_names:
         raise ValueError('builderNames is not specified: %r' % (spec,))
 
       build = self.command.build
       build_is_from_buildbucket = bool(
           build.getProperties().getProperty(buildbucket.common.INFO_PROPERTY))
       trigger_via_buildbucket = bucket or build_is_from_buildbucket
 
       properties = self.getPropertiesForTriggeredBuild(build.getProperties(),
                                                        properties)
 
       if trigger_via_buildbucket:
         d = self.triggerBuildsViaBuildBucket(
             bucket, builder_names, properties, tags, changes)
       else:
         d = self.triggerBuildsLocally(builder_names, properties, changes)
       # addAsyncOpToCursor expects a deferred to return a build result. If a
       # buildset is added, then it is a success. This lambda function returns a
       # tuple, which is received by addAsyncOpToCursor.
       d.addCallback(lambda _: (builder.SUCCESS, None))
-      description = 'Triggering build(s) on %s' % (', '.join(builder_names),)
-      self.addAsyncOpToCursor(d, description)
+      self.addAsyncOpToCursor(
+          d,
+          'Triggering build(s) on %s' % (', '.join(builder_names),))
+      d.addErrback(handle_exception)
     except Exception as ex:
-      self.finishStep(self.cursor, builder.FAILURE, ex)

[Dirk Pranke, 2016/05/05 02:16:07]

I'm confused ... why isn't it sufficient to just
s/builder.FAILURE/builder.EXCEPTION/ here and make sure that trigger is a
'can_fail_build' step?

[nodir, 2016/05/05 16:45:32]

because triggering is an async operation, in particular the "it is not
configured" exception is raised asynchronously . Note handle_exception is passed
to addErrback
I believe can_fail_build concept was removed from recipe engine in favor of
python exceptions
https://github.com/luci/recipes-py/blob/0eba5ac3953b892d26a5a91decf84730261e1...
+martiniss to verify

[Dirk Pranke, 2016/05/05 16:56:10]

Ah, okay.
Yeah, I was vaguely referring to whatever the equivalent concept was ...

[nodir, 2016/05/05 16:58:20]

iannucci verified that exceptions are used in recipes to cause a step to fail
entire bulid.

I forgot to say something that may be non-obvious: can_fail_build is a recipe
concept. We never had it on the master side. Now can_fail_build does not exist,
so there is no way to check if step is critical at the time STEP_TRIGGER
annotation is produced.

+      handle_exception(ex)
 
   @staticmethod
   def getPropertiesForTriggeredBuild(current_properties, new_properties):
     props = {
         'parent_buildername': current_properties.getProperty('buildername'),
         'parent_buildnumber': current_properties.getProperty('buildnumber'),
     }
     props.update(new_properties)
     return props
 
@@ skipping 137 matching lines @@
     bsid, brids = yield master.addBuildset(
         ssid=ssid,
         reason='Triggered by %s' % build.builder.name,
         # Specify property source.
         properties={k: (v, 'ParentBuild') for k, v in properties.iteritems()},
         builderNames=builder_names)
     log.msg('Triggered a buildset %s with builders %s' % (bsid, builder_names))
     defer.returnValue((bsid, brids))
 
   def handleReturnCode(self, return_code):
-    # Treat all non-zero return codes as failure.
-    # We could have a special return code for warnings/exceptions, however,
-    # this might conflict with some existing use of a return code.
-    # Besides, applications can always intercept return codes and emit
-    # STEP_* tags.
-    succeeded = return_code == 0
-    if succeeded:
+    succeeded = False
+    if self.fatal_errors:
+      # Even if return_code==0 and self.honor_zero_return_code is True
+      # we can't do much when there are fatal errors
+      preamble = self.sections[0]
+      fatal_errors_str = '\n'.join(self.fatal_errors)
+      fatal_error_log = preamble['step'].addLog('fatal_errors')
+      fatal_error_log.addStdout(fatal_errors_str)
+      fatal_error_log.finish()
+      self.annotate_status = builder.EXCEPTION
+      if not self.cursorIsPreamble():
+        self.finishCursor(self.annotate_status,
+                          reason='Fatal errors: %s' % fatal_errors_str)
+    elif return_code == 0:
+      succeeded = True
       # Do not close the initial section because it will be closed by
       # self.finished when runCommand completes.
       if not self.cursorIsPreamble():
         self.closeCursor()
       if self.honor_zero_return_code:
         self.annotate_status = builder.SUCCESS
     else:
+      # Treat all non-zero return codes as failure.
+      # We could have a special return code for warnings/exceptions, however,
+      # this might conflict with some existing use of a return code.
+      # Besides, applications can always intercept return codes and emit
+      # STEP_* tags.
+
       # Only change annotate status if it'd otherwise indicate success.
       # This helps propagate purple (exception) step status to to-level
       # purple build status as opposed to red.
       if self.annotate_status in (builder.SUCCESS, builder.WARNINGS):
         self.annotate_status = builder.FAILURE
       if not self.cursorIsPreamble():
         self.finishCursor(self.annotate_status,
                           reason='return code was %d.' % return_code)
     self.cleanupSteps(exclude_async_pending=succeeded)
 
@@ skipping 110 matching lines @@
       and starts to wait for remaining steps to finish. Returns command_result
       as Deferred."""
       self.scriptComplete(command_result)
       steps_d = self.script_observer.waitForSteps()
       # Ignore the waitForSteps' result and return the original result,
       # so the caller of runCommand receives command_result.
       steps_d.addCallback(lambda *_: command_result)
       return steps_d
     d.addCallback(onCommandFinished)
     return d