From 8d144dc4f89c9c6c7e938f35119ed78ae1e0aeec Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Fri, 5 May 2017 10:13:45 -0700 Subject: [PATCH] Wait for merger to complete before item is ready In zuul v2, prepareRef waited until the merger had completed its work before telling the pipeline manager that the item was ready. This was important as it ensured there was a zuul ref on a merger for jobs to be able to fetch from. We dropped that in v3 because the jobs would build their own repo states on the executor, however, we're about to put something similar back in that we're going have the merger return the complete repo state so that the executors can later use that to reconstruct the work the merger performed. Because prepareItem (essentially the v3 version of prepareRef) did not wait for the merger result before declaring the item ready, we can end up launching jobs before the merger completes. This isn't a big deal, yet, but it could still produce surprising results and make for extra work. This change restores the old behavior, as well as makes a test less suceptible to race conditions (again, which are more likely to be seen in future changes). Similarly, we also lost setting ready to false in the case of a merge conflict (a behavior which was present in v2). This corrects that as well to ensure that we don't execute jobs if there is a merge conflict. Because some tests were written with the assumption that only one merger is running, and we are now running two (a dedicated merger and one in the executor), they are more likely to fail due to race conditions, especially now that we are waiting for mergers to complete before executing jobs. This change also removes the dedicated merger from the test infrastructure so we rely only on the merger in the executor. Change-Id: I6d4da3d901d99290ef99b478ea9a4659058fe839 --- tests/base.py | 4 ---- tests/unit/test_scheduler.py | 5 +++-- zuul/manager/__init__.py | 4 ++++ 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/tests/base.py b/tests/base.py index 0105ffaaad..d62d9caa04 100755 --- a/tests/base.py +++ b/tests/base.py @@ -1777,8 +1777,6 @@ class ZuulTestCase(BaseTestCase): old_urlopen = urllib.request.urlopen urllib.request.urlopen = URLOpenerFactory - self._startMerger() - self.executor_server = RecordingExecutorServer( self.config, self.connections, jobdir_root=self.test_root, @@ -2047,8 +2045,6 @@ class ZuulTestCase(BaseTestCase): def shutdown(self): self.log.debug("Shutting down after tests") self.executor_client.stop() - self.merge_server.stop() - self.merge_server.join() self.merge_client.stop() self.executor_server.stop() self.sched.stop() diff --git a/tests/unit/test_scheduler.py b/tests/unit/test_scheduler.py index f394c0cf2b..d4163693f9 100755 --- a/tests/unit/test_scheduler.py +++ b/tests/unit/test_scheduler.py @@ -1012,6 +1012,7 @@ class TestScheduler(ZuulTestCase): self.fake_gerrit.addEvent(A.addApproval('approved', 1)) self.waitUntilSettled() self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1)) self.waitUntilSettled() @@ -1509,8 +1510,8 @@ class TestScheduler(ZuulTestCase): tenant = self.sched.abide.tenants.get('tenant-one') trusted, project = tenant.getProject('org/project') url = self.fake_gerrit.getGitUrl(project) - self.merge_server.merger._addProject('review.example.com', - 'org/project', url) + self.executor_server.merger._addProject('review.example.com', + 'org/project', url) A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A') A.addPatchset(large=True) # TODOv3(jeblair): add hostname to upstream root diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py index 5b32e5bc1b..d13a1b436c 100644 --- a/zuul/manager/__init__.py +++ b/zuul/manager/__init__.py @@ -522,6 +522,10 @@ class PipelineManager(object): build_set.setConfiguration() if build_set.merge_state == build_set.NEW: return self.scheduleMerge(item, ['zuul.yaml', '.zuul.yaml']) + if build_set.merge_state == build_set.PENDING: + return False + if build_set.unable_to_merge: + return False if build_set.config_error: return False return True