From b5a33ded98466959101eaa4df19155c7401caab9 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 12 Jan 2021 12:22:13 +0000 Subject: [PATCH] Retry with failover lambda for FunctionError and status > 299 For all FunctionErrors, and for invoke errors (status > 299) we want to retry with failover lambda. We are doing this, because if there is a connection or other error with one lambda, the failover lambda may still work and it's worth trying. With time, we will probably have more complex retry flow, depending on the error and even maybe differing for each MNO (broadcast provider). --- app/clients/cbc_proxy.py | 28 ++-- tests/app/clients/test_cbc_proxy.py | 243 +++++++++++++++++----------- 2 files changed, 170 insertions(+), 101 deletions(-) diff --git a/app/clients/cbc_proxy.py b/app/clients/cbc_proxy.py index 50aae56a7..241bb3f48 100644 --- a/app/clients/cbc_proxy.py +++ b/app/clients/cbc_proxy.py @@ -112,12 +112,10 @@ class CBCProxyClientBase(ABC): payload_bytes = bytes(json.dumps(payload), encoding='utf8') result = self._invoke_lambda(self.lambda_name, payload_bytes) - if 'FunctionError' in result: - if result['Payload']['errorType'] == "CBCNewConnectionError": - current_app.logger.info(f"Got CBCNewConnectionError for {self.lambda_name}, calling failover lambda") - result = self._invoke_lambda(self.failover_lambda_name, payload_bytes) - else: - raise CBCProxyException('Function exited with unhandled exception') + if not result: + failover_result = self._invoke_lambda(self.failover_lambda_name, payload_bytes) + if not failover_result: + raise CBCProxyException(f'Lambda failed for both {self.lambda_name} and {self.failover_lambda_name}') return result @@ -128,12 +126,24 @@ class CBCProxyClientBase(ABC): InvocationType='RequestResponse', Payload=payload_bytes, ) + current_app.logger.info(f"Finished calling lambda {lambda_name}") if result['StatusCode'] > 299: - raise CBCProxyException('Could not invoke lambda') + current_app.logger.info( + f"Error calling lambda {self.lambda_name} with status code { result['StatusCode']}, {result.get('Payload')}" + ) + success = False - current_app.logger.info(f"Finished calling lambda {lambda_name}") - return result + elif 'FunctionError' in result: + current_app.logger.info( + f"Error calling lambda {self.lambda_name} with function error { result['Payload'] }" + ) + success = False + + else: + success = True + + return success def infer_language_from(self, content): if non_gsm_characters(content): diff --git a/tests/app/clients/test_cbc_proxy.py b/tests/app/clients/test_cbc_proxy.py index 047ffc6d9..ed5b5a3ca 100644 --- a/tests/app/clients/test_cbc_proxy.py +++ b/tests/app/clients/test_cbc_proxy.py @@ -127,60 +127,6 @@ def test_cbc_proxy_ee_create_and_send_invokes_function( assert payload['language'] == expected_language - -@pytest.mark.parametrize('cbc', ['bt-ee', 'vodafone']) -def test_cbc_proxy_will_failover_to_second_lambda_if_connection_error( - mocker, - cbc_proxy_ee, - cbc_proxy_vodafone, - cbc -): - cbc_proxy = cbc_proxy_ee if cbc == 'bt-ee' else cbc_proxy_vodafone - - ld_client_mock = mocker.patch.object( - cbc_proxy, - '_lambda_client', - create=True, - ) - - ld_client_mock.invoke.side_effect = [ - { - 'StatusCode': 200, - 'FunctionError': 'Handled', - 'Payload': { - "errorMessage": "", - "errorType": "CBCNewConnectionError" - } - }, - { - 'StatusCode': 200 - } - ] - - cbc_proxy.create_and_send_broadcast( - identifier='my-identifier', - message_number='0000007b', - headline='my-headline', - description='test-description', - areas=EXAMPLE_AREAS, - sent='a-passed-through-sent-value', - expires='a-passed-through-expires-value', - ) - - assert ld_client_mock.invoke.call_args_list == [ - call( - FunctionName=f'{cbc}-1-proxy', - InvocationType='RequestResponse', - Payload=mocker.ANY, - ), - call( - FunctionName=f'{cbc}-2-proxy', - InvocationType='RequestResponse', - Payload=mocker.ANY, - ) - ] - - def test_cbc_proxy_ee_cancel_invokes_function(mocker, cbc_proxy_ee): identifier = 'my-identifier' MockProviderMessage = namedtuple( @@ -353,16 +299,115 @@ def test_cbc_proxy_vodafone_cancel_invokes_function(mocker, cbc_proxy_vodafone): assert payload['sent'] == sent -def test_cbc_proxy_create_and_send_handles_invoke_error(mocker, cbc_proxy_ee): - identifier = 'my-identifier' - headline = 'my-headline' - description = 'my-description' - - sent = 'a-passed-through-sent-value' - expires = 'a-passed-through-expires-value' +@pytest.mark.parametrize('cbc', ['bt-ee', 'vodafone']) +def test_cbc_proxy_will_failover_to_second_lambda_if_function_error( + mocker, + cbc_proxy_ee, + cbc_proxy_vodafone, + cbc +): + cbc_proxy = cbc_proxy_ee if cbc == 'bt-ee' else cbc_proxy_vodafone ld_client_mock = mocker.patch.object( - cbc_proxy_ee, + cbc_proxy, + '_lambda_client', + create=True, + ) + + ld_client_mock.invoke.side_effect = [ + { + 'StatusCode': 200, + 'FunctionError': 'Handled', + 'Payload': { + "errorMessage": "", + "errorType": "CBCNewConnectionError" + } + }, + { + 'StatusCode': 200 + } + ] + + cbc_proxy.create_and_send_broadcast( + identifier='my-identifier', + message_number='0000007b', + headline='my-headline', + description='test-description', + areas=EXAMPLE_AREAS, + sent='a-passed-through-sent-value', + expires='a-passed-through-expires-value', + ) + + assert ld_client_mock.invoke.call_args_list == [ + call( + FunctionName=f'{cbc}-1-proxy', + InvocationType='RequestResponse', + Payload=mocker.ANY, + ), + call( + FunctionName=f'{cbc}-2-proxy', + InvocationType='RequestResponse', + Payload=mocker.ANY, + ) + ] + + +@pytest.mark.parametrize('cbc', ['bt-ee', 'vodafone']) +def test_cbc_proxy_will_failover_to_second_lambda_if_invoke_error( + mocker, + cbc_proxy_ee, + cbc_proxy_vodafone, + cbc +): + cbc_proxy = cbc_proxy_ee if cbc == 'bt-ee' else cbc_proxy_vodafone + + ld_client_mock = mocker.patch.object( + cbc_proxy, + '_lambda_client', + create=True, + ) + + ld_client_mock.invoke.side_effect = [ + { + 'StatusCode': 400 + }, + { + 'StatusCode': 200 + } + ] + + cbc_proxy.create_and_send_broadcast( + identifier='my-identifier', + message_number='0000007b', + headline='my-headline', + description='test-description', + areas=EXAMPLE_AREAS, + sent='a-passed-through-sent-value', + expires='a-passed-through-expires-value', + ) + + assert ld_client_mock.invoke.call_args_list == [ + call( + FunctionName=f'{cbc}-1-proxy', + InvocationType='RequestResponse', + Payload=mocker.ANY, + ), + call( + FunctionName=f'{cbc}-2-proxy', + InvocationType='RequestResponse', + Payload=mocker.ANY, + ) + ] + + +@pytest.mark.parametrize('cbc', ['bt-ee', 'vodafone']) +def test_cbc_proxy_create_and_send_tries_failover_lambda_on_invoke_error_and_raises_if_both_invoke_error( + mocker, cbc_proxy_ee, cbc_proxy_vodafone, cbc +): + cbc_proxy = cbc_proxy_ee if cbc == 'bt-ee' else cbc_proxy_vodafone + + ld_client_mock = mocker.patch.object( + cbc_proxy, '_lambda_client', create=True, ) @@ -372,34 +417,40 @@ def test_cbc_proxy_create_and_send_handles_invoke_error(mocker, cbc_proxy_ee): } with pytest.raises(CBCProxyException) as e: - cbc_proxy_ee.create_and_send_broadcast( - identifier=identifier, + cbc_proxy.create_and_send_broadcast( + identifier='my-identifier', message_number='0000007b', - headline=headline, - description=description, + headline='my-headline', + description='my-description', areas=EXAMPLE_AREAS, - sent=sent, expires=expires, + sent='a-passed-through-sent-value', + expires='a-passed-through-expires-value', ) - assert e.match('Could not invoke lambda') + assert e.match(f'Lambda failed for both {cbc}-1-proxy and {cbc}-2-proxy') - ld_client_mock.invoke.assert_called_once_with( - FunctionName='bt-ee-1-proxy', - InvocationType='RequestResponse', - Payload=mocker.ANY, - ) + assert ld_client_mock.invoke.call_args_list == [ + call( + FunctionName=f'{cbc}-1-proxy', + InvocationType='RequestResponse', + Payload=mocker.ANY, + ), + call( + FunctionName=f'{cbc}-2-proxy', + InvocationType='RequestResponse', + Payload=mocker.ANY, + ) + ] -def test_cbc_proxy_create_and_send_handles_function_error(mocker, cbc_proxy_ee): - identifier = 'my-identifier' - headline = 'my-headline' - description = 'my-description' - - sent = 'a-passed-through-sent-value' - expires = 'a-passed-through-expires-value' +@pytest.mark.parametrize('cbc', ['bt-ee', 'vodafone']) +def test_cbc_proxy_create_and_send_tries_failover_lambda_on_function_error_and_raises_if_both_function_error( + mocker, cbc_proxy_ee, cbc_proxy_vodafone, cbc +): + cbc_proxy = cbc_proxy_ee if cbc == 'bt-ee' else cbc_proxy_vodafone ld_client_mock = mocker.patch.object( - cbc_proxy_ee, + cbc_proxy, '_lambda_client', create=True, ) @@ -414,22 +465,30 @@ def test_cbc_proxy_create_and_send_handles_function_error(mocker, cbc_proxy_ee): } with pytest.raises(CBCProxyException) as e: - cbc_proxy_ee.create_and_send_broadcast( - identifier=identifier, + cbc_proxy.create_and_send_broadcast( + identifier='my-identifier', message_number='0000007b', - headline=headline, - description=description, + headline='my-headline', + description='my-description', areas=EXAMPLE_AREAS, - sent=sent, expires=expires, + sent='a-passed-through-sent-value', + expires='a-passed-through-expires-value', ) - assert e.match('Function exited with unhandled exception') + assert e.match(f'Lambda failed for both {cbc}-1-proxy and {cbc}-2-proxy') - ld_client_mock.invoke.assert_called_once_with( - FunctionName='bt-ee-1-proxy', - InvocationType='RequestResponse', - Payload=mocker.ANY, - ) + assert ld_client_mock.invoke.call_args_list == [ + call( + FunctionName=f'{cbc}-1-proxy', + InvocationType='RequestResponse', + Payload=mocker.ANY, + ), + call( + FunctionName=f'{cbc}-2-proxy', + InvocationType='RequestResponse', + Payload=mocker.ANY, + ) + ] def test_cbc_proxy_send_canary_invokes_function(mocker, cbc_proxy_client):