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

Container name feature implementation #774

Closed
wants to merge 14 commits into
base: develop
from

Conversation

Projects
None yet
5 participants
@ndobryanskyy
Copy link
Contributor

ndobryanskyy commented Nov 19, 2018

Changes

This PR brings new feature to the SAM cli - --container-name option, which enables customer to specify the name for AWS Lambda runtime Docker container when run.

Original issue

#568 - root cause issue; #759 - design doc PR, which describes the need for new --container-name flag. It is a requred step for enabling .NET Core debugging. For detailed explanation see the rendered doc.

Sidenote

I do still need help of where to put some help to let the user know, that this flag is here. Any comments on this is appreciated.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@sthulb

This comment has been minimized.

Copy link
Contributor

sthulb commented Nov 19, 2018

I looked at this issue yesterday. I was thinking of going further than just names for containers — I was also thinking of adding labels too (perhaps ones for another PR)

@ndobryanskyy

This comment has been minimized.

Copy link
Contributor

ndobryanskyy commented Nov 19, 2018

@sthulb yes we can add this functionality, but I don't see practical use-case for this yet.
--container-name is specifically added to enable .NET Core debugging, see - #568 .

And what do you think about the changes, do you have any remarks, comments?

@sthulb

This comment has been minimized.

Copy link
Contributor

sthulb commented Nov 23, 2018

from a personal point of view, I’d rather see the container name be automatic, so every container gets a name, probably based on the resource name.

It would still give you the ability to have an explicit name to allow for debugging.

@ndobryanskyy

This comment has been minimized.

Copy link
Contributor

ndobryanskyy commented Nov 23, 2018

@sthulb, you mean, that SAM should assign explicit name for each container it runs? And --container-name option will override this behavior?

@sanathkr
Copy link
Contributor

sanathkr left a comment

Thank you so much for adding this feature. You have done a good job at understanding the conventions used in this project and following it 💯. I have some comments to simplify certain parts of the code.

Also, can you add a functional test and integration test that tests for the container name property? In the functional & integ tests use a random string as container name so we can run multiple instances of integ tests in parallel.

Feel free to disagree with my comments or propose alternatives if anything doesn't make sense.

Show resolved Hide resolved samcli/local/docker/manager.py Outdated
Show resolved Hide resolved Makefile
Show resolved Hide resolved samcli/local/docker/manager.py
raise InvokeContextException("Running AWS SAM projects locally requires Docker. Have you got it installed?")

if self._container_name:
if not container_manager.is_valid_container_name(self._container_name):

This comment has been minimized.

@sanathkr

sanathkr Nov 24, 2018

Contributor

Can you move the container name regex validation into a CLI option? You can create a new common option similar to this https://github.com/awslabs/aws-sam-cli/blob/develop/samcli/commands/_utils/options.py#L93 and use a callback to validate.

This will help reuse this logic across sam local invoke|start-api|start-lambda commands

This comment has been minimized.

@ndobryanskyy

ndobryanskyy Nov 24, 2018

Contributor

Very good idea, thanks! I'll certainly do it.

This comment has been minimized.

@ndobryanskyy

ndobryanskyy Nov 25, 2018

Contributor

It seems, that there were common options already, so I've just added callback

click.option('--container-name',
help="When specified, Lambda function container will start with this name.",
envvar="SAM_DOCKER_CONTAINER_NAME",
callback=validate_container_name),

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

Will that do?

Show resolved Hide resolved samcli/commands/local/cli_common/invoke_context.py Outdated
Show resolved Hide resolved samcli/local/docker/manager.py
Show resolved Hide resolved samcli/commands/local/cli_common/invoke_context.py Outdated
Show resolved Hide resolved samcli/local/docker/manager.py Outdated
Show resolved Hide resolved samcli/local/docker/manager.py Outdated
container.start(input_data=input_data)
except docker.errors.APIError as api_error:
if api_error.status_code == 409:
raise DockerContainerException("'{}' Docker container name is already taken".format(container.name))

This comment has been minimized.

@sanathkr

sanathkr Nov 24, 2018

Contributor

nit: Can we replace 'taken' with 'in use' ? Conveys the intent better

This comment has been minimized.

@ndobryanskyy

ndobryanskyy Nov 24, 2018

Contributor

Your version aligns more closely with actual docker error message. So why not, I will do it.
image

Or we can just str(api_error) it will display something similar, what do you think?

This comment has been minimized.

@ndobryanskyy

ndobryanskyy Nov 25, 2018

Contributor

I've removed is_container_name_taken method altogether. Now it does this

except docker.errors.APIError as api_error:
raise DockerContainerException("[From Docker]\n{}".format(api_error))

Preceding the error message with "[From Docker]", because Docker API error messages are pretty and informative.

If you want I can still log.debug(exc_info=True) here also to get traceback

ndobryanskyy added some commits Nov 25, 2018

@ndobryanskyy

This comment has been minimized.

Copy link
Contributor

ndobryanskyy commented Nov 28, 2018

Fixed review comments and implemented integration and functional tests. @sanathkr, please, take a look, when you have time.

@jfuss

This comment has been minimized.

Copy link
Contributor

jfuss commented Nov 30, 2018

@ndobryanskyy Could you please rebase and address the merge conflicts. Our recent release brought in a bunch of changes.

I am going to review now, as well.

@jfuss
Copy link
Contributor

jfuss left a comment

I have made a couple comments. Let me know what you think.

My biggest concern here is the impact on calling Lambda to Lambda locally and that container_name can be set outside of debugging. If a customer adds this container_name to the command, any container that is created will be get this name. If my lambda is wired up to call the endpoint from start-lambda (that defines a container_name, or is set in the env vars), then the lambda function I want to call will fail since the container already exists (I use the sdk or cli to call Lambda1 that calls Lambda2. Both lambdas will be created with the same container name and will fail). As of this revision, the container_name can be set outside the 'debug' experience. To me, this will add extra overhead on the customer to understand when to use this Option. The start-api command allows for multiple requests when not running in debug mode, so adding this container_name in that context completely breaks that. To make it easier on constrain the problem, we should move this container_name into the DebugContext and only set the container name when we are in debug mode. We can always lift this restriction but constraining in the future becomes much more difficult.

@@ -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",

This comment has been minimized.

@jfuss

jfuss Nov 30, 2018

Contributor

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.

@@ -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:

This comment has been minimized.

@jfuss

jfuss Nov 30, 2018

Contributor

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

@@ -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:

This comment has been minimized.

@jfuss

jfuss Nov 30, 2018

Contributor

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

This comment has been minimized.

@ndobryanskyy

ndobryanskyy Nov 30, 2018

Contributor

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

This comment has been minimized.

@jfuss

jfuss Nov 30, 2018

Contributor

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

@@ -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

This comment has been minimized.

@jfuss

jfuss Nov 30, 2018

Contributor

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?

This comment has been minimized.

@ndobryanskyy

ndobryanskyy Nov 30, 2018

Contributor

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?

This comment has been minimized.

@jfuss

jfuss Nov 30, 2018

Contributor

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.

This comment has been minimized.

@ndobryanskyy

ndobryanskyy Nov 30, 2018

Contributor

@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

This comment has been minimized.

@jfuss

jfuss Nov 30, 2018

Contributor

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.

This comment has been minimized.

@jfuss

jfuss Nov 30, 2018

Contributor

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 :)

This comment has been minimized.

@ndobryanskyy

ndobryanskyy Nov 30, 2018

Contributor

@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

This comment has been minimized.

@TheSriram

TheSriram Nov 30, 2018

Contributor

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?

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?")

This comment has been minimized.

@TheSriram

TheSriram Nov 30, 2018

Contributor

Nice touch!

@@ -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

This comment has been minimized.

@TheSriram

TheSriram Nov 30, 2018

Contributor

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?

@ndobryanskyy

This comment has been minimized.

Copy link
Contributor

ndobryanskyy commented Dec 1, 2018

Guys, yesterday, after looking through your comments I've actually got hit by brilliant idea. What if we just eliminate the container name feature altogether and use some cool trick with Docker, namely docker ps --filter.

The Trick

Docker supports listing containers by using docker ps command, but this particular command not only can list all containers, but it could also filter them based on certain attributes. But what is most interesting for us is that it could filter running containers based on published port (and as you know, when -d <debugger_port> flag is supplied to SAM CLI it will publish debugger_port for the running container), so using docker ps --filter publish=<debugger_port> -q (-q for returning only container ID) we could easily find id of the Lambda container started by SAM, without knowing or specifying the exact name. Pretty neat, right ?

Real world example

Yesterday, I've also tried to use this approach for .NET Core debugging by checking out SAM develop branch, adding debugging support entry point there, which uses my modified version of .NET Core lambda runner with wait for debugger flag support. And it worked! 🎉

This how my pipeTransport section of launch.json look like:

"pipeTransport": {
   "pipeProgram": "powershell",
   "pipeArgs": [ 
     "-c",
     "docker exec -i $(docker ps -q -f publish=6000)"
   ],
   "debuggerPath": "/tmp/lambci_debug_files/vsdbg",
   "pipeCwd": "${workspaceRoot}"
}

Lambda was invoked with this command: samdev local invoke -e event.json -d 6000 --debugger-path "C:/users/ndobryanskyy/vsdbg" --skip-pull-image "SuperFunction" --region us-east

I like that it requires nearly no changes to the SAM CLI in order to work,the only thing we need to is provide customers with this launch.json file. I will have time tomorrow to investigate whether it is possible to use the same technique for VS 2017, but I'm almost sure, that the answer is yes.

@sanathkr @TheSriram @jfuss what do you guys think of it?
I can prepare the PR with required changes by Monday. And file another one with ContainerManager improvements from this one.

@sanathkr

This comment has been minimized.

Copy link
Contributor

sanathkr commented Dec 2, 2018

This is incredibly slick! I wish we had found this before you did the whole container name PR.

+1 to take this route as it leads to a good UX

@ndobryanskyy ndobryanskyy referenced this pull request Dec 4, 2018

Merged

Move checking Docker connectivity to ContainerManager #828

3 of 3 tasks complete
@ndobryanskyy

This comment has been minimized.

Copy link
Contributor

ndobryanskyy commented Dec 5, 2018

Closing it as not required as of #825

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment