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,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

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), 
+            subprocess.check_call('git reset --hard {}'.format(self.git_hash),
-                stdout=sys.stdout, stderr=sys.stderr, shell=True)
+                                  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, 
+            subprocess.check_call('git checkout master', stdout=sys.stdout,
-                stderr=sys.stderr, shell=True)
+                                  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 '\
+        cmd = 'arc patch --force --nobranch --no-ansi --diff "{}" --nocommit ' \
-                '--conduit-token "{}" --conduit-uri "{}"'.format(
+              '--conduit-token "{}" --conduit-uri "{}"'.format(
-            self.diff_id, self.conduit_token, self.host )
+            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()
-

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)

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

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': {

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.
+                *checkout_scripts('linux', scripts_refspec),
-                '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"',
-
                 '${SRC}/scripts/summary.py',
             ],
             'artifact_paths': ['artifacts/**/*'],

scripts/requirements.txt

@@ -1,9 +1,9 @@
 backoff==1.10.0
-gitpython==3.0.6
+gitpython==3.1.9
-lxml==4.4.1
+lxml==4.5.2
-pathspec==0.7.0
+pathspec==0.8.0
 phabricator==0.7.0
-pyaml==19.12.0
+pyaml==20.4.0
-requests==2.22.0
+requests==2.24.0
 retrying==1.3.3
-unidiff==0.5.5
+unidiff==0.6.0

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.
+        *checkout_scripts('linux', scripts_refspec),
-        '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"',
         'set +e',
     ]
 
@@ -106,17 +97,7 @@ def generic_windows(projects: str) -> List:
         'commands': [
             clear_sccache if no_cache else '',
             'sccache --zero-stats',
-
+            *checkout_scripts('windows', scripts_refspec),
-            # 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%',
 
             '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"',
+    ]