Browse Source

Merge "Delay Github fileschanges workaround to pipeline processing"

tags/3.4.0
Zuul 5 months ago
parent
commit
5ae6d549a5

+ 0
- 1
tests/base.py View File

@@ -1001,7 +1001,6 @@ class FakeGithubPullRequest(object):
1001 1001
         msg = self.subject + '-' + str(self.number_of_commits)
1002 1002
         for fn, content in self.files.items():
1003 1003
             fn = os.path.join(repo.working_dir, fn)
1004
-            f = open(fn, 'w')
1005 1004
             with open(fn, 'w') as f:
1006 1005
                 f.write(content)
1007 1006
             repo.index.add([fn])

+ 36
- 0
tests/unit/test_github_driver.py View File

@@ -119,6 +119,42 @@ class TestGithubDriver(ZuulTestCase):
119 119
         self.waitUntilSettled()
120 120
         self.assertEqual(1, len(self.history))
121 121
 
122
+    @simple_layout('layouts/files-github.yaml', driver='github')
123
+    def test_pull_changed_files_length_mismatch_reenqueue(self):
124
+        # Hold jobs so we can trigger a reconfiguration while the item is in
125
+        # the pipeline
126
+        self.executor_server.hold_jobs_in_build = True
127
+
128
+        files = {'{:03d}.txt'.format(n): 'test' for n in range(300)}
129
+        # File 301 which is not included in the list of files of the PR,
130
+        # since Github only returns max. 300 files in alphabetical order
131
+        files["foobar-requires"] = "test"
132
+        A = self.fake_github.openFakePullRequest(
133
+            'org/project', 'master', 'A', files=files)
134
+
135
+        self.fake_github.emitEvent(A.getPullRequestOpenedEvent())
136
+        self.waitUntilSettled()
137
+
138
+        # Comment on the pull request to trigger updateChange
139
+        self.fake_github.emitEvent(A.getCommentAddedEvent('casual comment'))
140
+        self.waitUntilSettled()
141
+
142
+        # Trigger reconfig to enforce a reenqueue of the item
143
+        self.sched.reconfigure(self.config)
144
+        self.waitUntilSettled()
145
+
146
+        # Now we can release all jobs
147
+        self.executor_server.hold_jobs_in_build = True
148
+        self.executor_server.release()
149
+        self.waitUntilSettled()
150
+
151
+        # There must be exactly one successful job in the history. If there is
152
+        # an aborted job in the history the reenqueue failed.
153
+        self.assertHistory([
154
+            dict(name='project-test1', result='SUCCESS',
155
+                 changes="%s,%s" % (A.number, A.head_sha)),
156
+        ])
157
+
122 158
     @simple_layout('layouts/basic-github.yaml', driver='github')
123 159
     def test_pull_github_files_error(self):
124 160
         A = self.fake_github.openFakePullRequest(

+ 11
- 17
zuul/driver/github/githubconnection.py View File

@@ -904,24 +904,17 @@ class GithubConnection(BaseConnection):
904 904
 
905 905
         return changes
906 906
 
907
-    def getFilesChanges(self, project_name, head, base):
908
-        job = self.sched.merger.getFilesChanges(self.connection_name,
909
-                                                project_name,
910
-                                                head, base)
911
-        self.log.debug("Waiting for fileschanges job %s", job)
912
-        job.wait()
913
-        if not job.updated:
914
-            raise Exception("Fileschanges job {} failed".format(job))
915
-        self.log.debug("Fileschanges job %s got changes on files %s",
916
-                       job, job.files)
917
-        return job.files
918
-
919 907
     def _updateChange(self, change):
920 908
         self.log.info("Updating %s" % (change,))
921 909
         change.pr = self.getPull(change.project.name, change.number)
922 910
         change.ref = "refs/pull/%s/head" % change.number
923 911
         change.branch = change.pr.get('base').get('ref')
924
-        change.files = change.pr.get('files')
912
+
913
+        # Don't overwrite the files list. The change object is bound to a
914
+        # specific revision and thus the changed files won't change. This is
915
+        # important if we got the files later because of the 300 files limit.
916
+        if not change.files:
917
+            change.files = change.pr.get('files')
925 918
         # Github's pull requests files API only returns at max
926 919
         # the first 300 changed files of a PR in alphabetical order.
927 920
         # https://developer.github.com/v3/pulls/#list-pull-requests-files
@@ -929,10 +922,11 @@ class GithubConnection(BaseConnection):
929 922
             self.log.warning("Got only %s files but PR has %s files.",
930 923
                              len(change.files),
931 924
                              change.pr.get('changed_files', 0))
932
-            change.files = self.getFilesChanges(
933
-                change.project.name,
934
-                change.ref,
935
-                change.branch)
925
+            # In this case explicitly set change.files to None to signalize
926
+            # that we need to ask the mergers later in pipeline processing.
927
+            # We cannot query the files here using the mergers because this
928
+            # can slow down the github event queue considerably.
929
+            change.files = None
936 930
         change.title = change.pr.get('title')
937 931
         change.open = change.pr.get('state') == 'open'
938 932
         change.is_merged = change.pr.get('merged')

+ 28
- 9
zuul/manager/__init__.py View File

@@ -578,8 +578,6 @@ class PipelineManager(object):
578 578
             return self._loadDynamicLayout(item)
579 579
 
580 580
     def scheduleMerge(self, item, files=None, dirs=None):
581
-        build_set = item.current_build_set
582
-
583 581
         self.log.debug("Scheduling merge for item %s (files: %s, dirs: %s)" %
584 582
                        (item, files, dirs))
585 583
         build_set = item.current_build_set
@@ -594,23 +592,38 @@ class PipelineManager(object):
594 592
                                            precedence=self.pipeline.precedence)
595 593
         return False
596 594
 
595
+    def scheduleFilesChanges(self, item):
596
+        self.log.debug("Scheduling fileschanged for item %s", item)
597
+        build_set = item.current_build_set
598
+        build_set.files_state = build_set.PENDING
599
+
600
+        self.sched.merger.getFilesChanges(
601
+            item.change.project.connection_name, item.change.project.name,
602
+            item.change.ref, item.change.branch, build_set=build_set)
603
+        return False
604
+
597 605
     def prepareItem(self, item):
598 606
         # This runs on every iteration of _processOneItem
599 607
         # Returns True if the item is ready, false otherwise
608
+        ready = True
600 609
         build_set = item.current_build_set
601 610
         if not build_set.ref:
602 611
             build_set.setConfiguration()
603 612
         if build_set.merge_state == build_set.NEW:
604
-            return self.scheduleMerge(item,
605
-                                      files=['zuul.yaml', '.zuul.yaml'],
606
-                                      dirs=['zuul.d', '.zuul.d'])
613
+            ready = self.scheduleMerge(item,
614
+                                       files=['zuul.yaml', '.zuul.yaml'],
615
+                                       dirs=['zuul.d', '.zuul.d'])
616
+        if build_set.files_state == build_set.NEW:
617
+            ready = self.scheduleFilesChanges(item)
618
+        if build_set.files_state == build_set.PENDING:
619
+            ready = False
607 620
         if build_set.merge_state == build_set.PENDING:
608
-            return False
621
+            ready = False
609 622
         if build_set.unable_to_merge:
610
-            return False
623
+            ready = False
611 624
         if build_set.config_errors:
612
-            return False
613
-        return True
625
+            ready = False
626
+        return ready
614 627
 
615 628
     def prepareJobs(self, item):
616 629
         # This only runs once the item is in the pipeline's action window
@@ -820,6 +833,12 @@ class PipelineManager(object):
820 833
         self._resumeBuilds(build.build_set)
821 834
         return True
822 835
 
836
+    def onFilesChangesCompleted(self, event):
837
+        build_set = event.build_set
838
+        item = build_set.item
839
+        item.change.files = event.files
840
+        build_set.files_state = build_set.COMPLETE
841
+
823 842
     def onMergeCompleted(self, event):
824 843
         build_set = event.build_set
825 844
         item = build_set.item

+ 11
- 5
zuul/merger/client.py View File

@@ -132,12 +132,14 @@ class MergeClient(object):
132 132
         return job
133 133
 
134 134
     def getFilesChanges(self, connection_name, project_name, branch,
135
-                        tosha=None, precedence=zuul.model.PRECEDENCE_HIGH):
135
+                        tosha=None, precedence=zuul.model.PRECEDENCE_HIGH,
136
+                        build_set=None):
136 137
         data = dict(connection=connection_name,
137 138
                     project=project_name,
138 139
                     branch=branch,
139 140
                     tosha=tosha)
140
-        job = self.submitJob('merger:fileschanges', data, None, precedence)
141
+        job = self.submitJob('merger:fileschanges', data, build_set,
142
+                             precedence)
141 143
         return job
142 144
 
143 145
     def onBuildCompleted(self, job):
@@ -153,9 +155,13 @@ class MergeClient(object):
153 155
                       (job, merged, job.updated, commit))
154 156
         job.setComplete()
155 157
         if job.build_set:
156
-            self.sched.onMergeCompleted(job.build_set,
157
-                                        merged, job.updated, commit, files,
158
-                                        repo_state)
158
+            if job.name == 'merger:fileschanges':
159
+                self.sched.onFilesChangesCompleted(job.build_set, files)
160
+            else:
161
+                self.sched.onMergeCompleted(job.build_set,
162
+                                            merged, job.updated, commit, files,
163
+                                            repo_state)
164
+
159 165
         # The test suite expects the job to be removed from the
160 166
         # internal account after the wake flag is set.
161 167
         self.jobs.remove(job)

+ 9
- 0
zuul/model.py View File

@@ -1784,6 +1784,10 @@ class BuildSet(object):
1784 1784
         self.files = RepoFiles()
1785 1785
         self.repo_state = {}
1786 1786
         self.tries = {}
1787
+        if item.change.files is not None:
1788
+            self.files_state = self.COMPLETE
1789
+        else:
1790
+            self.files_state = self.NEW
1787 1791
 
1788 1792
     @property
1789 1793
     def ref(self):
@@ -2584,6 +2588,11 @@ class Ref(object):
2584 2588
         return set()
2585 2589
 
2586 2590
     def updatesConfig(self):
2591
+        if self.files is None:
2592
+            # If self.files is None we don't know if this change updates the
2593
+            # config so assume it does as this is a safe default if we don't
2594
+            # know.
2595
+            return True
2587 2596
         if 'zuul.yaml' in self.files or '.zuul.yaml' in self.files or \
2588 2597
            [True for fn in self.files if fn.startswith("zuul.d/") or
2589 2598
             fn.startswith(".zuul.d/")]:

+ 31
- 0
zuul/scheduler.py View File

@@ -215,6 +215,18 @@ class MergeCompletedEvent(ResultEvent):
215 215
         self.repo_state = repo_state
216 216
 
217 217
 
218
+class FilesChangesCompletedEvent(ResultEvent):
219
+    """A remote fileschanges operation has completed
220
+
221
+    :arg BuildSet build_set: The build_set which is ready.
222
+    :arg list files: List of files changed.
223
+    """
224
+
225
+    def __init__(self, build_set, files):
226
+        self.build_set = build_set
227
+        self.files = files
228
+
229
+
218 230
 class NodesProvisionedEvent(ResultEvent):
219 231
     """Nodes have been provisioned for a build_set
220 232
 
@@ -475,6 +487,11 @@ class Scheduler(threading.Thread):
475 487
         self.result_event_queue.put(event)
476 488
         self.wake_event.set()
477 489
 
490
+    def onFilesChangesCompleted(self, build_set, files):
491
+        event = FilesChangesCompletedEvent(build_set, files)
492
+        self.result_event_queue.put(event)
493
+        self.wake_event.set()
494
+
478 495
     def onNodesProvisioned(self, req):
479 496
         event = NodesProvisionedEvent(req)
480 497
         self.result_event_queue.put(event)
@@ -1107,6 +1124,8 @@ class Scheduler(threading.Thread):
1107 1124
                 self._doBuildCompletedEvent(event)
1108 1125
             elif isinstance(event, MergeCompletedEvent):
1109 1126
                 self._doMergeCompletedEvent(event)
1127
+            elif isinstance(event, FilesChangesCompletedEvent):
1128
+                self._doFilesChangesCompletedEvent(event)
1110 1129
             elif isinstance(event, NodesProvisionedEvent):
1111 1130
                 self._doNodesProvisionedEvent(event)
1112 1131
             else:
@@ -1264,6 +1283,18 @@ class Scheduler(threading.Thread):
1264 1283
             return
1265 1284
         pipeline.manager.onMergeCompleted(event)
1266 1285
 
1286
+    def _doFilesChangesCompletedEvent(self, event):
1287
+        build_set = event.build_set
1288
+        if build_set is not build_set.item.current_build_set:
1289
+            self.log.warning("Build set %s is not current", build_set)
1290
+            return
1291
+        pipeline = build_set.item.pipeline
1292
+        if not pipeline:
1293
+            self.log.warning("Build set %s is not associated with a pipeline",
1294
+                             build_set)
1295
+            return
1296
+        pipeline.manager.onFilesChangesCompleted(event)
1297
+
1267 1298
     def _doNodesProvisionedEvent(self, event):
1268 1299
         request = event.request
1269 1300
         request_id = event.request_id

Loading…
Cancel
Save