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.

Sign up for GitHub

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

Jump to bottom

Container name feature implementation #774

Closed
wants to merge 14 commits into from
Closed

Container name feature implementation #774

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
d39a873
Add initial implementation of --container-name feature
ndobryanskyy Nov 16, 2018
604e5fe
Implement additional Error hadnling, while starting container
ndobryanskyy Nov 16, 2018
c5646b0
Fix flake and pylint comments
ndobryanskyy Nov 16, 2018
01f84d1
Fix tests
ndobryanskyy Nov 19, 2018
e45d92b
Merge pull request #1 from ndobryanskyy/feature/container-name
ndobryanskyy Nov 19, 2018
37d9f1f
Merge from develop
ndobryanskyy Nov 19, 2018
8409b45
Merge from develop
Nov 19, 2018
9cce3d8
Change is_docker_reachable to property
Nov 19, 2018
5536800
Merge branch 'develop' into develop
ndobryanskyy Nov 19, 2018
c0f1d51
Merge branch 'develop' of https://github.com/awslabs/aws-sam-cli into…
ndobryanskyy Nov 23, 2018
5981dcf
Merge branch 'develop' of https://github.com/ndobryanskyy/aws-sam-cli…
ndobryanskyy Nov 23, 2018
ae7aadf
Fix review comments
ndobryanskyy Nov 25, 2018
dce892d
Add functional and integration tests
ndobryanskyy Nov 27, 2018
56abfc4
Merge from upstream
ndobryanskyy Dec 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Expand Up @@ -20,7 +20,7 @@ flake:
flake8 tests/unit tests/integration

lint:
# Liner performs static analysis to catch latent bugs
# Linter performs static analysis to catch latent bugs
ndobryanskyy marked this conversation as resolved.
Show resolved Hide resolved
pylint --rcfile .pylintrc samcli

# Command to run everytime you make changes to verify everything works
Expand Down
60 changes: 32 additions & 28 deletions samcli/commands/local/cli_common/invoke_context.py
View file
Open in desktop
Expand Up @@ -6,9 +6,6 @@
import json
import os

import docker
import requests

import samcli.lib.utils.osutils as osutils
from samcli.commands.local.lib.local_lambda import LocalLambdaRunner
from samcli.commands.local.lib.debug_context import DebugContext
Expand Down Expand Up @@ -43,7 +40,7 @@ class InvokeContext(object):
This class sets up some resources that need to be cleaned up after the context object is used.
"""

def __init__(self,
def __init__(self, # pylint: disable=R0914
template_file,
function_identifier=None,
env_vars_file=None,
Expand All @@ -56,7 +53,8 @@ def __init__(self,
debug_args=None,
debugger_path=None,
aws_region=None,
parameter_overrides=None):
parameter_overrides=None,
container_name=None):
"""
Initialize the context

Expand Down Expand Up @@ -91,6 +89,8 @@ def __init__(self,
AWS region to use
parameter_overrides dict
Values for the template parameters
container_name str
Docker container name

"""
self._template_file = template_file
Expand All @@ -106,12 +106,14 @@ def __init__(self,
self._debug_args = debug_args
self._debugger_path = debugger_path
self._parameter_overrides = parameter_overrides or {}
self._container_name = container_name

self._template_dict = None
self._function_provider = None
self._env_vars_value = None
self._log_file_handle = None
self._debug_context = None
self._container_manager = None

def __enter__(self):
"""
Expand All @@ -131,7 +133,10 @@ def __enter__(self):
self._debug_args,
self._debugger_path)

self._check_docker_connectivity()
self._container_manager = self._get_container_manager(self._docker_network, self._skip_pull_image)

if not self._container_manager.is_docker_reachable:
raise InvokeContextException("Running AWS SAM projects locally requires Docker. Have you got it installed?")
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice touch!


return self

Expand Down Expand Up @@ -179,13 +184,11 @@ def local_lambda_runner(self):
locally
"""

container_manager = ContainerManager(docker_network_id=self._docker_network,
skip_pull_image=self._skip_pull_image)

lambda_runtime = LambdaRuntime(container_manager)
lambda_runtime = LambdaRuntime(self._container_manager)
ndobryanskyy marked this conversation as resolved.
Show resolved Hide resolved
return LocalLambdaRunner(local_runtime=lambda_runtime,
function_provider=self._function_provider,
cwd=self.get_cwd(),
container_name=self._container_name,
env_vars_values=self._env_vars_value,
debug_context=self._debug_context,
aws_profile=self._aws_profile,
Expand Down Expand Up @@ -300,6 +303,25 @@ def _setup_log_file(log_file):

return open(log_file, 'wb')

@staticmethod
def _get_container_manager(docker_network, skip_pull_image):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: How about _make_container_manager instead of _get_? Get creates the impression that you are getting an existing instance whereas this method actually creates a new instance

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've chosen that name, because of the surrounding code. It has _get_{something} pattern. Like _get_debug_context() which also creates a new instance

"""
Creates a ContainerManager

Parameters
----------
docker_network str
Docker network identifier
skip_pull_image bool
Should the manager skip pulling the image

Returns
-------
samcli.local.docker.manager.ContainerManager
Object representing Docker container manager
"""
return ContainerManager(docker_network_id=docker_network, skip_pull_image=skip_pull_image)

@staticmethod
def _get_debug_context(debug_port, debug_args, debugger_path):
"""
Expand Down Expand Up @@ -339,21 +361,3 @@ def _get_debug_context(debug_port, debug_args, debugger_path):
debugger_path = str(debugger)

return DebugContext(debug_port=debug_port, debug_args=debug_args, debugger_path=debugger_path)

@staticmethod
def _check_docker_connectivity(docker_client=None):
"""
Checks if Docker daemon is running. This is required for us to invoke the function locally

:param docker_client: Instance of Docker client
:return bool: True, if Docker is available
:raises InvokeContextException: If Docker is not available
"""

docker_client = docker_client or docker.from_env()

try:
docker_client.ping()
# When Docker is not installed, a request.exceptions.ConnectionError is thrown.
except (docker.errors.APIError, requests.exceptions.ConnectionError):
raise InvokeContextException("Running AWS SAM projects locally requires Docker. Have you got it installed?")
13 changes: 13 additions & 0 deletions samcli/commands/local/cli_common/options.py
View file
Open in desktop
Expand Up @@ -5,6 +5,8 @@
import click
from samcli.commands._utils.options import template_click_option, docker_click_options, parameter_override_click_option

from samcli.local.docker.manager import ContainerManager
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this import strange. I understand why but the Options shouldn't need to interact with a ContainerManger. Maybe we should place the is_valid_container_name somewhere else?

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 can go to utils or something, but for me, ContainerManager should be the only one responsible for the whole Docker management cycle. It will semantically be better, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

To me validation isn't part of that management cycle, it more of a attribute of the container itself not the class that manages.
@sanathkr or @thesriram what are your thoughts here. I just want to make sure we keep the right abstractions is all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jfuss, if we treat it like an attribute we can simply inline this method here. But for me, it is more like OOP, if we leave it in ContainerManager

Copy link
Contributor

Choose a reason for hiding this comment

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

I more meant on the Container class instead of the ContainerManger. This isn't a huge deal for me, just something I noticed while reading through.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am ok with this abstraction you have. I think your reasoning is good. I think we just have a slightly different model for what is in the Docker management cycle. I am more worried about the use of container_name outside of debugging than this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jfuss, okay, then 😅 As for the container_name I do agree, that it is more about debugging, then something else. I will get back to PC, consider options and get back to you really nice catch.

Also, as related question, what do you think about determining whether DebugContext is true with debug_port or debugger_path as debugging port does not make sense for don't core debugging and customer would be forced to supply unused argument to get it working

Copy link
Contributor

Choose a reason for hiding this comment

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

It might make sense to add one more abstraction to debugContext, with the transport. Since technically dotnet debugging can be done over ssh or docker. I'm not sure about other languages.

We still want to be able support ssh based debugging too. Thoughts?



def service_common_options(port):
def construct_options(f):
Expand Down Expand Up @@ -47,6 +49,12 @@ def invoke_common_options(f):
:param f: Callback passed by Click
"""

def validate_container_name(ctx, param, container_name):
if container_name and not ContainerManager.is_valid_container_name(container_name):
raise click.BadParameter("({}), only [a-zA-Z0-9][a-zA-Z0-9_.-] are allowed."
.format(container_name))
return container_name

invoke_options = [
template_click_option(),

Expand All @@ -56,6 +64,11 @@ def invoke_common_options(f):

parameter_override_click_option(),

click.option('--container-name',
help="When specified, Lambda function container will start with this name.",
envvar="SAM_DOCKER_CONTAINER_NAME",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should set an Env Var for this. My biggest concern with introducing this container_name option is that it breaks Lambda calling Lambda locally. Having an Env Var increases the risk of this, as it becomes a set and forget.

callback=validate_container_name),

click.option('--debug-port', '-d',
help="When specified, Lambda function container will start in debug mode and will expose this "
"port on localhost.",
Expand Down
13 changes: 8 additions & 5 deletions samcli/commands/local/invoke/cli.py
View file
Open in desktop
Expand Up @@ -12,7 +12,7 @@
from samcli.local.lambdafn.exceptions import FunctionNotFound
from samcli.commands.validate.lib.exceptions import InvalidSamDocumentException
from samcli.commands.local.lib.exceptions import OverridesNotWellDefinedError
from samcli.local.docker.manager import DockerImagePullFailedException
from samcli.local.docker.manager import DockerImagePullFailedException, DockerContainerException


LOG = logging.getLogger(__name__)
Expand Down Expand Up @@ -45,18 +45,18 @@
@pass_context # pylint: disable=R0914
def cli(ctx, function_identifier, template, event, no_event, env_vars, debug_port,
debug_args, debugger_path, docker_volume_basedir, docker_network, log_file, skip_pull_image, profile, region,
parameter_overrides):
parameter_overrides, container_name):

# All logic must be implemented in the ``do_cli`` method. This helps with easy unit testing

do_cli(ctx, function_identifier, template, event, no_event, env_vars, debug_port, debug_args, debugger_path,
docker_volume_basedir, docker_network, log_file, skip_pull_image, profile, region,
parameter_overrides) # pragma: no cover
parameter_overrides, container_name) # pragma: no cover


def do_cli(ctx, function_identifier, template, event, no_event, env_vars, debug_port, # pylint: disable=R0914
debug_args, debugger_path, docker_volume_basedir, docker_network, log_file, skip_pull_image, profile,
region, parameter_overrides):
region, parameter_overrides, container_name):
"""
Implementation of the ``cli`` method, just separated out for unit testing purposes
"""
Expand Down Expand Up @@ -87,7 +87,8 @@ def do_cli(ctx, function_identifier, template, event, no_event, env_vars, debug_
debug_args=debug_args,
debugger_path=debugger_path,
aws_region=region,
parameter_overrides=parameter_overrides) as context:
parameter_overrides=parameter_overrides,
container_name=container_name) as context:
Copy link
Contributor

Choose a reason for hiding this comment

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

The container_name options is meant to help with debugging right? Can we move this into the DebugContext?


# Invoke the function
context.local_lambda_runner.invoke(context.function_name,
Expand All @@ -101,6 +102,8 @@ def do_cli(ctx, function_identifier, template, event, no_event, env_vars, debug_
raise UserException(str(ex))
except DockerImagePullFailedException as ex:
raise UserException(str(ex))
except DockerContainerException as ex:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you fold this and the DockerImagePullFailedException into the (InvalidSamDocumentException, OverridesNotWellDefinedError) tuple? Make this a little less long.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I thought of that initially, but decided to stick to existing separation, as it seemed to me as intentional

Copy link
Contributor

Choose a reason for hiding this comment

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

I wrote the DockerImagePullFailedExpection there. Not sure why I did it that way instead of what I suggested. :)

raise UserException(str(ex))


def _get_event(event_file_name):
Expand Down
9 changes: 6 additions & 3 deletions samcli/commands/local/lib/local_lambda.py
View file
Open in desktop
Expand Up @@ -26,6 +26,7 @@ def __init__(self,
local_runtime,
function_provider,
cwd,
container_name=None,
env_vars_values=None,
aws_profile=None,
debug_context=None,
Expand All @@ -37,16 +38,17 @@ def __init__(self,
:param samcli.commands.local.lib.provider.FunctionProvider function_provider: Provider that can return a
Lambda function
:param string cwd: Current working directory. We will resolve all function CodeURIs relative to this directory.
:param string container_name: Optional. Name for the Docker container to use
:param dict env_vars_values: Optional. Dictionary containing values of environment variables
:param integer debug_port: Optional. Port to bind the debugger to
:param string debug_args: Optional. Additional arguments passed to the debugger
:param DebugContext debug_context: Optional. Contains debugging info (port, debugger path)
ndobryanskyy marked this conversation as resolved.
Show resolved Hide resolved
:param string aws_profile: Optional. AWS Credentials profile to use
:param string aws_region: Optional. AWS region to use
"""

self.local_runtime = local_runtime
self.provider = function_provider
self.cwd = cwd
self.container_name = container_name
self.env_vars_values = env_vars_values or {}
self.aws_profile = aws_profile
self.aws_region = aws_region
Expand Down Expand Up @@ -78,7 +80,8 @@ def invoke(self, function_name, event, stdout=None, stderr=None):
config = self._get_invoke_config(function)

# Invoke the function
self.local_runtime.invoke(config, event, debug_context=self.debug_context, stdout=stdout, stderr=stderr)
self.local_runtime.invoke(config, event, debug_context=self.debug_context,
container_name=self.container_name, stdout=stdout, stderr=stderr)

def is_debugging(self):
"""
Expand Down
11 changes: 6 additions & 5 deletions samcli/commands/local/start_api/cli.py
View file
Open in desktop
Expand Up @@ -47,18 +47,18 @@ def cli(ctx,

# Common Options for Lambda Invoke
template, env_vars, debug_port, debug_args, debugger_path, docker_volume_basedir,
docker_network, log_file, skip_pull_image, profile, region, parameter_overrides
):
docker_network, log_file, skip_pull_image, profile, region, parameter_overrides,
container_name):
# All logic must be implemented in the ``do_cli`` method. This helps with easy unit testing

do_cli(ctx, host, port, static_dir, template, env_vars, debug_port, debug_args, debugger_path,
docker_volume_basedir, docker_network, log_file, skip_pull_image, profile, region,
parameter_overrides) # pragma: no cover
parameter_overrides, container_name) # pragma: no cover


def do_cli(ctx, host, port, static_dir, template, env_vars, debug_port, debug_args, # pylint: disable=R0914
debugger_path, docker_volume_basedir, docker_network, log_file, skip_pull_image, profile, region,
parameter_overrides):
parameter_overrides, container_name):
"""
Implementation of the ``cli`` method, just separated out for unit testing purposes
"""
Expand All @@ -81,7 +81,8 @@ def do_cli(ctx, host, port, static_dir, template, env_vars, debug_port, debug_ar
debug_args=debug_args,
debugger_path=debugger_path,
aws_region=region,
parameter_overrides=parameter_overrides) as invoke_context:
parameter_overrides=parameter_overrides,
container_name=container_name) as invoke_context:

service = LocalApiService(lambda_invoke_context=invoke_context,
port=port,
Expand Down
14 changes: 8 additions & 6 deletions samcli/commands/local/start_lambda/cli.py
View file
Open in desktop
Expand Up @@ -55,23 +55,24 @@
@invoke_common_options
@cli_framework_options
@pass_context
def cli(ctx,
def cli(ctx, # pylint: disable=R0914
# start-lambda Specific Options
host, port,

# Common Options for Lambda Invoke
template, env_vars, debug_port, debug_args, debugger_path, docker_volume_basedir,
docker_network, log_file, skip_pull_image, profile, region, parameter_overrides
):
docker_network, log_file, skip_pull_image, profile, region, parameter_overrides,
container_name):
# All logic must be implemented in the ``do_cli`` method. This helps with easy unit testing

do_cli(ctx, host, port, template, env_vars, debug_port, debug_args, debugger_path, docker_volume_basedir,
docker_network, log_file, skip_pull_image, profile, region, parameter_overrides) # pragma: no cover
docker_network, log_file, skip_pull_image, profile, region, parameter_overrides,
container_name) # pragma: no cover


def do_cli(ctx, host, port, template, env_vars, debug_port, debug_args, # pylint: disable=R0914
debugger_path, docker_volume_basedir, docker_network, log_file, skip_pull_image, profile, region,
parameter_overrides):
parameter_overrides, container_name):
"""
Implementation of the ``cli`` method, just separated out for unit testing purposes
"""
Expand All @@ -94,7 +95,8 @@ def do_cli(ctx, host, port, template, env_vars, debug_port, debug_args, # pylin
debug_args=debug_args,
debugger_path=debugger_path,
aws_region=region,
parameter_overrides=parameter_overrides) as invoke_context:
parameter_overrides=parameter_overrides,
container_name=container_name) as invoke_context:

service = LocalLambdaService(lambda_invoke_context=invoke_context,
port=port,
Expand Down
13 changes: 13 additions & 0 deletions samcli/local/docker/container.py
View file
Open in desktop
Expand Up @@ -33,6 +33,7 @@ def __init__(self,
cmd,
working_dir,
host_dir,
name=None,
memory_limit_mb=None,
exposed_ports=None,
entrypoint=None,
Expand All @@ -52,12 +53,14 @@ def __init__(self,
:param dict exposed_ports: Optional. Dict of ports to expose
:param list entrypoint: Optional. Entry point process for the container. Defaults to the value in Dockerfile
:param dict env_vars: Optional. Dict of environment variables to setup in the container
:param string name: Optional. Name, that will be asssigned to the container, when it starts
"""

self._image = image
self._cmd = cmd
self._working_dir = working_dir
self._host_dir = host_dir
self._name = name
self._exposed_ports = exposed_ports
self._entrypoint = entrypoint
self._env_vars = env_vars
Expand Down Expand Up @@ -120,6 +123,9 @@ def create(self):
if self._entrypoint:
kwargs["entrypoint"] = self._entrypoint

if self._name:
kwargs["name"] = self._name

if self._memory_limit_mb:
# Ex: 128m => 128MB
kwargs["mem_limit"] = "{}m".format(self._memory_limit_mb)
Expand Down Expand Up @@ -256,6 +262,13 @@ def _write_container_output(output_itr, stdout=None, stderr=None):
LOG.debug("Dropping Docker container output because of unconfigured frame type. "
"Frame Type: %s. Data: %s", frame_type, data)

@property
def name(self):
"""
Gets the name of the container
"""
return self._name

@property
def network_id(self):
"""
Expand Down