Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add better errors for too large deployments #380

Merged
merged 8 commits into from
Jun 21, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 72 additions & 6 deletions chalice/awsclient.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@
import json

import botocore.session # noqa
from botocore.exceptions import ClientError
from botocore.vendored.requests import ConnectionError as \
RequestsConnectionError
from typing import Any, Optional, Dict, Callable, List, Iterator # noqa

from chalice.constants import DEFAULT_STAGE_NAME
Expand All @@ -33,16 +36,29 @@
_CLIENT_METHOD = Callable[..., Dict[str, Any]]


def _get_mb(value):
# type: (int) -> str
return '%.1f MB' % (float(value) / (1024 ** 2))


class ResourceDoesNotExistError(Exception):
pass


class DeploymentPackageTooLargeError(Exception):
def __init__(self, msg, additional_details=None):
# type: (str, str) -> None
self.additional_details = additional_details
super(DeploymentPackageTooLargeError, self).__init__(msg)


class TypedAWSClient(object):

# 30 * 5 == 150 seconds or 2.5 minutes for the initial lambda
# creation + role propagation.
LAMBDA_CREATE_ATTEMPTS = 30
DELAY_TIME = 5
MAX_LAMBDA_DEPLOYMENT_SIZE = 50 * (1024 ** 2)

def __init__(self, session, sleep=time.sleep):
# type: (botocore.session.Session, Callable[[int], None]) -> None
Expand Down Expand Up @@ -92,8 +108,12 @@ def create_function(self,
kwargs['Timeout'] = timeout
if memory_size is not None:
kwargs['MemorySize'] = memory_size
return self._call_client_method_with_retries(
self._client('lambda').create_function, kwargs)['FunctionArn']
try:
return self._call_client_method_with_retries(
self._client('lambda').create_function, kwargs)['FunctionArn']
except Exception as e:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't think this is right. We're catching all exceptions and casting them down into a more specific exception (LambdaClientError). If there is actually a bug or something exceptional that happens in the code I'd want the original stack trace to propagate up, not be masked by LambdaClientError.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Offline: Decided to just catch botocore.exceptions.ClientError and RequestsConnectionError for this.

self._raise_large_deployment_related_errors(e, len(zip_contents))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider creating some lambda specific error so pieces higher up the stack know that we failed in the lambda function.

raise e

def _call_client_method_with_retries(self, method, kwargs):
# type: (_CLIENT_METHOD, Dict[str, Any]) -> Dict[str, Any]
Expand All @@ -102,18 +122,60 @@ def _call_client_method_with_retries(self, method, kwargs):
while True:
try:
response = method(**kwargs)
except client.exceptions.InvalidParameterValueException:
except client.exceptions.InvalidParameterValueException as e:
# We're assuming that if we receive an
# InvalidParameterValueException, it's because
# the role we just created can't be used by
# Lambda so retry until it can be.
self._sleep(self.DELAY_TIME)
attempts += 1
if attempts >= self.LAMBDA_CREATE_ATTEMPTS:
message = e.response['Error'].get('Message', '')
retryable_message = (
'The role defined for the function cannot be assumed '
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to relax the matching here. Either partial string match or regex matching

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am planning to just check to see if the string 'role' is included in the error message if that is fine?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer something more robust than that. If the function name can ever be in the error message and that name contains role you'll get a false positive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the literal error message, you will get for both create and update is:

The role defined for the function cannot be assumed by Lambda.

It does not include the name of the function currently. I can check for 'role' and 'assumed by Lambda'? Or just 'assumed by Lambda'?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I mean in general for InvalidParameterValueException, which is the code block this is in. This a) applies to more than just the role not being able to be assumed and b) has no guarantees about the message changing in the future. Something like role.*cannot be assumed seems reasonable to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright I can do that.

'by Lambda.'
)
if attempts >= self.LAMBDA_CREATE_ATTEMPTS or \
retryable_message not in message:
raise
continue
return response

def _raise_large_deployment_related_errors(self, error,
deployment_size):
# type: (Any, int) -> None
error_message = str(error)
should_raise = False
additional_details = None
if (isinstance(error, RequestsConnectionError) and
deployment_size > self.MAX_LAMBDA_DEPLOYMENT_SIZE):
# When the zip deployment package is too large and Lambda
# aborts the connection as chalice is still sending it
# data
additional_details = (
'This is likely because the deployment package is %s. '
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets move this up the stack and have stuff in awsclient just collect all the raw data, deployment package size, original exceptions, etc. We'd then have the Exception object assemble the suggestions and pretty print the error messages to the user.

'Lambda only allows deployment packages that are %s or '
'less in size.' % (
_get_mb(deployment_size),
_get_mb(self.MAX_LAMBDA_DEPLOYMENT_SIZE)
)
)
should_raise = True
elif isinstance(error, ClientError):
code = error.response['Error'].get('Code')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These will default to None which will crash in your __contains__ call on L169. These should default to str types, probably just the empty string.

message = error.response['Error'].get('Message')
if code == 'RequestEntityTooLargeException':
# Happens when the zipped deployment package sent to lambda
# is too large
should_raise = True
elif code == 'InvalidParameterValueException' and \
'Unzipped size must be smaller' in message:
# Happens when the contents of the unzipped deployment
# package sent to lambda is too large
should_raise = True
if should_raise:
raise DeploymentPackageTooLargeError(
error_message, additional_details)

def delete_function(self, function_name):
# type: (str) -> None
lambda_client = self._client('lambda')
Expand All @@ -140,8 +202,12 @@ def update_function(self,
the targeted lambda function.
"""
lambda_client = self._client('lambda')
return_value = lambda_client.update_function_code(
FunctionName=function_name, ZipFile=zip_contents)
try:
return_value = lambda_client.update_function_code(
FunctionName=function_name, ZipFile=zip_contents)
except Exception as e:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing, we should really only have except Exceptions and the top most layer of the code. Restrict this to the subset of exceptions we care about.

Copy link
Contributor Author

@kyleknap kyleknap Jun 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Offline: Catch RequestsConnectionError and botocore.exceptions.ClientError here instead.

self._raise_large_deployment_related_errors(e, len(zip_contents))
raise e

kwargs = {} # type: Dict[str, Any]
if environment_variables is not None:
Expand Down
38 changes: 38 additions & 0 deletions chalice/deploy/deployer.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
from chalice import __version__ as chalice_version
from chalice import policy
from chalice.awsclient import TypedAWSClient, ResourceDoesNotExistError
from chalice.awsclient import DeploymentPackageTooLargeError
from chalice.config import Config, DeployedResources # noqa
from chalice.deploy.packager import LambdaDeploymentPackager
from chalice.deploy.swagger import SwaggerGenerator
Expand All @@ -29,6 +30,7 @@

NULLARY = Callable[[], str]
OPT_RESOURCES = Optional[DeployedResources]
OPT_STR = Optional[str]


def create_default_deployer(session, prompter=None):
Expand Down Expand Up @@ -169,6 +171,19 @@ def _validate_manage_iam_role(config):
)


class ChaliceDeploymentError(Exception):
def __init__(self, error_msg, where=None, suggestion=None):
# type: (str, OPT_STR, OPT_STR) -> None
msg = 'DEPLOYMENT ERROR - '
if where:
msg += 'While %s, received the following error: ' % where
msg += error_msg
if suggestion:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if suggestion is not None

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should not matter if the check changes here. The output will stay the same, but sure I can update.

msg += '. '
msg += suggestion
super(ChaliceDeploymentError, self).__init__(msg)


class NoPrompt(object):
def confirm(self, text, default=False, abort=False):
# type: (str, bool, bool) -> bool
Expand Down Expand Up @@ -206,6 +221,13 @@ def deploy(self, config, chalice_stage_name=DEFAULT_STAGE_NAME):
rest api, lambda function, role, etc.

"""
try:
return self._do_deploy(config, chalice_stage_name)
except Exception as error:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing about except Exception. This would be fine in say cli/__init__.py, but not in the deployer module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Offline: Catch LambdaClientError and botocore.exceptions.ClientError here instead.

raise self._get_chalice_deployment_error(error)

def _do_deploy(self, config, chalice_stage_name=DEFAULT_STAGE_NAME):
# type: (Config, str) -> Dict[str, Any]
validate_configuration(config)
existing_resources = config.deployed_resources(chalice_stage_name)
deployed_values = self._lambda_deploy.deploy(
Expand All @@ -229,6 +251,22 @@ def deploy(self, config, chalice_stage_name=DEFAULT_STAGE_NAME):
chalice_stage_name: deployed_values
}

def _get_chalice_deployment_error(self, error):
# type: (Any) -> ChaliceDeploymentError
suggestion = None
where = None
if isinstance(error, DeploymentPackageTooLargeError):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move this out into the ChaliceDeploymentError class. We should want the deployer to only be about deploying resources and move the logic of helpful error messages out to a separate class.

where = 'sending your chalice handler code to Lambda'
suggestion = (
'To avoid this error, decrease the size of your chalice '
'application by removing code or removing '
'dependencies from your chalice application.'
)
if error.additional_details:
suggestion = error.additional_details + ' ' + suggestion
return ChaliceDeploymentError(
str(error), where=where, suggestion=suggestion)


class LambdaDeployer(object):
def __init__(self,
Expand Down
10 changes: 10 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import botocore.session
from botocore.stub import Stubber
import mock
import pytest
from pytest import fixture

Expand Down Expand Up @@ -98,6 +99,15 @@ def stubbed_session():
return s


@fixture
def mock_lambda_client():
session = botocore.session.Session()
lambda_client = session.create_client('lambda')
mock_client = mock.Mock(lambda_client)
mock_client.exceptions = lambda_client.exceptions
return mock_client


@fixture
def no_local_config(monkeypatch):
"""Ensure no local AWS configuration is used.
Expand Down
Loading