From 2f72b66adfbedd83412b119e377e4a340f238aca Mon Sep 17 00:00:00 2001 From: Aki Sasaki Date: Fri, 11 Jan 2019 14:59:56 -0800 Subject: [PATCH] Don't retry on 404s. --- scriptworker/artifacts.py | 4 ++-- scriptworker/cot/verify.py | 10 +++++----- scriptworker/exceptions.py | 26 ++++++++++++++++++++++---- scriptworker/test/__init__.py | 31 ++++++++++++++++++------------- scriptworker/test/test_utils.py | 11 ++++++++++- scriptworker/utils.py | 16 ++++++++++++---- 6 files changed, 69 insertions(+), 29 deletions(-) diff --git a/scriptworker/artifacts.py b/scriptworker/artifacts.py index 5cf353f5..a2bb6b7e 100644 --- a/scriptworker/artifacts.py +++ b/scriptworker/artifacts.py @@ -290,8 +290,8 @@ async def download_artifacts(context, file_urls, parent_dir=None, session=None, list: the full paths to the files downloaded Raises: - scriptworker.exceptions.DownloadError: on download failure after - max retries. + scriptworker.exceptions.BaseDownloadError: on download failure after + any applicable retries. """ parent_dir = parent_dir or context.config['work_dir'] diff --git a/scriptworker/cot/verify.py b/scriptworker/cot/verify.py index f4d53ea3..1fbba590 100644 --- a/scriptworker/cot/verify.py +++ b/scriptworker/cot/verify.py @@ -28,7 +28,7 @@ from scriptworker.config import read_worker_creds, apply_product_config from scriptworker.constants import DEFAULT_CONFIG from scriptworker.context import Context -from scriptworker.exceptions import CoTError, DownloadError, ScriptWorkerGPGException +from scriptworker.exceptions import CoTError, BaseDownloadError, ScriptWorkerGPGException from scriptworker.gpg import get_body, GPG from scriptworker.log import contextual_log_handler from scriptworker.task import ( @@ -591,7 +591,7 @@ async def download_cot(chain): chain (ChainOfTrust): the chain of trust to add to. Raises: - DownloadError: on failure. + BaseDownloadError: on failure. """ mandatory_artifact_tasks = [] @@ -689,7 +689,7 @@ async def download_cot_artifacts(chain): Raises: CoTError: on chain of trust sha validation error, on a mandatory artifact - DownloadError: on download error on a mandatory artifact + BaseDownloadError: on download error on a mandatory artifact """ upstream_artifacts = chain.task['payload'].get('upstreamArtifacts', []) @@ -1489,7 +1489,7 @@ async def verify_parent_task(chain, link): verify_link_in_task_graph(chain, link, target_link) try: await verify_parent_task_definition(chain, link) - except (DownloadError, KeyError) as e: + except (BaseDownloadError, KeyError) as e: raise CoTError(e) @@ -1955,7 +1955,7 @@ async def verify_chain_of_trust(chain): # verify the worker_impls, e.g. docker-worker await verify_worker_impls(chain) await trace_back_to_tree(chain) - except (DownloadError, KeyError, AttributeError) as exc: + except (BaseDownloadError, KeyError, AttributeError) as exc: log.critical("Chain of Trust verification error!", exc_info=True) if isinstance(exc, CoTError): raise diff --git a/scriptworker/exceptions.py b/scriptworker/exceptions.py index 58c2829d..82884ccf 100644 --- a/scriptworker/exceptions.py +++ b/scriptworker/exceptions.py @@ -88,8 +88,8 @@ def __init__(self, msg): super().__init__(msg, exit_code=STATUSES['malformed-payload']) -class DownloadError(ScriptWorkerTaskException): - """Failure in ``scriptworker.utils.download_file``. +class BaseDownloadError(ScriptWorkerTaskException): + """Base class for DownloadError and Download404. Attributes: exit_code (int): this is set to 4 (resource-unavailable). @@ -97,17 +97,35 @@ class DownloadError(ScriptWorkerTaskException): """ def __init__(self, msg): - """Initialize DownloadError. + """Initialize Download404. Args: msg (string): the error message """ - super(DownloadError, self).__init__( + super(BaseDownloadError, self).__init__( msg, exit_code=STATUSES['resource-unavailable'] ) +class Download404(BaseDownloadError): + """404 in ``scriptworker.utils.download_file``. + + Attributes: + exit_code (int): this is set to 4 (resource-unavailable). + + """ + + +class DownloadError(BaseDownloadError): + """Failure in ``scriptworker.utils.download_file``. + + Attributes: + exit_code (int): this is set to 4 (resource-unavailable). + + """ + + class CoTError(ScriptWorkerTaskException, KeyError): """Failure in Chain of Trust verification. diff --git a/scriptworker/test/__init__.py b/scriptworker/test/__init__.py index a6575141..9960b3ff 100644 --- a/scriptworker/test/__init__.py +++ b/scriptworker/test/__init__.py @@ -6,6 +6,7 @@ import arrow import asyncio from copy import deepcopy +import functools import json import mock import os @@ -168,17 +169,18 @@ def unsuccessful_queue(): return UnsuccessfulQueue() +@asyncio.coroutine +def _fake_request(resp_status, method, url, *args, **kwargs): + resp = FakeResponse(method, url, status=resp_status) + resp._history = (FakeResponse(method, url, status=302),) + return resp + + @pytest.mark.asyncio @pytest.fixture(scope='function') async def fake_session(): - @asyncio.coroutine - def _fake_request(method, url, *args, **kwargs): - resp = FakeResponse(method, url) - resp._history = (FakeResponse(method, url, status=302),) - return resp - session = aiohttp.ClientSession() - session._request = _fake_request + session._request = functools.partial(_fake_request, 200) yield session await session.close() @@ -186,14 +188,17 @@ def _fake_request(method, url, *args, **kwargs): @pytest.mark.asyncio @pytest.fixture(scope='function') async def fake_session_500(): - @asyncio.coroutine - def _fake_request(method, url, *args, **kwargs): - resp = FakeResponse(method, url, status=500) - resp._history = (FakeResponse(method, url, status=302),) - return resp + session = aiohttp.ClientSession() + session._request = functools.partial(_fake_request, 500) + yield session + await session.close() + +@pytest.mark.asyncio +@pytest.fixture(scope='function') +async def fake_session_404(): session = aiohttp.ClientSession() - session._request = _fake_request + session._request = functools.partial(_fake_request, 404) yield session await session.close() diff --git a/scriptworker/test/test_utils.py b/scriptworker/test/test_utils.py index 0e8be9a3..d14be342 100644 --- a/scriptworker/test/test_utils.py +++ b/scriptworker/test/test_utils.py @@ -9,11 +9,12 @@ import re import shutil import tempfile -from scriptworker.exceptions import DownloadError, ScriptWorkerException, ScriptWorkerRetryException +from scriptworker.exceptions import DownloadError, Download404, ScriptWorkerException, ScriptWorkerRetryException import scriptworker.utils as utils from . import ( FakeResponse, fake_session, + fake_session_404, fake_session_500, noop_async, tmpdir, @@ -23,6 +24,7 @@ assert tmpdir, context # silence flake8 assert fake_session, fake_session_500 # silence flake8 +assert fake_session_404 # silence flake8 # constants helpers and fixtures {{{1 # from https://github.com/SecurityInnovation/PGPy/blob/develop/tests/test_01_types.py @@ -327,6 +329,13 @@ async def test_download_file_exception(context, fake_session_500, tmpdir): await utils.download_file(context, "url", path, session=fake_session_500) +@pytest.mark.asyncio +async def test_download_file_404(context, fake_session_404, tmpdir): + path = os.path.join(tmpdir, "foo") + with pytest.raises(Download404): + await utils.download_file(context, "url", path, session=fake_session_404) + + # format_json {{{1 def test_format_json(): expected = '\n'.join([ diff --git a/scriptworker/utils.py b/scriptworker/utils.py index 7679d9f9..93b7d82d 100644 --- a/scriptworker/utils.py +++ b/scriptworker/utils.py @@ -23,6 +23,7 @@ from taskcluster.client import createTemporaryCredentials from scriptworker.exceptions import ( DownloadError, + Download404, ScriptWorkerException, ScriptWorkerRetryException, ScriptWorkerTaskException, @@ -466,6 +467,12 @@ def load_json_or_yaml(string, is_path=False, file_type='json', # download_file {{{1 +async def _log_download_error(resp, msg): + log.debug(msg, {'url': get_loggable_url(str(resp.url)), 'status': resp.status, 'body': (await resp.text())[:1000]}) + for i, h in enumerate(resp.history): + log.debug("Redirect history %s: %s; body=%s", get_loggable_url(str(h.url)), h.status, (await h.text())[:1000]) + + async def download_file(context, url, abs_filename, session=None, chunk_size=128): """Download a file, async. @@ -484,10 +491,11 @@ async def download_file(context, url, abs_filename, session=None, chunk_size=128 log.info("Downloading %s", loggable_url) parent_dir = os.path.dirname(abs_filename) async with session.get(url) as resp: - if resp.status != 200: - log.debug("Failed to download %s: %s; body=%s", get_loggable_url(str(resp.url)), resp.status, (await resp.text())[:1000]) - for i, h in enumerate(resp.history): - log.debug("Redirect history %s: %s; body=%s", get_loggable_url(str(h.url)), h.status, (await h.text())[:1000]) + if resp.status == 404: + await _log_download_error(resp, "404 downloading %(url)s: %(status)s; body=%(body)s") + raise Download404("{} status {}!".format(loggable_url, resp.status)) + elif resp.status != 200: + await _log_download_error(resp, "Failed to download %(url)s: %(status)s; body=%(body)s") raise DownloadError("{} status {} is not 200!".format(loggable_url, resp.status)) makedirs(parent_dir) with open(abs_filename, "wb") as fd: