From 5d3a3ab042de564b9ba18a88dd0cbb57602eda1c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 14 Dec 2017 13:26:38 +0000 Subject: [PATCH 1/6] Can't have spaces between queue names --- manifest-delivery-base.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/manifest-delivery-base.yml b/manifest-delivery-base.yml index 71dc5bcc9..8838ad40b 100644 --- a/manifest-delivery-base.yml +++ b/manifest-delivery-base.yml @@ -56,6 +56,6 @@ applications: NOTIFY_APP_NAME: delivery-worker - name: notify-delivery-worker-receipts - command: scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q ses-callbacks, service-callbacks + command: scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q ses-callbacks,service-callbacks env: NOTIFY_APP_NAME: delivery-worker-receipts From c6e6fad01fb5ed4caa65cca82a7caa0422babbb2 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 14 Dec 2017 14:23:32 +0000 Subject: [PATCH 2/6] if apps crash on startup, then fail deploy process we saw an issue where the app started, then immediately crashed due to a setup error. However, jenkins had already returned positively, and the deploy continued. cf-deploy should fail if the app doesn't start up. We do this by looking through the cloudfoundry events, and aborting if there are any `app.crash` events for the new GUID. --- Makefile | 3 +++ scripts/run_app_paas.sh | 2 -- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index b4a03d2eb..d9fe648df 100644 --- a/Makefile +++ b/Makefile @@ -281,6 +281,9 @@ cf-deploy: ## Deploys the app to Cloud Foundry # sleep for 10 seconds to try and make sure that all worker threads (either web api or celery) have finished before we delete # when we delete the DB is unbound from the app, which can cause "permission denied for relation" psycopg2 errors. sleep 10 + + # get the new GUID, and find all crash events for that. If there were any crashes we will abort the deploy. + [ $(cf curl "/v2/events?q=type:app.crash&q=actee:$$(cf app --guid notify-delivery-worker-receipts)" | jq ".total_results") -eq 0 ] cf delete -f ${CF_APP}-rollback .PHONY: cf-deploy-api-db-migration diff --git a/scripts/run_app_paas.sh b/scripts/run_app_paas.sh index 78610c1bd..2a4d2988d 100755 --- a/scripts/run_app_paas.sh +++ b/scripts/run_app_paas.sh @@ -55,8 +55,6 @@ function on_exit { break fi done - echo "Application process terminated, waiting 10 seconds" - sleep 10 echo "Terminating remaining subprocesses.." kill 0 } From f23074596b5229646096cc5c34f21012a6b6ce06 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 13 Dec 2017 15:52:38 +0000 Subject: [PATCH 3/6] Update billable units for letters pdf task --- app/celery/letters_pdf_tasks.py | 33 +++++++++++-- tests/app/celery/test_letters_pdf_tasks.py | 55 +++++++++++++++++++++- 2 files changed, 83 insertions(+), 5 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 24c7347ae..a7b565139 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -1,4 +1,6 @@ +from datetime import datetime from flask import current_app +import math from requests import ( post as requests_post, RequestException @@ -9,7 +11,11 @@ from botocore.exceptions import ClientError as BotoClientError from app import notify_celery from app.aws import s3 from app.config import QueueNames -from app.dao.notifications_dao import get_notification_by_id, update_notification_status_by_id +from app.dao.notifications_dao import ( + get_notification_by_id, + update_notification_status_by_id, + dao_update_notifications_by_reference +) from app.statsd_decorators import statsd @@ -19,7 +25,7 @@ def create_letters_pdf(self, notification_id): try: notification = get_notification_by_id(notification_id, _raise=True) - pdf_data = get_letters_pdf( + pdf_data, billable_units = get_letters_pdf( notification.template, contact_block=notification.reply_to_text, org_id=notification.service.dvla_organisation.id, @@ -28,6 +34,24 @@ def create_letters_pdf(self, notification_id): current_app.logger.info("PDF Letter {} reference {} created at {}, {} bytes".format( notification.id, notification.reference, notification.created_at, len(pdf_data))) s3.upload_letters_pdf(reference=notification.reference, crown=notification.service.crown, filedata=pdf_data) + + updated_count = dao_update_notifications_by_reference( + references=[notification.reference], + update_dict={ + "billable_units": billable_units, + "updated_at": datetime.utcnow() + } + ) + + if not updated_count: + msg = "Update letter notification billing units failed: notification not found with reference {}".format( + notification.reference) + current_app.logger.error(msg) + else: + current_app.logger.info( + 'Letter notification reference {reference}: billable units set to {billable_units}'.format( + reference=str(notification.reference), billable_units=billable_units)) + except (RequestException, BotoClientError): try: current_app.logger.exception( @@ -62,4 +86,7 @@ def get_letters_pdf(template, contact_block, org_id, values): ) resp.raise_for_status() - return resp.content + pages_per_sheet = 2 + billable_units = math.ceil(int(resp.headers.get("X-pdf-page-count", 0)) / pages_per_sheet) + + return resp.content, billable_units diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 0af5c78ab..c540a2388 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -10,6 +10,7 @@ from app.celery.letters_pdf_tasks import ( create_letters_pdf, get_letters_pdf, ) +from app.models import Notification from tests.conftest import set_config_values @@ -46,8 +47,36 @@ def test_get_letters_pdf_calls_notifications_template_preview_service_correctly( } +@pytest.mark.parametrize('page_count,expected_billable_units', [ + ('1', 1), + ('2', 1), + ('3', 2) +]) +def test_get_letters_pdf_calculates_billing_units( + notify_api, mocker, client, sample_letter_template, page_count, expected_billable_units): + contact_block = 'Mr Foo,\n1 Test Street,\nLondon\nN1' + dvla_org_id = '002' + + with set_config_values(notify_api, { + 'TEMPLATE_PREVIEW_API_HOST': 'http://localhost/notifications-template-preview', + 'TEMPLATE_PREVIEW_API_KEY': 'test-key' + }): + with requests_mock.Mocker() as request_mock: + request_mock.post( + 'http://localhost/notifications-template-preview/print.pdf', + content=b'\x00\x01', + headers={'X-pdf-page-count': page_count}, + status_code=200 + ) + + _, billable_units = get_letters_pdf( + sample_letter_template, contact_block=contact_block, org_id=dvla_org_id, values=None) + + assert billable_units == expected_billable_units + + def test_create_letters_pdf_calls_upload_letters_pdf(mocker, sample_letter_notification): - mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=b'\x00\x01') + mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', '1')) mock_s3 = mocker.patch('app.celery.tasks.s3.upload_letters_pdf') create_letters_pdf(sample_letter_notification.id) @@ -59,6 +88,28 @@ def test_create_letters_pdf_calls_upload_letters_pdf(mocker, sample_letter_notif ) +def test_create_letters_pdf_sets_billable_units(mocker, sample_letter_notification): + mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', 1)) + mocker.patch('app.celery.tasks.s3.upload_letters_pdf') + + create_letters_pdf(sample_letter_notification.id) + noti = Notification.query.filter(Notification.reference == sample_letter_notification.reference).one() + assert noti.billable_units == 1 + + +def test_create_letters_pdf_handles_update_failure(mocker, sample_letter_notification): + mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', 1)) + mocker.patch('app.celery.tasks.s3.upload_letters_pdf') + mocker.patch('app.celery.tasks.letters_pdf_tasks.dao_update_notifications_by_reference', return_value=0) + mock_error_logger = mocker.patch('app.celery.letters_pdf_tasks.current_app.logger.error') + + create_letters_pdf(sample_letter_notification.id) + + mock_error_logger.assert_called_once_with( + "Update letter notification billing units failed: notification not found with reference {}".format( + sample_letter_notification.reference)) + + def test_create_letters_pdf_non_existent_notification(notify_api, mocker, fake_uuid): with pytest.raises(expected_exception=NoResultFound): create_letters_pdf(fake_uuid) @@ -75,7 +126,7 @@ def test_create_letters_pdf_handles_request_errors(mocker, sample_letter_notific def test_create_letters_pdf_handles_s3_errors(mocker, sample_letter_notification): - mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf') + mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', 1)) mock_s3 = mocker.patch('app.celery.tasks.s3.upload_letters_pdf', side_effect=ClientError({}, 'operation_name')) mock_retry = mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.retry') From 0045cd6b72149a3cff620fb016281a035eef981f Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 14 Dec 2017 11:38:17 +0000 Subject: [PATCH 4/6] Use `dao_update_notification` to update `billable_units` for letter notifications --- app/celery/letters_pdf_tasks.py | 23 ++++++---------------- tests/app/celery/test_letters_pdf_tasks.py | 13 ------------ 2 files changed, 6 insertions(+), 30 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index a7b565139..d70265791 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -1,4 +1,3 @@ -from datetime import datetime from flask import current_app import math from requests import ( @@ -14,7 +13,7 @@ from app.config import QueueNames from app.dao.notifications_dao import ( get_notification_by_id, update_notification_status_by_id, - dao_update_notifications_by_reference + dao_update_notification ) from app.statsd_decorators import statsd @@ -35,22 +34,12 @@ def create_letters_pdf(self, notification_id): notification.id, notification.reference, notification.created_at, len(pdf_data))) s3.upload_letters_pdf(reference=notification.reference, crown=notification.service.crown, filedata=pdf_data) - updated_count = dao_update_notifications_by_reference( - references=[notification.reference], - update_dict={ - "billable_units": billable_units, - "updated_at": datetime.utcnow() - } - ) + notification.billable_units = billable_units + dao_update_notification(notification) - if not updated_count: - msg = "Update letter notification billing units failed: notification not found with reference {}".format( - notification.reference) - current_app.logger.error(msg) - else: - current_app.logger.info( - 'Letter notification reference {reference}: billable units set to {billable_units}'.format( - reference=str(notification.reference), billable_units=billable_units)) + current_app.logger.info( + 'Letter notification reference {reference}: billable units set to {billable_units}'.format( + reference=str(notification.reference), billable_units=billable_units)) except (RequestException, BotoClientError): try: diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index c540a2388..a414fa2db 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -97,19 +97,6 @@ def test_create_letters_pdf_sets_billable_units(mocker, sample_letter_notificati assert noti.billable_units == 1 -def test_create_letters_pdf_handles_update_failure(mocker, sample_letter_notification): - mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', 1)) - mocker.patch('app.celery.tasks.s3.upload_letters_pdf') - mocker.patch('app.celery.tasks.letters_pdf_tasks.dao_update_notifications_by_reference', return_value=0) - mock_error_logger = mocker.patch('app.celery.letters_pdf_tasks.current_app.logger.error') - - create_letters_pdf(sample_letter_notification.id) - - mock_error_logger.assert_called_once_with( - "Update letter notification billing units failed: notification not found with reference {}".format( - sample_letter_notification.reference)) - - def test_create_letters_pdf_non_existent_notification(notify_api, mocker, fake_uuid): with pytest.raises(expected_exception=NoResultFound): create_letters_pdf(fake_uuid) From 7d1c4ea7225c5a02ea9f70f94c3634d0e3e99b4a Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 14 Dec 2017 14:23:32 +0000 Subject: [PATCH 5/6] fix makefile syntax --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index d9fe648df..ea6bb8c41 100644 --- a/Makefile +++ b/Makefile @@ -283,7 +283,7 @@ cf-deploy: ## Deploys the app to Cloud Foundry sleep 10 # get the new GUID, and find all crash events for that. If there were any crashes we will abort the deploy. - [ $(cf curl "/v2/events?q=type:app.crash&q=actee:$$(cf app --guid notify-delivery-worker-receipts)" | jq ".total_results") -eq 0 ] + [ $$(cf curl "/v2/events?q=type:app.crash&q=actee:$$(cf app --guid ${CF_APP})" | jq ".total_results") -eq 0 ] cf delete -f ${CF_APP}-rollback .PHONY: cf-deploy-api-db-migration From 0ad43f0c5b1601e8ab3d36e6e16f340585688b58 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 14 Dec 2017 16:00:51 +0000 Subject: [PATCH 6/6] Create letters pdf queue was renamed with tasks, but was lost in another merge - needs to be correct name otherwise the delivery worker will not pick up the queue --- app/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/config.py b/app/config.py index f2d37a769..0205262a0 100644 --- a/app/config.py +++ b/app/config.py @@ -30,7 +30,7 @@ class QueueNames(object): RETRY = 'retry-tasks' NOTIFY = 'notify-internal-tasks' PROCESS_FTP = 'process-ftp-tasks' - CREATE_LETTERS_PDF = 'create-letters-pdf' + CREATE_LETTERS_PDF = 'create-letters-pdf-tasks' CALLBACKS = 'service-callbacks' @staticmethod