From 43fae020f51d6ccf9e873d59438607d92c66ceb8 Mon Sep 17 00:00:00 2001 From: Mikhail Goncharov Date: Mon, 12 Oct 2020 16:25:23 +0200 Subject: [PATCH] retry most of the requests with backoff fixes #192 additionally: - install python libs after scripts checkout, so we don't need to rebuild images and restart agents only to add a new python dependency - updated lib versions - similar scripts checkout in steps --- scripts/buildkite_utils.py | 3 ++ scripts/phabtalk/apply_patch.py | 33 +++++++++++--------- scripts/phabtalk/apply_patch2.py | 47 +++++++++++----------------- scripts/phabtalk/phabtalk.py | 43 ++++++++++++-------------- scripts/pipeline_create_branch.py | 5 ++- scripts/pipeline_premerge.py | 14 ++------- scripts/requirements.txt | 12 ++++---- scripts/steps.py | 51 ++++++++++++++++++------------- 8 files changed, 100 insertions(+), 108 deletions(-) diff --git a/scripts/buildkite_utils.py b/scripts/buildkite_utils.py index 37e25b7..9696c87 100644 --- a/scripts/buildkite_utils.py +++ b/scripts/buildkite_utils.py @@ -3,6 +3,8 @@ import os import re import subprocess from typing import Optional + +import backoff import requests @@ -28,6 +30,7 @@ class BuildkiteApi: self.token = token self.organization = organization + @backoff.on_exception(backoff.expo, Exception, max_tries=3, logger='', factor=3) def get_build(self, pipeline: str, build_number: str): authorization = f'Bearer {self.token}' # https://buildkite.com/docs/apis/rest-api/builds#get-a-build diff --git a/scripts/phabtalk/apply_patch.py b/scripts/phabtalk/apply_patch.py index 0caf138..f4336c3 100755 --- a/scripts/phabtalk/apply_patch.py +++ b/scripts/phabtalk/apply_patch.py @@ -18,6 +18,8 @@ import os import subprocess import sys from typing import List, Optional + +import backoff from phabricator import Phabricator @@ -27,11 +29,11 @@ class ApplyPatch: # TODO: turn os.environ parameter into command line arguments # this would be much clearer and easier for testing self.comment_file_path = comment_file_path - self.conduit_token = os.environ.get('CONDUIT_TOKEN') # type: Optional[str] + self.conduit_token = os.environ.get('CONDUIT_TOKEN') # type: Optional[str] self.host = os.environ.get('PHABRICATOR_HOST') # type: Optional[str] self._load_arcrc() self.diff_id = os.environ['DIFF_ID'] # type: str - self.diff_json_path = os.environ['DIFF_JSON'] # type: str + self.diff_json_path = os.environ['DIFF_JSON'] # type: str if not self.host.endswith('/api/'): self.host += '/api/' self.phab = Phabricator(token=self.conduit_token, host=self.host) @@ -49,12 +51,14 @@ class ApplyPatch: self.host = next(iter(arcrc['hosts'])) self.conduit_token = arcrc['hosts'][self.host]['token'] + @backoff.on_exception(backoff.expo, Exception, max_tries=5, logger='', factor=3) + def update_interfaces(self): + self.phab.update_interfaces() + def run(self): """try to apply the patch from phabricator - - Write to `self.comment_file` for showing error messages on Phabricator. """ - self.phab.update_interfaces() + self.update_interfaces() try: if self.git_hash is None: @@ -77,11 +81,12 @@ class ApplyPatch: print('WARNING: could not write build/diff.json log file') self.git_hash = diff['sourceControlBaseRevision'] + @backoff.on_exception(backoff.expo, Exception, max_tries=5, logger='', factor=3) def _git_checkout(self): try: print('Checking out git hash {}'.format(self.git_hash)) - subprocess.check_call('git reset --hard {}'.format(self.git_hash), - stdout=sys.stdout, stderr=sys.stderr, shell=True) + subprocess.check_call('git reset --hard {}'.format(self.git_hash), + stdout=sys.stdout, stderr=sys.stderr, shell=True) except subprocess.CalledProcessError: print('WARNING: checkout of hash failed, using master branch instead.') self.msg += [ @@ -89,20 +94,21 @@ class ApplyPatch: 'the repository. Did you configure the "Parent Revision" in ' 'Phabricator properly? Trying to apply the patch to the ' 'master branch instead...'.format(self.git_hash)] - subprocess.check_call('git checkout master', stdout=sys.stdout, - stderr=sys.stderr, shell=True) + subprocess.check_call('git checkout master', stdout=sys.stdout, + stderr=sys.stderr, shell=True) subprocess.check_call('git show -s', stdout=sys.stdout, stderr=sys.stderr, shell=True) print('git checkout completed.') + @backoff.on_exception(backoff.expo, Exception, max_tries=5, logger='', factor=3) def _apply_patch(self): print('running arc patch...') - cmd = 'arc patch --force --nobranch --no-ansi --diff "{}" --nocommit '\ - '--conduit-token "{}" --conduit-uri "{}"'.format( - self.diff_id, self.conduit_token, self.host ) + cmd = 'arc patch --force --nobranch --no-ansi --diff "{}" --nocommit ' \ + '--conduit-token "{}" --conduit-uri "{}"'.format( + self.diff_id, self.conduit_token, self.host) result = subprocess.run(cmd, capture_output=True, shell=True, text=True) print(result.stdout + result.stderr) - if result.returncode != 0: + if result.returncode != 0: msg = ( 'ERROR: arc patch failed with error code {}. ' 'Check build log for details.'.format(result.returncode)) @@ -130,4 +136,3 @@ if __name__ == "__main__": args = parser.parse_args() patcher = ApplyPatch(args.comment_file_path, args.commit) patcher.run() - diff --git a/scripts/phabtalk/apply_patch2.py b/scripts/phabtalk/apply_patch2.py index 1784e16..d6dfe29 100755 --- a/scripts/phabtalk/apply_patch2.py +++ b/scripts/phabtalk/apply_patch2.py @@ -25,9 +25,9 @@ import sys import time from typing import List, Optional, Tuple -from phabricator import Phabricator +import backoff from git import Repo, BadName, GitCommandError - +from phabricator import Phabricator # FIXME: maybe move to config file """URL of upstream LLVM repository.""" @@ -62,7 +62,7 @@ class ApplyPatch: self.host = url # type: Optional[str] self._load_arcrc() self.diff_id = diff_id # type: int - self.phid = phid # type: str + self.phid = phid # type: str if not self.host.endswith('/api/'): self.host += '/api/' self.phab = self._create_phab() @@ -137,14 +137,12 @@ class ApplyPatch: logging.error(f'exception: {e}') return 1 + @backoff.on_exception(backoff.expo, Exception, max_tries=5, logger='', factor=3) def _refresh_master(self): """Update local git repo and origin. As origin is disjoint from upstream, it needs to be updated by this script. """ - if not self.push_branch: - return - logging.info('Syncing local, origin and upstream...') self.repo.git.clean('-ffxdq') self.repo.git.reset('--hard') @@ -155,12 +153,10 @@ class ApplyPatch: self.repo.remotes.upstream.fetch() self.repo.git.pull('origin', 'master') self.repo.git.pull('upstream', 'master') - try: + if self.push_branch: self.repo.git.push('origin', 'master') - logging.info('refresh of master branch completed') - except GitCommandError as e: - logging.info('Info: Could not push to origin master.') + @backoff.on_exception(backoff.expo, Exception, max_tries=5, logger='', factor=3) def _create_branch(self, base_revision: Optional[str]): self.repo.git.fetch('--all') try: @@ -179,6 +175,7 @@ class ApplyPatch: self.repo.head.reset(index=True, working_tree=True) logging.info('Base branch revision is {}'.format(self.repo.head.commit.hexsha)) + @backoff.on_exception(backoff.expo, Exception, max_tries=5, logger='', factor=3) def _commit_and_push(self, revision_id): """Commit the patch and push it to origin.""" if not self.push_branch: @@ -196,19 +193,23 @@ Review-ID: {} logging.info('Branch {} pushed to origin'.format(self.branch_name)) pass + @backoff.on_exception(backoff.expo, Exception, max_tries=5, logger='', factor=3) def _create_phab(self): phab = Phabricator(token=self.conduit_token, host=self.host) - try_call(lambda: phab.update_interfaces()) + phab.update_interfaces() return phab + @backoff.on_exception(backoff.expo, Exception, max_tries=5, logger='', factor=3) def _get_diff(self, diff_id: int): """Get a diff from Phabricator based on it's diff id.""" - return try_call(lambda: self.phab.differential.getdiff(diff_id=diff_id)) + return self.phab.differential.getdiff(diff_id=diff_id) + @backoff.on_exception(backoff.expo, Exception, max_tries=5, logger='', factor=3) def _get_revision(self, revision_id: int): """Get a revision from Phabricator based on its revision id.""" - return try_call(lambda: self.phab.differential.query(ids=[revision_id])[0]) + return self.phab.differential.query(ids=[revision_id])[0] + @backoff.on_exception(backoff.expo, Exception, max_tries=5, logger='', factor=3) def _get_revisions(self, *, phids: List[str] = None): """Get a list of revisions from Phabricator based on their PH-IDs.""" if phids is None: @@ -217,7 +218,7 @@ Review-ID: {} # Handle an empty query locally. Otherwise the connection # will time out. return [] - return try_call(lambda: self.phab.differential.query(phids=phids)) + return self.phab.differential.query(phids=phids) def _get_dependencies(self, diff_id) -> Tuple[int, List[int], str]: """Get all dependencies for the diff. @@ -243,10 +244,11 @@ Review-ID: {} return revision_id, result, base_revision + @backoff.on_exception(backoff.expo, Exception, max_tries=5, logger='', factor=3) def _apply_diff(self, diff_id: int, revision_id: int): """Download and apply a diff to the local working copy.""" logging.info('Applying diff {} for revision {}...'.format(diff_id, diff_to_str(revision_id))) - diff = try_call(lambda: self.phab.differential.getrawdiff(diffID=str(diff_id)).response) + diff = self.phab.differential.getrawdiff(diffID=str(diff_id)).response logging.debug(f'diff {diff_id}:\n{diff}') proc = subprocess.run('git apply -', input=diff, shell=True, text=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) @@ -303,21 +305,6 @@ def diff_list_to_str(diffs: List[int]) -> str: return ', '.join([diff_to_str(d) for d in diffs]) -def try_call(call): - """Tries to call function several times retrying on socked.timeout.""" - c = 0 - while True: - try: - return call() - except socket.timeout as e: - c += 1 - if c > 5: - logging.error('Connection to Pharicator failed, giving up: {}'.format(e)) - raise - logging.error('Connection to Pharicator failed, retrying: {}'.format(e)) - time.sleep(c * 10) - - if __name__ == "__main__": parser = argparse.ArgumentParser(description='Apply Phabricator patch to working directory.') parser.add_argument('diff_id', type=int) diff --git a/scripts/phabtalk/phabtalk.py b/scripts/phabtalk/phabtalk.py index ded4e57..66f7c0d 100755 --- a/scripts/phabtalk/phabtalk.py +++ b/scripts/phabtalk/phabtalk.py @@ -16,12 +16,14 @@ Interactions with Phabricator. """ +import logging import socket import time -import uuid from typing import Optional, List, Dict +import uuid + +import backoff from phabricator import Phabricator -import logging class PhabTalk: @@ -29,16 +31,22 @@ class PhabTalk: See https://secure.phabricator.com/conduit/method/harbormaster.sendmessage/ """ - def __init__(self, token: Optional[str], host: Optional[str] = 'https://reviews.llvm.org/api/', dryrun: bool = False): + def __init__(self, token: Optional[str], host: Optional[str] = 'https://reviews.llvm.org/api/', + dryrun: bool = False): self._phab = None # type: Optional[Phabricator] if not dryrun: self._phab = Phabricator(token=token, host=host) - _try_call(self._phab.update_interfaces) + self.update_interfaces() + + @backoff.on_exception(backoff.expo, Exception, max_tries=5, logger='', factor=3) + def update_interfaces(self): + self._phab.update_interfaces() @property def dryrun(self): return self._phab is None + @backoff.on_exception(backoff.expo, Exception, max_tries=5, logger='', factor=3) def get_revision_id(self, diff: str) -> Optional[str]: """Get the revision ID for a diff from Phabricator.""" if self.dryrun: @@ -55,6 +63,7 @@ class PhabTalk: if revision_id is not None: self._comment_on_revision(revision_id, text) + @backoff.on_exception(backoff.expo, Exception, max_tries=5, logger='', factor=3) def _comment_on_revision(self, revision: str, text: str): """Add comment on a differential based on the revision id.""" @@ -74,6 +83,7 @@ class PhabTalk: transactions=transactions) print('Uploaded comment to Revision D{}:{}'.format(revision, text)) + @backoff.on_exception(backoff.expo, Exception, max_tries=5, logger='', factor=3) def update_build_status(self, phid: str, working: bool, success: bool, lint: {}, unit: []): """Submit collected report to Phabricator. """ @@ -111,14 +121,15 @@ class PhabTalk: print('lint: {}'.format(lint_messages)) return - _try_call(lambda: self._phab.harbormaster.sendmessage( + self._phab.harbormaster.sendmessage( buildTargetPHID=phid, type=result_type, unit=unit, - lint=lint_messages)) + lint=lint_messages) print('Uploaded build status {}, {} test results and {} lint results'.format( result_type, len(unit), len(lint_messages))) + @backoff.on_exception(backoff.expo, Exception, max_tries=5, logger='', factor=3) def create_artifact(self, phid, artifact_key, artifact_type, artifact_data): if self.dryrun: print('harbormaster.createartifact =================') @@ -126,11 +137,11 @@ class PhabTalk: print('artifactType: {}'.format(artifact_type)) print('artifactData: {}'.format(artifact_data)) return - _try_call(lambda: self._phab.harbormaster.createartifact( + self._phab.harbormaster.createartifact( buildTargetPHID=phid, artifactKey=artifact_key, artifactType=artifact_type, - artifactData=artifact_data)) + artifactData=artifact_data) def maybe_add_url_artifact(self, phid: str, url: str, name: str): if phid is None: @@ -180,19 +191,3 @@ class Report: def add_artifact(self, dir: str, file: str, name: str): self.artifacts.append({'dir': dir, 'file': file, 'name': name}) - - -def _try_call(call): - """Tries to call function several times retrying on socked.timeout.""" - c = 0 - while True: - try: - call() - except socket.timeout as e: - c += 1 - if c > 5: - print('Connection to Pharicator failed, giving up: {}'.format(e)) - raise - print('Connection to Pharicator failed, retrying: {}'.format(e)) - time.sleep(c * 10) - break diff --git a/scripts/pipeline_create_branch.py b/scripts/pipeline_create_branch.py index bcf1022..520c804 100755 --- a/scripts/pipeline_create_branch.py +++ b/scripts/pipeline_create_branch.py @@ -29,7 +29,10 @@ if __name__ == '__main__': steps.append({ 'label': 'create branch', 'key': 'create-branch', - 'commands': ['scripts/apply_patch.sh'], + 'commands': [ + 'pip install -r scripts/requirements.txt', + 'scripts/apply_patch.sh' + ], 'agents': {'queue': 'linux'}, 'timeout_in_minutes': 20, 'env': { diff --git a/scripts/pipeline_premerge.py b/scripts/pipeline_premerge.py index 1cee08a..30b7fcc 100755 --- a/scripts/pipeline_premerge.py +++ b/scripts/pipeline_premerge.py @@ -19,7 +19,7 @@ import os from choose_projects import ChooseProjects import git from phabtalk.phabtalk import PhabTalk -from steps import generic_linux, generic_windows, from_shell_output +from steps import generic_linux, generic_windows, from_shell_output, checkout_scripts import yaml steps_generators = [ @@ -80,17 +80,7 @@ if __name__ == '__main__': report_step = { 'label': ':spiral_note_pad: report', 'commands': [ - # Clone scripts. - 'export SRC=${BUILDKITE_BUILD_PATH}/llvm-premerge-checks', - 'rm -rf ${SRC}', - 'git clone --depth 1 https://github.com/google/llvm-premerge-checks.git "${SRC}"', - 'cd ${SRC}', - f'git fetch origin "{scripts_refspec}":x', - 'git checkout x', - 'echo "llvm-premerge-checks commit"', - 'git rev-parse HEAD', - 'cd "$BUILDKITE_BUILD_CHECKOUT_PATH"', - + *checkout_scripts('linux', scripts_refspec), '${SRC}/scripts/summary.py', ], 'artifact_paths': ['artifacts/**/*'], diff --git a/scripts/requirements.txt b/scripts/requirements.txt index ddc7f68..503aee0 100644 --- a/scripts/requirements.txt +++ b/scripts/requirements.txt @@ -1,9 +1,9 @@ backoff==1.10.0 -gitpython==3.0.6 -lxml==4.4.1 -pathspec==0.7.0 +gitpython==3.1.9 +lxml==4.5.2 +pathspec==0.8.0 phabricator==0.7.0 -pyaml==19.12.0 -requests==2.22.0 +pyaml==20.4.0 +requests==2.24.0 retrying==1.3.3 -unidiff==0.5.5 \ No newline at end of file +unidiff==0.6.0 \ No newline at end of file diff --git a/scripts/steps.py b/scripts/steps.py index 54c2748..43ae72e 100644 --- a/scripts/steps.py +++ b/scripts/steps.py @@ -41,16 +41,7 @@ def generic_linux(projects: str, check_diff: bool) -> List: 'ccache --show-config', 'mkdir -p artifacts', 'dpkg -l >> artifacts/packages.txt', - # Clone scripts. - 'export SRC=${BUILDKITE_BUILD_PATH}/llvm-premerge-checks', - 'rm -rf ${SRC}', - 'git clone --depth 1 https://github.com/google/llvm-premerge-checks.git "${SRC}"', - 'cd ${SRC}', - f'git fetch origin "{scripts_refspec}":x', - 'git checkout x', - 'echo "llvm-premerge-checks commit"', - 'git rev-parse HEAD', - 'cd "$BUILDKITE_BUILD_CHECKOUT_PATH"', + *checkout_scripts('linux', scripts_refspec), 'set +e', ] @@ -106,17 +97,7 @@ def generic_windows(projects: str) -> List: 'commands': [ clear_sccache if no_cache else '', 'sccache --zero-stats', - - # Clone scripts. - 'set SRC=%BUILDKITE_BUILD_PATH%/llvm-premerge-checks', - 'rm -rf %SRC%', - 'git clone --depth 1 https://github.com/google/llvm-premerge-checks.git %SRC%', - 'cd %SRC%', - f'git fetch origin "{scripts_refspec}":x', - 'git checkout x', - 'echo llvm-premerge-checks commit:', - 'git rev-parse HEAD', - 'cd %BUILDKITE_BUILD_CHECKOUT_PATH%', + *checkout_scripts('windows', scripts_refspec), 'powershell -command "' f'%SRC%/scripts/premerge_checks.py --projects=\'{projects}\' --log-level={log_level} {filter_output}; ' @@ -170,3 +151,31 @@ def from_shell_output(command) -> []: stdout: >>>{out.getvalue().decode()}>>>''') return steps + + +def checkout_scripts(target_os: str, scripts_refspec: str) -> []: + if target_os == 'windows': + return [ + 'set SRC=%BUILDKITE_BUILD_PATH%/llvm-premerge-checks', + 'rm -rf %SRC%', + 'git clone --depth 1 https://github.com/google/llvm-premerge-checks.git %SRC%', + 'cd %SRC%', + f'git fetch origin "{scripts_refspec}":x', + 'git checkout x', + 'echo llvm-premerge-checks commit:', + 'git rev-parse HEAD', + 'pip install -r %SRC%/scripts/requirements.txt', + 'cd %BUILDKITE_BUILD_CHECKOUT_PATH%', + ] + return [ + 'export SRC=${BUILDKITE_BUILD_PATH}/llvm-premerge-checks', + 'rm -rf ${SRC}', + 'git clone --depth 1 https://github.com/google/llvm-premerge-checks.git "${SRC}"', + 'cd ${SRC}', + f'git fetch origin "{scripts_refspec}":x', + 'git checkout x', + 'echo "llvm-premerge-checks commit"', + 'git rev-parse HEAD', + 'pip install -r ${SRC}/scripts/requirements.txt', + 'cd "$BUILDKITE_BUILD_CHECKOUT_PATH"', + ]