From 17c6eefabe1c7e22b1cd7e62caa675128af79ee7 Mon Sep 17 00:00:00 2001 From: Daniel King Date: Mon, 6 Apr 2020 17:52:06 -0400 Subject: [PATCH 1/4] [ci] better errors on bad github response Not sure what was wrong, but CI was getting 422s from GitHub. Using a `raise_for_status=True` ClientSession circumvented gidgethubs native error handling logic smothering the HTTP response body where github places critical debugging information. Aiohttp is aware that `raise_for_status` provides no access to the response body. They addressed this in https://github.com/aio-libs/aiohttp/pulls/3892, but that has not been released because 4.0.0 has not yet been released. Another relevant issue: https://github.com/aio-libs/aiohttp/issues/4600. --- ci/ci/ci.py | 6 +++++- ci/ci/github.py | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/ci/ci/ci.py b/ci/ci/ci.py index 1585cf6dc20..8881f09a134 100644 --- a/ci/ci/ci.py +++ b/ci/ci/ci.py @@ -323,7 +323,11 @@ async def on_startup(app): raise_for_status=True, timeout=aiohttp.ClientTimeout(total=60)) app['client_session'] = session - app['github_client'] = gh_aiohttp.GitHubAPI(session, 'ci', oauth_token=oauth_token) + app['github_client'] = gh_aiohttp.GitHubAPI( + aiohttp.ClientSession( + timeout=aiohttp.ClientTimeout(total=60)), + 'ci', + oauth_token=oauth_token) app['batch_client'] = await BatchClient('ci', session=session) with open('/ci-user-secret/sql-config.json', 'r') as f: diff --git a/ci/ci/github.py b/ci/ci/github.py index a7f8b68bdb1..539e7f493e1 100644 --- a/ci/ci/github.py +++ b/ci/ci/github.py @@ -309,7 +309,7 @@ async def post_github_status(self, gh_client, gh_status): except gidgethub.HTTPException as e: log.info(f'{self.short_str()}: notify github of build state failed due to exception: {e}') except aiohttp.client_exceptions.ClientResponseError as e: - log.error(f'{self.short_str()}: Unexpected exception in post to github: {e}') + log.exception(f'{self.short_str()}: Unexpected exception in post to github: {data} {e}') async def _update_github(self, gh): await self._update_last_known_github_status(gh) From f2f6ce2ba852c239b855d04c8ea87c379d30f532 Mon Sep 17 00:00:00 2001 From: Daniel King Date: Mon, 6 Apr 2020 18:24:15 -0400 Subject: [PATCH 2/4] wabhjkladhjksfkdsa --- ci/ci/ci.py | 66 +++++++++++++++++++++++++++++++++++++++++++++++++ ci/ci/github.py | 6 +++-- 2 files changed, 70 insertions(+), 2 deletions(-) diff --git a/ci/ci/ci.py b/ci/ci/ci.py index 8881f09a134..ad65e8baa9f 100644 --- a/ci/ci/ci.py +++ b/ci/ci/ci.py @@ -9,7 +9,73 @@ import aiohttp_session import aiomysql import uvloop +import gidgethub from gidgethub import aiohttp as gh_aiohttp, routing as gh_routing, sansio as gh_sansio +from gidgethub import (BadRequest, GitHubBroken, HTTPException, InvalidField, + RateLimitExceeded, RedirectionException) +from typing import Any, Dict, Mapping, Optional, Tuple, Type + + +def decipher_response(status_code: int, headers: Mapping, + body: bytes) -> Tuple[Any, Optional[gidgethub.sansio.RateLimit], Optional[str]]: + """Decipher an HTTP response for a GitHub API request. + The mapping providing the headers is expected to support lowercase keys. + The parameters of this function correspond to the three main parts + of an HTTP response: the status code, headers, and body. Assuming + no errors which lead to an exception being raised, a 3-item tuple + is returned. The first item is the decoded body (typically a JSON + object, but possibly None or a string depending on the content + type of the body). The second item is an instance of RateLimit + based on what the response specified. + The last item of the tuple is the URL where to request the next + part of results. If there are no more results then None is + returned. Do be aware that the URL can be a URI template and so + may need to be expanded. + If the status code is anything other than 200, 201, or 204, then + an HTTPException is raised. + """ + data = gidgethub.sansio._decode_body(headers.get("content-type"), body) + if status_code in {200, 201, 204}: + return data, gidgethub.sansio.RateLimit.from_http(headers), gidgethub.sansio._next_link(headers.get("link")) + else: + try: + message = data["message"] + except (TypeError, KeyError): + message = None + exc_type: Type[HTTPException] + if status_code >= 500: + exc_type = GitHubBroken + elif status_code >= 400: + exc_type = BadRequest + if status_code == 403: + rate_limit = gidgethub.sansio.RateLimit.from_http(headers) + if rate_limit and not rate_limit.remaining: + raise RateLimitExceeded(rate_limit, message) + elif status_code == 422: + errors = data.get("errors", None) + log.info(f'fml {body.decode()} {errors}') + if errors: + fields = ", ".join(repr(e["field"]) for e in errors) + message = f"{message} for {fields}" + else: + message = data["message"] + raise InvalidField(errors, message) + elif status_code >= 300: + exc_type = RedirectionException + else: + exc_type = HTTPException + import http + status_code_enum = http.HTTPStatus(status_code) + args: Tuple + if message: + args = status_code_enum, message + else: + args = status_code_enum, + raise exc_type(*args) + +gidgethub.sansio.decipher_response = decipher_response + + from hailtop.utils import collect_agen, humanize_timedelta_msecs from hailtop.batch_client.aioclient import BatchClient from hailtop.config import get_deploy_config diff --git a/ci/ci/github.py b/ci/ci/github.py index 539e7f493e1..ef78f5cbe05 100644 --- a/ci/ci/github.py +++ b/ci/ci/github.py @@ -306,8 +306,10 @@ async def post_github_status(self, gh_client, gh_status): await gh_client.post( f'/repos/{self.target_branch.branch.repo.short_str()}/statuses/{self.source_sha}', data=data) + except KeyError as e: + log.exception(f'{self.short_str()}: notify github of build state failed due to exception: {data} {e}') except gidgethub.HTTPException as e: - log.info(f'{self.short_str()}: notify github of build state failed due to exception: {e}') + log.info(f'{self.short_str()}: notify github of build state failed due to exception: {data} {e}') except aiohttp.client_exceptions.ClientResponseError as e: log.exception(f'{self.short_str()}: Unexpected exception in post to github: {data} {e}') @@ -323,7 +325,7 @@ def _hail_github_status_from_statuses(statuses_json): if n_hail_status == 0: return None if n_hail_status == 1: - return hail_status[0] + return hail_status[0]['state'] raise ValueError( f'github sent multiple status summaries for our one ' 'context {GITHUB_STATUS_CONTEXT}: {hail_status}\n\n{statuses_json}') From 2e025e2b0eed8211e24357973bfa0610311a3238 Mon Sep 17 00:00:00 2001 From: Daniel King Date: Mon, 6 Apr 2020 18:25:06 -0400 Subject: [PATCH 3/4] fdsiahfds --- ci/ci/ci.py | 66 ----------------------------------------------------- 1 file changed, 66 deletions(-) diff --git a/ci/ci/ci.py b/ci/ci/ci.py index ad65e8baa9f..8881f09a134 100644 --- a/ci/ci/ci.py +++ b/ci/ci/ci.py @@ -9,73 +9,7 @@ import aiohttp_session import aiomysql import uvloop -import gidgethub from gidgethub import aiohttp as gh_aiohttp, routing as gh_routing, sansio as gh_sansio -from gidgethub import (BadRequest, GitHubBroken, HTTPException, InvalidField, - RateLimitExceeded, RedirectionException) -from typing import Any, Dict, Mapping, Optional, Tuple, Type - - -def decipher_response(status_code: int, headers: Mapping, - body: bytes) -> Tuple[Any, Optional[gidgethub.sansio.RateLimit], Optional[str]]: - """Decipher an HTTP response for a GitHub API request. - The mapping providing the headers is expected to support lowercase keys. - The parameters of this function correspond to the three main parts - of an HTTP response: the status code, headers, and body. Assuming - no errors which lead to an exception being raised, a 3-item tuple - is returned. The first item is the decoded body (typically a JSON - object, but possibly None or a string depending on the content - type of the body). The second item is an instance of RateLimit - based on what the response specified. - The last item of the tuple is the URL where to request the next - part of results. If there are no more results then None is - returned. Do be aware that the URL can be a URI template and so - may need to be expanded. - If the status code is anything other than 200, 201, or 204, then - an HTTPException is raised. - """ - data = gidgethub.sansio._decode_body(headers.get("content-type"), body) - if status_code in {200, 201, 204}: - return data, gidgethub.sansio.RateLimit.from_http(headers), gidgethub.sansio._next_link(headers.get("link")) - else: - try: - message = data["message"] - except (TypeError, KeyError): - message = None - exc_type: Type[HTTPException] - if status_code >= 500: - exc_type = GitHubBroken - elif status_code >= 400: - exc_type = BadRequest - if status_code == 403: - rate_limit = gidgethub.sansio.RateLimit.from_http(headers) - if rate_limit and not rate_limit.remaining: - raise RateLimitExceeded(rate_limit, message) - elif status_code == 422: - errors = data.get("errors", None) - log.info(f'fml {body.decode()} {errors}') - if errors: - fields = ", ".join(repr(e["field"]) for e in errors) - message = f"{message} for {fields}" - else: - message = data["message"] - raise InvalidField(errors, message) - elif status_code >= 300: - exc_type = RedirectionException - else: - exc_type = HTTPException - import http - status_code_enum = http.HTTPStatus(status_code) - args: Tuple - if message: - args = status_code_enum, message - else: - args = status_code_enum, - raise exc_type(*args) - -gidgethub.sansio.decipher_response = decipher_response - - from hailtop.utils import collect_agen, humanize_timedelta_msecs from hailtop.batch_client.aioclient import BatchClient from hailtop.config import get_deploy_config From 2232d6cfec08365cac15baa21e5418e200537c07 Mon Sep 17 00:00:00 2001 From: Daniel King Date: Mon, 6 Apr 2020 18:26:03 -0400 Subject: [PATCH 4/4] wip --- ci/ci/github.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/ci/ci/github.py b/ci/ci/github.py index ef78f5cbe05..f73ba7e646b 100644 --- a/ci/ci/github.py +++ b/ci/ci/github.py @@ -306,8 +306,6 @@ async def post_github_status(self, gh_client, gh_status): await gh_client.post( f'/repos/{self.target_branch.branch.repo.short_str()}/statuses/{self.source_sha}', data=data) - except KeyError as e: - log.exception(f'{self.short_str()}: notify github of build state failed due to exception: {data} {e}') except gidgethub.HTTPException as e: log.info(f'{self.short_str()}: notify github of build state failed due to exception: {data} {e}') except aiohttp.client_exceptions.ClientResponseError as e: