Report to all reporters even if one fails

We short circuited reporting if any one reported failed. Instead do our
best to report to all reporters even if any one fails this way the
message has a better chance of getting out. This issue was identified
during the openstack zuul v3 transition as an item that needed to be
fixed.

Change-Id: Ia7f14fb108cd9ecdb14f6cada29abdf69acc1ed2
Story: 2001441

releasenotes/notes/reporters-always-report-27702c27369176da.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    Story 2001441 is fixed. Failure by one Zuul reporter will not short
+    circuit the reporting of other reporters. This ensures as much
+    information as possible is reported for each change even if some
+    failures occur. Note that the build set status is changed to 'ERROR'
+    after the first failed reporter.

tests/unit/test_scheduler.py

@@ -21,6 +21,7 @@ import textwrap
 import os
 import shutil
 import time
+from unittest import mock
 from unittest import skip
 
 import git
@@ -3392,6 +3393,50 @@ class TestScheduler(ZuulTestCase):
         self.assertEqual(A.messages[0],
                          self.smtp_messages[1]['body'])
 
+    @simple_layout('layouts/smtp.yaml')
+    @mock.patch('zuul.driver.gerrit.gerritreporter.GerritReporter.report')
+    def test_failed_reporter(self, report_mock):
+        '''Test that one failed reporter doesn't break other reporters'''
+        # Warning hacks. We sort the reports here so that the test is
+        # deterministic. Gerrit reporting will fail, but smtp reporting
+        # should succeed.
+        report_mock.side_effect = Exception('Gerrit failed to report')
+
+        tenant = self.sched.abide.tenants.get('tenant-one')
+        check = tenant.layout.pipelines['check']
+
+        check.success_actions = sorted(check.success_actions,
+                                       key=lambda x: x.name)
+        self.assertEqual(check.success_actions[0].name, 'gerrit')
+        self.assertEqual(check.success_actions[1].name, 'smtp')
+
+        A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
+        self.waitUntilSettled()
+        self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+
+        # We know that if gerrit ran first and failed and smtp ran second
+        # and sends mail then we handle failures in reporters gracefully.
+        self.assertEqual(len(self.smtp_messages), 2)
+
+        # A.messages only holds what FakeGerrit places in it. Thus we
+        # work on the knowledge of what the first message should be as
+        # it is only configured to go to SMTP.
+
+        self.assertEqual('zuul@example.com',
+                         self.smtp_messages[0]['from_email'])
+        self.assertEqual(['you@example.com'],
+                         self.smtp_messages[0]['to_email'])
+        self.assertEqual('Starting check jobs.',
+                         self.smtp_messages[0]['body'])
+
+        self.assertEqual('zuul_from@example.com',
+                         self.smtp_messages[1]['from_email'])
+        self.assertEqual(['alternative_me@example.com'],
+                         self.smtp_messages[1]['to_email'])
+        # This double checks that Gerrit side failed
+        self.assertEqual(A.messages, [])
+
     def test_timer_smtp(self):
         "Test that a periodic job is triggered"
         # This test can not use simple_layout because it must start

zuul/manager/__init__.py

@@ -151,15 +151,12 @@ class PipelineManager(object):
 
     def reportStart(self, item):
         if not self.pipeline._disabled:
-            try:
-                self.log.info("Reporting start, action %s item %s" %
-                              (self.pipeline.start_actions, item))
-                ret = self.sendReport(self.pipeline.start_actions, item)
-                if ret:
-                    self.log.error("Reporting item start %s received: %s" %
-                                   (item, ret))
-            except Exception:
-                self.log.exception("Exception while reporting start:")
+            self.log.info("Reporting start, action %s item %s" %
+                          (self.pipeline.start_actions, item))
+            ret = self.sendReport(self.pipeline.start_actions, item)
+            if ret:
+                self.log.error("Reporting item start %s received: %s" %
+                               (item, ret))
 
     def sendReport(self, action_reporters, item, message=None):
         """Sends the built message off to configured reporters.
@@ -170,11 +167,14 @@ class PipelineManager(object):
         report_errors = []
         if len(action_reporters) > 0:
             for reporter in action_reporters:
-                ret = reporter.report(item)
-                if ret:
-                    report_errors.append(ret)
-            if len(report_errors) == 0:
-                return
+                try:
+                    ret = reporter.report(item)
+                    if ret:
+                        report_errors.append(ret)
+                except Exception as e:
+                    item.setReportedResult('ERROR')
+                    self.log.exception("Exception while reporting")
+                    report_errors.append(str(e))
         return report_errors
 
     def isChangeReadyToBeEnqueued(self, change):
@@ -844,16 +844,12 @@ class PipelineManager(object):
             self.pipeline._consecutive_failures >= self.pipeline.disable_at):
             self.pipeline._disabled = True
         if actions:
-            try:
-                self.log.info("Reporting item %s, actions: %s" %
-                              (item, actions))
-                ret = self.sendReport(actions, item)
-                if ret:
-                    self.log.error("Reporting item %s received: %s" %
-                                   (item, ret))
-            except Exception:
-                self.log.exception("Exception while reporting:")
-                item.setReportedResult('ERROR')
+            self.log.info("Reporting item %s, actions: %s" %
+                          (item, actions))
+            ret = self.sendReport(actions, item)
+            if ret:
+                self.log.error("Reporting item %s received: %s" %
+                               (item, ret))
         return ret
 
     def reportStats(self, item):