Save superfluous api requests in check run reporting
When reporting the check run result zuul is currently doing four github api requests: 1. Get repository 2. Get head commit of pr 3. Get check runs of head commit 4. (optional) Create initial version of check run in case start reporting is disabled 5. Update check run that has been created in start reporting The first three requests are basically unneeded if we craft a direct PATCH request and remember the check run id that has been created in the start reporting. This is needed since those run in the critical path of the event processing and can cause large event processing delays in a very busy zuul system. Change-Id: I55df1cc28279bb6923e51686dde8809421486c6a
This commit is contained in:
Tobias Henkel
parent
418bf6b462
commit
ba6d86ada2
|
|
@ -650,6 +650,9 @@ class FakeGithubSession(object):
|
|||
# Handle check run creation
|
||||
match = re.match(r'.*/repos/(.*)/check-runs$', url)
|
||||
if match:
|
||||
if self.client._data.fail_check_run_creation:
|
||||
return FakeResponse('Internal server error', 500)
|
||||
|
||||
org, reponame = match.groups()[0].split('/', 1)
|
||||
repo = self.client._data.repos.get((org, reponame))
|
||||
|
||||
|
|
@ -709,6 +712,32 @@ class FakeGithubSession(object):
|
|||
|
||||
return FakeResponse(None, 404)
|
||||
|
||||
def patch(self, url, data=None, headers=None, params=None, json=None):
|
||||
|
||||
# Handle check run update
|
||||
match = re.match(r'.*/repos/(.*)/check-runs/(.*)$', url)
|
||||
if match:
|
||||
org, reponame = match.groups()[0].split('/', 1)
|
||||
check_run_id = match.groups()[1]
|
||||
repo = self.client._data.repos.get((org, reponame))
|
||||
|
||||
# Find the specified check run
|
||||
check_runs = [
|
||||
check_run
|
||||
for commit in repo._commits.values()
|
||||
for check_run in commit._check_runs
|
||||
if check_run.id == check_run_id
|
||||
]
|
||||
check_run = check_runs[0]
|
||||
|
||||
check_run.update(json['conclusion'],
|
||||
json['completed_at'],
|
||||
json['output'],
|
||||
json['details_url'],
|
||||
json['external_id'],
|
||||
json['actions'])
|
||||
return FakeResponse(check_run.as_dict(), 200)
|
||||
|
||||
def get_repo(self, request, params=None):
|
||||
org, project, request = request.split('/', 2)
|
||||
project_name = '{}/{}'.format(org, project)
|
||||
|
|
@ -736,6 +765,7 @@ class FakeGithubData(object):
|
|||
self.pull_requests = pull_requests
|
||||
self.repos = {}
|
||||
self.reports = []
|
||||
self.fail_check_run_creation = False
|
||||
|
||||
|
||||
class FakeGithubClient(object):
|
||||
|
|
|
|||
|
|
@ -1912,19 +1912,22 @@ class TestGithubAppDriver(ZuulGithubAppTestCase):
|
|||
project = "org/project3"
|
||||
github = self.fake_github.getGithubClient(None)
|
||||
|
||||
# Make check run creation fail
|
||||
github._data.fail_check_run_creation = True
|
||||
|
||||
# pipeline reports pull request status both on start and success
|
||||
self.executor_server.hold_jobs_in_build = True
|
||||
A = self.fake_github.openFakePullRequest(project, "master", "A")
|
||||
self.fake_github.emitEvent(A.getPullRequestOpenedEvent())
|
||||
self.waitUntilSettled()
|
||||
|
||||
# We should have a pending check for the head sha
|
||||
# We should have no pending check for the head sha
|
||||
commit = github.repo_from_project(project)._commits.get(A.head_sha)
|
||||
check_runs = commit.check_runs()
|
||||
self.assertEqual(1, len(check_runs))
|
||||
self.assertEqual(0, len(check_runs))
|
||||
|
||||
# Delete this check_run to simulate a failed check_run creation
|
||||
commit._check_runs = []
|
||||
# Make check run creation work again
|
||||
github._data.fail_check_run_creation = False
|
||||
|
||||
# Now run the build and check if the update of the check_run could
|
||||
# still be accomplished.
|
||||
|
|
@ -1974,7 +1977,7 @@ class TestGithubAppDriver(ZuulGithubAppTestCase):
|
|||
self.assertEqual(0, len(check_runs))
|
||||
|
||||
expected_warning = (
|
||||
"Failed to update check run tenant-one/checks-api-reporting: "
|
||||
"Failed to create check run tenant-one/checks-api-reporting: "
|
||||
"403 Resource not accessible by integration"
|
||||
)
|
||||
self.assertEqual(2, len(A.comments))
|
||||
|
|
|
|||
|
|
@ -1988,13 +1988,27 @@ class GithubConnection(BaseConnection):
|
|||
pull_request.remove_label(label)
|
||||
log.debug("Removed label %s from %s#%s", label, proj, pr_number)
|
||||
|
||||
def _create_or_update_check(self, github, project_name, check_run_id,
|
||||
**kwargs):
|
||||
if check_run_id:
|
||||
# Update an existing check run
|
||||
url = github.session.build_url(
|
||||
'repos', project_name, 'check-runs', str(check_run_id))
|
||||
resp = github.session.patch(url, json=kwargs)
|
||||
else:
|
||||
# Create a new check run
|
||||
url = github.session.build_url(
|
||||
'repos', project_name, 'check-runs')
|
||||
resp = github.session.post(url, json=kwargs)
|
||||
resp.raise_for_status()
|
||||
return resp.json().get('id')
|
||||
|
||||
def updateCheck(self, project, pr_number, sha, status, completed, context,
|
||||
details_url, message, file_comments, external_id,
|
||||
zuul_event_id=None):
|
||||
zuul_event_id=None, check_run_id=None):
|
||||
log = get_annotated_logger(self.log, zuul_event_id)
|
||||
github = self.getGithubClient(project, zuul_event_id=zuul_event_id)
|
||||
self._append_accept_header(github, PREVIEW_CHECKS_ACCEPT)
|
||||
owner, proj = project.split("/")
|
||||
|
||||
# Always provide an empty list of actions by default
|
||||
actions = []
|
||||
|
|
@ -2018,7 +2032,7 @@ class GithubConnection(BaseConnection):
|
|||
context
|
||||
)
|
||||
)
|
||||
return errors
|
||||
return None, errors
|
||||
|
||||
output = {"title": "Summary", "summary": message}
|
||||
|
||||
|
|
@ -2041,83 +2055,34 @@ class GithubConnection(BaseConnection):
|
|||
# conclusion, as the status will always be "completed".
|
||||
conclusion = status
|
||||
|
||||
# Unless something went wrong during the start reporting of this
|
||||
# change (e.g. the check_run creation failed), there should already
|
||||
# be a check_run available. If not we will create one.
|
||||
check_runs = []
|
||||
|
||||
repository = github.repository(owner, proj)
|
||||
try:
|
||||
check_runs = [
|
||||
c for c in repository.commit(sha).check_runs()
|
||||
if c.name == context
|
||||
]
|
||||
except github3.exceptions.GitHubException as exc:
|
||||
log.error(
|
||||
"Could not retrieve existing check runs for %s#%s on "
|
||||
"sha %s: %s",
|
||||
project, pr_number, sha, str(exc),
|
||||
if not check_run_id:
|
||||
log.debug("Could not find check run %s for %s#%s on sha %s. "
|
||||
"Creating a new one", context, project, pr_number,
|
||||
sha)
|
||||
action = 'create'
|
||||
arguments = dict(
|
||||
name=context,
|
||||
head_sha=sha,
|
||||
conclusion=conclusion,
|
||||
completed_at=completed_at,
|
||||
output=output,
|
||||
details_url=details_url,
|
||||
external_id=external_id,
|
||||
actions=actions,
|
||||
)
|
||||
|
||||
if not check_runs:
|
||||
log.debug(
|
||||
"Could not find check run %s for %s#%s on sha %s. "
|
||||
"Creating a new one",
|
||||
context, project, pr_number, sha,
|
||||
)
|
||||
|
||||
try:
|
||||
check_run = repository.create_check_run(
|
||||
name=context,
|
||||
head_sha=sha,
|
||||
conclusion=conclusion,
|
||||
completed_at=completed_at,
|
||||
output=output,
|
||||
details_url=details_url,
|
||||
external_id=external_id,
|
||||
actions=actions,
|
||||
)
|
||||
except github3.exceptions.GitHubException as exc:
|
||||
# TODO (felix): Should we retry the check_run creation?
|
||||
log.error(
|
||||
"Failed to create check run %s for %s#%s on sha %s: "
|
||||
"%s",
|
||||
context, project, pr_number, sha, str(exc)
|
||||
)
|
||||
errors.append(
|
||||
"Failed to create check run {}: {}".format(
|
||||
context, str(exc)
|
||||
)
|
||||
)
|
||||
else:
|
||||
check_run = check_runs[0]
|
||||
log.debug(
|
||||
"Updating existing check run %s for %s#%s on sha %s "
|
||||
"with status %s",
|
||||
context, project, pr_number, sha, status,
|
||||
log.debug("Updating existing check run %s for %s#%s on sha %s "
|
||||
"with status %s", context, project, pr_number, sha,
|
||||
status)
|
||||
action = 'update'
|
||||
arguments = dict(
|
||||
conclusion=conclusion,
|
||||
completed_at=completed_at,
|
||||
output=output,
|
||||
details_url=details_url,
|
||||
external_id=external_id,
|
||||
actions=actions,
|
||||
)
|
||||
|
||||
try:
|
||||
check_run.update(
|
||||
conclusion=conclusion,
|
||||
completed_at=completed_at,
|
||||
output=output,
|
||||
details_url=details_url,
|
||||
external_id=external_id,
|
||||
actions=actions,
|
||||
)
|
||||
except github3.exceptions.GitHubException as exc:
|
||||
log.error(
|
||||
"Failed to update check run %s for %s#%s on sha %s: "
|
||||
"%s",
|
||||
context, project, pr_number, sha, str(exc),
|
||||
)
|
||||
errors.append(
|
||||
"Failed to update check run {}: {}".format(
|
||||
context, str(exc)
|
||||
)
|
||||
)
|
||||
|
||||
else:
|
||||
# Add an abort/dequeue action to running check runs
|
||||
actions.append(
|
||||
|
|
@ -2132,35 +2097,33 @@ class GithubConnection(BaseConnection):
|
|||
}
|
||||
)
|
||||
|
||||
# Report the start of a check run
|
||||
try:
|
||||
data = dict(
|
||||
name=context,
|
||||
head_sha=sha,
|
||||
status=status,
|
||||
output=output,
|
||||
details_url=details_url,
|
||||
external_id=external_id,
|
||||
actions=actions,
|
||||
)
|
||||
action = 'create'
|
||||
arguments = dict(
|
||||
name=context,
|
||||
head_sha=sha,
|
||||
status=status,
|
||||
output=output,
|
||||
details_url=details_url,
|
||||
external_id=external_id,
|
||||
actions=actions,
|
||||
)
|
||||
|
||||
url = github.session.build_url('repos', project,
|
||||
'check-runs')
|
||||
resp = github.session.post(url, json=data)
|
||||
resp.raise_for_status()
|
||||
# Create/update the check run
|
||||
try:
|
||||
check_run_id = self._create_or_update_check(
|
||||
github,
|
||||
project,
|
||||
check_run_id,
|
||||
**arguments,
|
||||
)
|
||||
|
||||
except requests.exceptions.HTTPError as exc:
|
||||
log.error(
|
||||
"Failed to create check run %s for %s#%s on sha %s: %s",
|
||||
context, project, pr_number, sha, str(exc),
|
||||
)
|
||||
errors.append(
|
||||
"Failed to update check run {}: {}".format(
|
||||
context, str(exc)
|
||||
)
|
||||
)
|
||||
except requests.exceptions.HTTPError as exc:
|
||||
log.error("Failed to %s check run %s for %s#%s on sha %s: %s",
|
||||
action, context, project, pr_number, sha, str(exc))
|
||||
errors.append("Failed to {} check run {}: {}".format(
|
||||
action, context, str(exc)))
|
||||
|
||||
return errors
|
||||
return check_run_id, errors
|
||||
|
||||
def _buildAnnotationsFromComments(self, file_comments):
|
||||
annotations = []
|
||||
|
|
|
|||
|
|
@ -245,7 +245,8 @@ class GithubReporter(BaseReporter):
|
|||
}
|
||||
)
|
||||
|
||||
return self.connection.updateCheck(
|
||||
state = item.dynamic_state[self.connection.connection_name]
|
||||
check_run_id, errors = self.connection.updateCheck(
|
||||
project,
|
||||
pr_number,
|
||||
sha,
|
||||
|
|
@ -257,8 +258,14 @@ class GithubReporter(BaseReporter):
|
|||
file_comments,
|
||||
external_id,
|
||||
zuul_event_id=item.event,
|
||||
check_run_id=state.get('check_run_id')
|
||||
)
|
||||
|
||||
if check_run_id:
|
||||
state['check_run_id'] = check_run_id
|
||||
|
||||
return errors
|
||||
|
||||
def setLabels(self, item):
|
||||
log = get_annotated_logger(self.log, item.event)
|
||||
project = item.change.project.name
|
||||
|
|
|
|||
|
|
@ -13,7 +13,7 @@
|
|||
# under the License.
|
||||
|
||||
import abc
|
||||
from collections import OrderedDict
|
||||
from collections import OrderedDict, defaultdict
|
||||
import copy
|
||||
import json
|
||||
import logging
|
||||
|
|
@ -2192,6 +2192,10 @@ class QueueItem(object):
|
|||
self._cached_sql_results = {}
|
||||
self.event = event # The trigger event that lead to this queue item
|
||||
|
||||
# Additional container for connection specifig information to be used
|
||||
# by reporters throughout the lifecycle
|
||||
self.dynamic_state = defaultdict(dict)
|
||||
|
||||
def annotateLogger(self, logger):
|
||||
"""Return an annotated logger with the trigger event"""
|
||||
return get_annotated_logger(logger, self.event)
|
||||
|
|
|
|||
Loading…
Reference in New Issue