From a2714c370a05d80f1d947c6344eda89a4902ec42 Mon Sep 17 00:00:00 2001
From: "James E. Blair" <jim@acmegating.com>
Date: Tue, 14 Dec 2021 10:52:36 -0800
Subject: [PATCH] Fix merging reconfiguration events

Reconfiguration events for the same tenant are supposed to be merged
so that if many branches are created, only one reconfiguration happens.

However, an error existed in the merge method which raised an exception
preventing the merge, and the exception handler around the merge method
was too broad and therefore suppressed the error.

This is well tested, but with tests that look more like "unit" tests
rather than functional tests.  They did not contain the branch_cache_ltime
dict which triggered the error.

This patch corrects the error and updates the test to be more realistic.

Change-Id: I0b5c7628a0580db55b56eec8c081ebee4d31d989
---
 tests/unit/test_scheduler.py | 49 ++++++++++++++++--------------------
 zuul/model.py                |  2 +-
 zuul/zk/event_queues.py      |  5 +++-
 3 files changed, 26 insertions(+), 30 deletions(-)

diff --git a/tests/unit/test_scheduler.py b/tests/unit/test_scheduler.py
index 0848f1118f..8e96836b70 100644
--- a/tests/unit/test_scheduler.py
+++ b/tests/unit/test_scheduler.py
@@ -3631,35 +3631,28 @@ class TestScheduler(ZuulTestCase):
 
     def test_reconfigure_merge(self):
         """Test that two reconfigure events are merged"""
+        # Wrap the recofiguration handler so we can count how many
+        # times it runs.
+        with mock.patch.object(
+                zuul.scheduler.Scheduler, '_doTenantReconfigureEvent',
+                wraps=self.scheds.first.sched._doTenantReconfigureEvent
+        ) as mymock:
+            with self.scheds.first.sched.run_handler_lock:
+                self.create_branch('org/project', 'stable/diablo')
+                self.fake_gerrit.addEvent(
+                    self.fake_gerrit.getFakeBranchCreatedEvent(
+                        'org/project', 'stable/diablo'))
+                self.create_branch('org/project', 'stable/essex')
+                self.fake_gerrit.addEvent(
+                    self.fake_gerrit.getFakeBranchCreatedEvent(
+                        'org/project', 'stable/essex'))
+                for _ in iterate_timeout(60, 'jobs started'):
+                    if len(self.scheds.first.sched.trigger_events[
+                            'tenant-one']) == 2:
+                        break
 
-        tenant = self.scheds.first.sched.abide.tenants['tenant-one']
-        (trusted, project) = tenant.getProject('org/project')
-
-        management_queue = self.scheds.first.sched.management_events[
-            'tenant-one']
-
-        with self.scheds.first.sched.run_handler_lock:
-            mgmt_queue_size = len(management_queue)
-            self.assertEqual(mgmt_queue_size, 0)
-
-            self.scheds.first.sched.reconfigureTenant(tenant, project, None)
-            mgmt_queue_size = len(management_queue)
-            self.assertEqual(mgmt_queue_size, 1)
-
-            self.scheds.first.sched.reconfigureTenant(tenant, project, None)
-            mgmt_queue_size = len(management_queue)
-            self.assertEqual(mgmt_queue_size, 2)
-
-            # The second event should be combined with the first so we should
-            # only see the merged entry when consuming from the queue.
-            mgmt_events = list(management_queue)
-            self.assertEqual(len(mgmt_events), 1)
-            self.assertEqual(len(mgmt_events[0].merged_events), 1)
-
-        self.waitUntilSettled()
-
-        mgmt_queue_size = len(management_queue)
-        self.assertEqual(mgmt_queue_size, 0)
+            self.waitUntilSettled()
+            mymock.assert_called_once()
 
     def test_live_reconfiguration(self):
         "Test that live reconfiguration works"
diff --git a/zuul/model.py b/zuul/model.py
index d65a9f3e72..6b5061f6a4 100644
--- a/zuul/model.py
+++ b/zuul/model.py
@@ -5548,7 +5548,7 @@ class TenantReconfigureEvent(ManagementEvent):
         if self.tenant_name != other.tenant_name:
             raise Exception("Can not merge events from different tenants")
         self.project_branches |= other.project_branches
-        for connection_name, ltime in other.branch_cache_ltimes:
+        for connection_name, ltime in other.branch_cache_ltimes.items():
             self.branch_cache_ltimes[connection_name] = max(
                 self.branch_cache_ltimes.get(connection_name, ltime), ltime)
         self.zuul_event_ltime = max(self.zuul_event_ltime,
diff --git a/zuul/zk/event_queues.py b/zuul/zk/event_queues.py
index 3f8453e732..23e9be31f3 100644
--- a/zuul/zk/event_queues.py
+++ b/zuul/zk/event_queues.py
@@ -560,8 +560,11 @@ class ManagementEventQueue(ZooKeeperEventQueue):
             if event.zuul_event_ltime is None:
                 event.zuul_event_ltime = zstat.creation_transaction_id
 
-            with suppress(ValueError):
+            try:
                 other_event = event_list[event_list.index(event)]
+            except ValueError:
+                other_event = None
+            if other_event:
                 if isinstance(other_event, model.TenantReconfigureEvent):
                     other_event.merge(event)
                     continue