Fix exception handling in Gerrit event connector

The original intent for connection event handling is that if there
is an error with the event, it should be discarded.  A recent
change to the Gerrit driver to use a threadpool executor to
pre-process events in parallel inadvertently altered the error
handling for that driver so that if certain exceptions were
encountered, the event would be retained in the incoming queue
indefinitely.

To return to the old behavior, the exception handling in the
gerrit event processor is updated so that all exceptions result
in the event handler completing sucessfully and the original
connection event is removed from the queue (regardless of whether
it produces any trigger events).

The new form looks similar to the corresponding code in the
github driver, which has used this structure for a longer time.

Change-Id: I0465a5c00e49300b13b75db626e0dc80a89cf4a1

tests/fakegerrit.py

@@ -863,6 +863,10 @@ class GerritWebServer(object):
                 path = self.path
                 self.log.debug("Got GET %s", path)
                 fake_gerrit.api_calls.append(('GET', path))
+                if fake_gerrit._fake_return_api_error:
+                    self.send_response(500)
+                    self.end_headers()
+                    return
 
                 m = self.change_re.match(path)
                 if m:
@@ -1178,6 +1182,7 @@ class FakeGerritConnection(gerritconnection.GerritConnection):
         self.changes = changes_db
         self.queries = []
         self.api_calls = []
+        self._fake_return_api_error = False
         self.upstream_root = upstream_root
         self.fake_checkers = []
         self._poller_event = poller_event

tests/unit/test_gerrit.py

@@ -24,11 +24,12 @@ import tests.base
 from tests.base import (
     AnsibleZuulTestCase,
     BaseTestCase,
-    simple_layout,
+    ZuulTestCase,
     gerrit_config,
     iterate_timeout,
+    okay_tracebacks,
+    simple_layout,
     skipIfMultiScheduler,
-    ZuulTestCase,
 )
 from zuul.lib import strings
 from zuul.driver.gerrit import GerritDriver
@@ -435,6 +436,18 @@ class TestGerritWeb(ZuulTestCase):
 
         self.assertEqual(A.data['status'], 'NEW')
 
+    @okay_tracebacks('_handleEvent')
+    def test_event_failure(self):
+        # Test that a failure to process a Gerrit event is handled
+        # correctly
+        A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
+        event = A.getPatchsetCreatedEvent(1)
+        self.fake_gerrit._fake_return_api_error = True
+        self.fake_gerrit.addEvent(event)
+        # If the event is not processed, then we will never settle and
+        # the test will time-out.
+        self.waitUntilSettled()
+
 
 class TestFileComments(AnsibleZuulTestCase):
     config_file = 'zuul-gerrit-web.conf'

zuul/driver/gerrit/gerritconnection.py

@@ -580,15 +580,17 @@ class GerritEventProcessor:
         if self.connector._stopped:
             return
 
-        attributes = {"rel": "GerritEvent"}
-        link = trace.Link(self.event_span.get_span_context(),
-                          attributes=attributes)
-        with self.tracer.start_as_current_span(
-                "GerritEventProcessing", links=[link]):
-            try:
+        try:
+            attributes = {"rel": "GerritEvent"}
+            link = trace.Link(self.event_span.get_span_context(),
+                              attributes=attributes)
+            with self.tracer.start_as_current_span(
+                    "GerritEventProcessing", links=[link]):
                 self.events = self._handleEvent(self.connection_event)
-            except GerritEventProcessingException as e:
-                self.log.warning("Skipping event due to %s", e)
+        except GerritEventProcessingException as e:
+            self.log.warning("Skipping event due to %s", e)
+        except Exception:
+            self.log.exception("Skipping event due to:")
         return self.events, self.connection_event
 
     def _handleEvent(self, connection_event):