zuul/zuul
Code Issues Proposed changes

Don't reconfigure after every gitlab merge

Browse Source 
The gitlab driver sets change.files to None on branch push events.
Like every other system, it also emits branch push events after
merging merge requests.  When the scheduler encounters a branch
updated event with files.None, it assumes the worst and performs
a tenant reconfiguration.  This means that Zuul performs a tenant
reconfiguration after every single MR is merged.

Gitlab supplies a list of changed files on push events.  This change
modifies the gitlab driver to do that.  This mirrors what is done
in the github driver.

Change-Id: Ia6651048eea7f183ee0a65b6e8de83fdf892c9a7
This commit is contained in:
James E. Blair 2022-02-17 14:45:25 -08:00
parent d5385d7fc5
commit 336bb21386
4 changed files with 95 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

30
tests/base.py
View File

@ -1978,7 +1978,15 @@ class FakeGitlabConnection(gitlabconnection.GitlabConnection):
def getPushEvent(
self, project, before=None, after=None,
branch='refs/heads/master'):
branch='refs/heads/master',
added_files=None, removed_files=None,
modified_files=None):
if added_files is None:
added_files = []
if removed_files is None:
removed_files = []
if modified_files is None:
modified_files = []
name = 'gl_push'
if not after:
repo_path = os.path.join(self.upstream_root, project)
@ -1992,6 +2000,14 @@ class FakeGitlabConnection(gitlabconnection.GitlabConnection):
'project': {
'path_with_namespace': project
},
'commits': [
{
'added': added_files,
'removed': removed_files,
'modified': modified_files
}
],
'total_commits_count': 1,
}
return (name, data)
@ -2161,6 +2177,18 @@ class FakeGitlabMergeRequest(object):
self.mergeMergeRequest()
return self.getMergeRequestEvent(action='merge')
def getMergeRequestMergedPushEvent(self, added_files=None,
removed_files=None,
modified_files=None):
return self.gitlab.getPushEvent(
project=self.project,
branch='refs/heads/%s' % self.branch,
before=random_sha1(),
after=self.sha,
added_files=added_files,
removed_files=removed_files,
modified_files=modified_files)
def getMergeRequestApprovedEvent(self):
self.approved = True
return self.getMergeRequestEvent(action='approved')

41
tests/unit/test_gitlab_driver.py
View File

@ -1,4 +1,5 @@
# Copyright 2019 Red Hat
# Copyright 2022 Acme Gating, LLC
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may
# not use this file except in compliance with the License. You may obtain
@ -220,6 +221,43 @@ class TestGitlabDriver(ZuulTestCase):
self.assertEqual(1, len(self.history))
self.assertHistory([{'name': 'project-promote'}])
@simple_layout('layouts/basic-gitlab.yaml', driver='gitlab')
def test_merge_push_does_not_reconfigure(self):
# Test that the push event that follows a merge doesn't
# needlessly trigger reconfiguration.
A = self.fake_gitlab.openFakeMergeRequest('org/project', 'master', 'A')
state1 = self.scheds.first.sched.local_layout_state.get("tenant-one")
self.fake_gitlab.emitEvent(A.getMergeRequestMergedEvent())
self.fake_gitlab.emitEvent(A.getMergeRequestMergedPushEvent())
self.waitUntilSettled()
self.assertEqual(2, len(self.history))
self.assertHistory([{'name': 'project-post-job'},
{'name': 'project-promote'}
], ordered=False)
state2 = self.scheds.first.sched.local_layout_state.get("tenant-one")
self.assertEqual(state1, state2)
@simple_layout('layouts/basic-gitlab.yaml', driver='gitlab')
def test_merge_push_does_reconfigure(self):
# Test that the push event that follows a merge does
# trigger reconfiguration if .zuul.yaml is changed.
A = self.fake_gitlab.openFakeMergeRequest('org/project', 'master', 'A')
state1 = self.scheds.first.sched.local_layout_state.get("tenant-one")
self.fake_gitlab.emitEvent(A.getMergeRequestMergedEvent())
self.fake_gitlab.emitEvent(A.getMergeRequestMergedPushEvent(
modified_files=['.zuul.yaml']))
self.waitUntilSettled()
self.assertEqual(2, len(self.history))
self.assertHistory([{'name': 'project-post-job'},
{'name': 'project-promote'}
], ordered=False)
state2 = self.scheds.first.sched.local_layout_state.get("tenant-one")
self.assertNotEqual(state1, state2)
@simple_layout('layouts/basic-gitlab.yaml', driver='gitlab')
def test_merge_request_updated_builds_aborted(self):
@ -427,7 +465,8 @@ class TestGitlabDriver(ZuulTestCase):
'job.yaml': playbook},
message='Add InRepo configuration'
)
event = self.fake_gitlab.getPushEvent('org/project')
event = self.fake_gitlab.getPushEvent('org/project',
modified_files=['.zuul.yaml'])
self.fake_gitlab.emitEvent(event)
self.waitUntilSettled()

21
zuul/driver/gitlab/gitlabconnection.py
View File

@ -1,4 +1,5 @@
# Copyright 2019 Red Hat, Inc.
# Copyright 2022 Acme Gating, LLC
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may
# not use this file except in compliance with the License. You may obtain
@ -190,6 +191,7 @@ class GitlabEventConnector(threading.Thread):
event.newrev = body['after']
event.oldrev = body['before']
event.type = 'gl_push'
event.commits = body.get('commits')
self.connection.clearConnectionCacheOnBranchEvent(event)
@ -645,9 +647,7 @@ class GitlabConnection(ZKChangeCacheMixin, ZKBranchCacheMixin, BaseConnection):
change.oldrev = change_key.oldrev
change.newrev = change_key.newrev
change.url = self.getGitwebUrl(project, sha=change.newrev)
# Explicitly set files to None and let the pipelines processor
# call the merger asynchronuously
change.files = None
change.files = self.getPushedFileNames(event)
try:
self._change_cache.set(change_key, change)
except ConcurrentUpdateError:
@ -679,6 +679,21 @@ class GitlabConnection(ZKChangeCacheMixin, ZKBranchCacheMixin, BaseConnection):
log.info("Updated change from Gitlab %s" % change)
return change
def getPushedFileNames(self, event):
if event is None:
return None
if event.total_commits_count > 20:
# Gitlab only includes files for the most recent 20
# commits. If we have more than that, set files to None
# so that mergers will perform lookups if necessary, and
# the scheduler will assume a reconfiguration is required.
return None
files = set()
for c in event.commits:
for f in c.get('added') + c.get('modified') + c.get('removed'):
files.add(f)
return list(files)
def canMerge(self, change, allow_needs, event=None):
log = get_annotated_logger(self.log, event)
can_merge = True if change.merge_status == "can_be_merged" else False

8
zuul/driver/gitlab/gitlabmodel.py
View File

@ -1,4 +1,5 @@
# Copyright 2019 Red Hat, Inc.
# Copyright 2022 Acme Gating, LLC
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may
# not use this file except in compliance with the License. You may obtain
@ -90,6 +91,8 @@ class GitlabTriggerEvent(TriggerEvent):
self.change_number = None
self.merge_request_description_changed = None
self.tag = None
self.commits = []
self.total_commits_count = 0
def toDict(self):
d = super().toDict()
@ -102,6 +105,8 @@ class GitlabTriggerEvent(TriggerEvent):
d["merge_request_description_changed"] = \
self.merge_request_description_changed
d["tag"] = self.tag
d["commits"] = self.commits
d["total_commits_count"] = self.total_commits_count
return d
def updateFromDict(self, d):
@ -115,6 +120,9 @@ class GitlabTriggerEvent(TriggerEvent):
self.merge_request_description_changed = \
d["merge_request_description_changed"]
self.tag = d["tag"]
self.commits = d.get("commits", [])
self.total_commits_count = d.get("total_commits_count",
len(self.commits))
def _repr(self):
r = [super(GitlabTriggerEvent, self)._repr()]