From b6ac7f074de59573f82661bf72ed2efd1122356d Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 23 Nov 2017 13:49:52 +0000 Subject: [PATCH 1/2] celery.task.retry exc param should be a throwable. This causes an issue when it hits the max retry limit, and tries to throw your exception to let you deal with it - at this point it was moaning because we pass in a string if it's not defined, and we're inside an exception block celery uses that instead. --- app/celery/callback_tasks.py | 4 ++-- app/celery/tasks.py | 9 ++++----- tests/app/celery/test_tasks.py | 11 +++++------ 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/app/celery/callback_tasks.py b/app/celery/callback_tasks.py index 3de4b746e..6f59a33f5 100644 --- a/app/celery/callback_tasks.py +++ b/app/celery/callback_tasks.py @@ -13,6 +13,6 @@ def process_ses_results(self, response): errors = process_ses_response(response) if errors: current_app.logger.error(errors) - except Exception: + except Exception as exc: current_app.logger.exception('Error processing SES results') - self.retry(queue=QueueNames.RETRY, exc="SES responses processed with error") + self.retry(queue=QueueNames.RETRY) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index e96d7e3d8..7cfb06385 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -323,8 +323,9 @@ def build_dvla_file(self, job_id): ) dao_update_job_status(job_id, JOB_STATUS_READY_TO_SEND) else: - current_app.logger.info("All notifications for job {} are not persisted".format(job_id)) - self.retry(queue=QueueNames.RETRY, exc="All notifications for job {} are not persisted".format(job_id)) + msg = "All notifications for job {} are not persisted".format(job_id) + current_app.logger.info(msg) + self.retry(queue=QueueNames.RETRY, exc=Exception(msg)) except Exception as e: # ? should this retry? current_app.logger.exception("build_dvla_file threw exception") @@ -520,9 +521,7 @@ def send_inbound_sms_to_service(self, inbound_sms_id, service_id): ) if not isinstance(e, HTTPError) or e.response.status_code >= 500: try: - self.retry(queue=QueueNames.RETRY, - exc='Unable to send_inbound_sms_to_service for service_id: {} and url: {}. \n{}'.format( - service_id, inbound_api.url, e)) + self.retry(queue=QueueNames.RETRY) except self.MaxRetriesExceededError: current_app.logger.exception('Retry: send_inbound_sms_to_service has retried the max number of times') diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index b3a046897..16498c40c 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1,8 +1,7 @@ -import codecs import json import uuid from datetime import datetime, timedelta -from unittest.mock import Mock +from unittest.mock import Mock, ANY import pytest import requests_mock from flask import current_app @@ -1049,8 +1048,10 @@ def test_build_dvla_file_retries_if_all_notifications_are_not_created(sample_let build_dvla_file(job.id) mocked.assert_not_called() - tasks.build_dvla_file.retry.assert_called_with(queue="retry-tasks", - exc="All notifications for job {} are not persisted".format(job.id)) + tasks.build_dvla_file.retry.assert_called_with( + queue="retry-tasks", + exc=ANY + ) assert Job.query.get(job.id).job_status == 'in progress' mocked_send_task.assert_not_called() @@ -1157,7 +1158,6 @@ def test_send_inbound_sms_to_service_retries_if_request_returns_500(notify_api, ) assert mocked.call_count == 1 assert mocked.call_args[1]['queue'] == 'retry-tasks' - assert exc_msg in mocked.call_args[1]['exc'] def test_send_inbound_sms_to_service_retries_if_request_throws_unknown(notify_api, sample_service, mocker): @@ -1177,7 +1177,6 @@ def test_send_inbound_sms_to_service_retries_if_request_throws_unknown(notify_ap ) assert mocked.call_count == 1 assert mocked.call_args[1]['queue'] == 'retry-tasks' - assert exc_msg in mocked.call_args[1]['exc'] def test_send_inbound_sms_to_service_does_not_retries_if_request_returns_404(notify_api, sample_service, mocker): From 1e4a4802989605eee9740023619d8ecd018bcb07 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 27 Nov 2017 15:11:58 +0000 Subject: [PATCH 2/2] only catch s3 and db errors in build-dvla-file-for-job this reduces the amount of error messages we log (we'll no longer log at error level when build-dvla-file-for-job retries while waiting for the task to finish), and make sure we retry in those cases above - db or s3 having temporary troubl --- app/celery/tasks.py | 10 ++++++---- tests/app/celery/test_tasks.py | 29 ++++++++++++++++++++--------- 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 7cfb06385..3a146cdea 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -18,6 +18,8 @@ from requests import ( RequestException ) from sqlalchemy.exc import SQLAlchemyError +from botocore.exceptions import ClientError as BotoClientError + from app import ( create_uuid, create_random_identifier, @@ -325,11 +327,11 @@ def build_dvla_file(self, job_id): else: msg = "All notifications for job {} are not persisted".format(job_id) current_app.logger.info(msg) - self.retry(queue=QueueNames.RETRY, exc=Exception(msg)) - except Exception as e: - # ? should this retry? + self.retry(queue=QueueNames.RETRY) + # specifically don't catch celery.retry errors + except (SQLAlchemyError, BotoClientError): current_app.logger.exception("build_dvla_file threw exception") - raise e + self.retry(queue=QueueNames.RETRY) @notify_celery.task(bind=True, name='update-letter-job-to-sent') diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 16498c40c..ad516a5e4 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1,15 +1,17 @@ import json import uuid from datetime import datetime, timedelta -from unittest.mock import Mock, ANY +from unittest.mock import Mock + import pytest import requests_mock from flask import current_app from freezegun import freeze_time from requests import RequestException from sqlalchemy.exc import SQLAlchemyError -from notifications_utils.template import SMSMessageTemplate, WithSubjectTemplate, LetterDVLATemplate from celery.exceptions import Retry +from botocore.exceptions import ClientError +from notifications_utils.template import SMSMessageTemplate, WithSubjectTemplate, LetterDVLATemplate from app import (encryption, DATETIME_FORMAT) from app.celery import provider_tasks @@ -1023,9 +1025,7 @@ def test_build_dvla_file(sample_letter_template, mocker): create_notification(template=job.template, job=job) mocked_upload = mocker.patch("app.celery.tasks.s3upload") mocked_send_task = mocker.patch("app.celery.tasks.notify_celery.send_task") - mocked_letter_template = mocker.patch("app.celery.tasks.LetterDVLATemplate") - mocked_letter_template_instance = mocked_letter_template.return_value - mocked_letter_template_instance.__str__.return_value = "dvla|string" + mocker.patch("app.celery.tasks.LetterDVLATemplate", return_value='dvla|string') build_dvla_file(job.id) mocked_upload.assert_called_once_with( @@ -1048,14 +1048,25 @@ def test_build_dvla_file_retries_if_all_notifications_are_not_created(sample_let build_dvla_file(job.id) mocked.assert_not_called() - tasks.build_dvla_file.retry.assert_called_with( - queue="retry-tasks", - exc=ANY - ) + tasks.build_dvla_file.retry.assert_called_with(queue="retry-tasks") assert Job.query.get(job.id).job_status == 'in progress' mocked_send_task.assert_not_called() +def test_build_dvla_file_retries_if_s3_err(sample_letter_template, mocker): + job = create_job(sample_letter_template, notification_count=1) + create_notification(job.template, job=job) + + mocker.patch('app.celery.tasks.LetterDVLATemplate', return_value='dvla|string') + mocker.patch('app.celery.tasks.s3upload', side_effect=ClientError({}, 'operation_name')) + retry_mock = mocker.patch('app.celery.tasks.build_dvla_file.retry', side_effect=Retry) + + with pytest.raises(Retry): + build_dvla_file(job.id) + + retry_mock.assert_called_once_with(queue='retry-tasks') + + def test_create_dvla_file_contents(notify_db_session, mocker): service = create_service(service_permissions=SERVICE_PERMISSION_TYPES) create_letter_contact(service=service, contact_block='London,\nNW1A 1AA')