From 2a392e7137b2db2f08f78450d970bc2f92f1efad Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 13 Nov 2019 17:01:03 +0000 Subject: [PATCH] update switch provider scheduled task it now looks at both providers and works out whether to deprioritise one, rather than binary switching from one to the other. If anything has altered the priorities in the last ten minutes it won't take any action. If both providers are slow it also won't take any action. --- app/celery/scheduled_tasks.py | 37 +++++++------- tests/app/celery/test_scheduled_tasks.py | 64 +++++++++++++++++++++--- 2 files changed, 76 insertions(+), 25 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 0d069ea63..b67d3e61b 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -25,19 +25,17 @@ from app.dao.jobs_dao import ( ) from app.dao.jobs_dao import dao_update_job from app.dao.notifications_dao import ( - is_delivery_slow_for_provider, dao_get_scheduled_notifications, set_scheduled_notification_to_processed, notifications_not_yet_sent, dao_precompiled_letters_still_pending_virus_check, dao_old_letters_with_created_status, - letters_missing_from_sending_bucket -) -from app.dao.provider_details_dao import ( - get_current_provider, - dao_reduce_sms_provider_priority + letters_missing_from_sending_bucket, + is_delivery_slow_for_providers, ) +from app.dao.provider_details_dao import dao_reduce_sms_provider_priority from app.dao.users_dao import delete_codes_older_created_more_than_a_day_ago +from app.dao.provider_details_dao import get_provider_details_by_notification_type from app.models import ( Job, JOB_STATUS_IN_PROGRESS, @@ -109,27 +107,30 @@ def delete_invitations(): @statsd(namespace="tasks") def switch_current_sms_provider_on_slow_delivery(): """ - Switch providers if at least 30% of notifications took more than four minutes to be delivered - in the last ten minutes. Search from the time we last switched to the current provider. + Reduce provider's priority if at least 30% of notifications took more than four minutes to be delivered + in the last ten minutes. If both providers are slow, don't do anything. If we changed the providers in the + last ten minutes, then don't update them again either. """ - current_provider = get_current_provider('sms') - if current_provider.updated_at > datetime.utcnow() - timedelta(minutes=10): + providers = get_provider_details_by_notification_type('sms') + # if something updated recently, don't update again. If the updated_at is null, set it to min time + # just to prevent errors + if any((provider.updated_at or datetime.min) > datetime.utcnow() - timedelta(minutes=10) for provider in providers): current_app.logger.info("Slow delivery notifications provider switched less than 10 minutes ago.") return + slow_delivery_notifications = is_delivery_slow_for_providers( threshold=0.3, created_at=datetime.utcnow() - timedelta(minutes=10), delivery_time=timedelta(minutes=4), ) - if slow_delivery_notifications[current_provider]: - current_app.logger.warning( - 'Slow delivery notifications detected for provider {}'.format( - current_provider.identifier - ) - ) - - dao_reduce_sms_provider_priority(current_provider.identifier) + # only adjust if some values are true and some are false - ie, don't adjust if all providers are fast or + # all providers are slow + if len(set(slow_delivery_notifications.values())) != 1: + for provider_name, is_slow in slow_delivery_notifications.items(): + if is_slow: + current_app.logger.warning('Slow delivery notifications detected for provider {}'.format(provider_name)) + dao_reduce_sms_provider_priority(provider_name) @notify_celery.task(name='check-job-status') diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 0c35c32fe..5fa11eebb 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -15,11 +15,13 @@ from app.celery.scheduled_tasks import ( replay_created_notifications, check_precompiled_letter_state, check_templated_letter_state, - check_for_missing_rows_in_completed_jobs + check_for_missing_rows_in_completed_jobs, + switch_current_sms_provider_on_slow_delivery, ) from app.config import QueueNames, TaskNames from app.dao.jobs_dao import dao_get_job_by_id from app.dao.notifications_dao import dao_get_scheduled_notifications +from app.dao.provider_details_dao import get_provider_details_by_notification_type, get_provider_details_by_identifier from app.models import ( JOB_STATUS_IN_PROGRESS, JOB_STATUS_ERROR, @@ -98,13 +100,61 @@ def test_should_update_all_scheduled_jobs_and_put_on_queue(sample_template, mock ]) -def test_switch_providers_on_slow_delivery_switches_once_then_does_not_switch_if_already_switched( - notify_api, - mocker, - sample_user, - sample_template +@freeze_time('2017-05-01 14:00:00') +def test_switch_current_sms_provider_on_slow_delivery_switches_when_one_provider_is_slow( + mocker, + restore_provider_details, ): - raise NotImplementedError # TODO + is_slow_dict = {'mmg': False, 'firetext': True} + mock_is_slow = mocker.patch('app.celery.scheduled_tasks.is_delivery_slow_for_providers', return_value=is_slow_dict) + mock_reduce = mocker.patch('app.celery.scheduled_tasks.dao_reduce_sms_provider_priority') + # updated_at times are older than the 10 minute window + get_provider_details_by_identifier('mmg').updated_at = datetime(2017, 5, 1, 13, 49) + get_provider_details_by_identifier('firetext').updated_at = None + + switch_current_sms_provider_on_slow_delivery() + + mock_is_slow.assert_called_once_with( + threshold=0.3, + created_at=datetime(2017, 5, 1, 13, 50), + delivery_time=timedelta(minutes=4) + ) + mock_reduce.assert_called_once_with('firetext') + + +@freeze_time('2017-05-01 14:00:00') +def test_switch_current_sms_provider_on_slow_delivery_does_nothing_if_recent_changes( + mocker, + restore_provider_details, +): + mock_is_slow = mocker.patch('app.celery.scheduled_tasks.is_delivery_slow_for_providers') + mock_reduce = mocker.patch('app.celery.scheduled_tasks.dao_reduce_sms_provider_priority') + get_provider_details_by_identifier('mmg').updated_at = datetime(2017, 5, 1, 13, 51) + + switch_current_sms_provider_on_slow_delivery() + + assert mock_is_slow.called is False + assert mock_reduce.called is False + + +@freeze_time('2017-05-01 14:00:00') +@pytest.mark.parametrize('is_slow_dict', [ + {'mmg': False, 'firetext': False}, + {'mmg': True, 'firetext': True}, +]) +def test_switch_current_sms_provider_on_slow_delivery_does_nothing_if_no_need( + mocker, + restore_provider_details, + is_slow_dict +): + mocker.patch('app.celery.scheduled_tasks.is_delivery_slow_for_providers', return_value=is_slow_dict) + mock_reduce = mocker.patch('app.celery.scheduled_tasks.dao_reduce_sms_provider_priority') + get_provider_details_by_identifier('mmg').updated_at = datetime(2017, 5, 1, 13, 51) + + + switch_current_sms_provider_on_slow_delivery() + + assert mock_reduce.called is False @freeze_time("2017-05-01 14:00:00")