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

Support debugging Golang functions. #495

Merged
merged 12 commits into from Jul 2, 2018

Conversation

Projects
None yet
5 participants
@austinlparker
Copy link
Contributor

austinlparker commented Jun 19, 2018

Issue #, if available:
#281
Description of changes:

  • Modify container options to allow for tracing inside a container (required by Delve)
  • Pass debug port into new entrypoint

Note, this will fail until lambci/docker-lambda#97 is merged and a new image is published. In the meantime, if you want to get going with this, you can build that PR's Dockerfile locally and tag it appropriately then --skip-pull-image on SAM CLI to use the local version.

Please don't merge this right away, I'd like to make it more robust and have some questions back in #281.

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

support golang debug flag
pass debug port to container

fix formatting

further formatting fixes

final formatting fixes

fix tests

update readme

@austinlparker austinlparker force-pushed the austinlparker:golang-debug branch from 5bacecb to e5b1023 Jun 20, 2018

@austinlparker

This comment has been minimized.

Copy link
Contributor

austinlparker commented Jun 22, 2018

Tests are currently failing; I'll fix those up and add coverage after getting consensus on the strategy here.

@bmoffatt

This comment has been minimized.

Copy link

bmoffatt commented Jun 22, 2018

LGTM!

@sanathkr and I had a quick chat - we're happy with the approach, though think that /var/debug should be passed to the lambdci image through some sort of environment variable.

@sanathkr
Copy link
Contributor

sanathkr left a comment

Hey @austinlparker this is amazing 🔥! Thanks for making both the lambci and SAM CLI changes.

I have some comments to improve the design but totally open to your ideas. So do speak up! It is collaborations like this that produce better software overall :)

If you don't have time or want help, hit me up. I can work on some parts of it. I really want to get this merged & released as soon as possible. I know the Gophers out there can't wait too!

@@ -44,7 +44,8 @@ def __init__(self,
docker_network=None,
log_file=None,
skip_pull_image=None,
aws_profile=None):
aws_profile=None,
delve_path=None):

This comment has been minimized.

@sanathkr

sanathkr Jun 22, 2018

Contributor

I read your comment about delve_path in the Issue. Yes, we should generalize this and not pass around one parameter specific to Golang everywhere.

Also, I think this is a good time to create a new data object say DebugingContext that encapsulates all properties necessary for debugging - debug port, debug args and delve path. This will allow us to first simplify the call signatures, second abstract away how customers pass debug information (CLI args, config file, sane defaults), third auto-load debug context in future from other config files like vscode's launch configuration if it has all info you need.

On that note, I am curious if you can reuse debug_args property for delve_path?

What do you think about these two?

This comment has been minimized.

@austinlparker

austinlparker Jun 25, 2018

Contributor

I'm leery of reusing debug_args for the delve_path because I can see end-users wanting to pass in options via that and there's no reason the container can't support it. I think a DebugContext object makes a lot of sense though. I think the delve_path can be generalized to debugger_path and packed into the new debug context. That'll enable us to re-use the argument for bringing in netcore debuggers.

This comment has been minimized.

@jfuss

jfuss Jun 25, 2018

Contributor

Random thought but something we should be considering. I do prefer the general path but I think we need to incorporate some specific language runtime into the debugger_path option.

One the of the benefits of serverless, is that each Lambda can be its own runtime. While this is the invoke_context, it is also used in the start-api command. For a specific invoke, I think a generic debugger_path makes perfect sense but not sure about in the context of start-api. By giving a generic path, I feel that we restrict an API GW + Lambda to be one language when it doesn't need to (or should) be. If the debug_path was a mapping of runtime to a path, this can be easily supported.

Thoughts?

This comment has been minimized.

@austinlparker

austinlparker Jun 25, 2018

Contributor

I think that makes sense, yeah. To expand on it, what if the DebugContext was runtime-specific and the CLI allowed for multiple debug flags? Envisioning something like the following as an invocation:

sam local start-api -d runtime=go1.x debug_port=5986 debugger_path=/path/to/debugger -d runtime=nodejs8.10 debug_port=5986 debug_args='--foo'...

This would probably end up being a breaking change, but I'm not sure how big of a deal it would be; probably not a lot of people using automation around the -d option.

This comment has been minimized.

@austinlparker

austinlparker Jun 25, 2018

Contributor

@jfuss / @sanathkr - I'm going to plead ignorance on python here, but after exploring the Click docs for a while, I can't seem to find a way to have it deserialize named arguments to an option like I did in my invocation example. The best thing I can find would be to load the debug context object from a file. Y'all think that would work? So it'd look something like this...

sam local start-api -d debug_contexts.json...

debug_contexts.json

{
  "runtime": "go1.x",
  "port": 5986,
  "debugger_path": "path/to/dlv",
  "debug_args": ""
},
{
  "runtime": "nodejs8.10",
  "port": 5986,
  "debug_args": "--foo --bar=baz"
}
}
}
kwargs["security_opt"] = ["seccomp:unconfined"]
kwargs["cap_add"] = ["SYS_PTRACE"]

This comment has been minimized.

@sanathkr

sanathkr Jun 22, 2018

Contributor

This is the bit that @bmoffatt helped me understand. Can you add comments explaining what these are and why they are necessary?

@@ -94,6 +96,24 @@ def create(self):
"tty": False
}

if self._delve_path:

This comment has been minimized.

@sanathkr

sanathkr Jun 22, 2018

Contributor

Leaving a note here for your reference. This property will become more generic soon. When it does, this if condition being specific to golang will not make sense.

entrypoint = ["/var/runtime/aws-lambda-go"] \
+ [
"-debug=true",
"-delvePort=" + debug_port

This comment has been minimized.

@sanathkr

sanathkr Jun 22, 2018

Contributor

Can we set delvePath here? This will allow SAM CLI to continue to work even if Docker image changed their default value of delvePath.

I am okay with a hard-coded value like /tmp/samcli_debug_files or something

@@ -94,6 +96,24 @@ def create(self):
"tty": False
}

if self._delve_path:
LOG.info("in golang debug mode")
kwargs["volumes"] = {

This comment has been minimized.

@sanathkr

sanathkr Jun 22, 2018

Contributor

Design feedback: container.py is the base class for all containers. The options you add, like PTrace and delve mounts, are specific to Lambda Containers which is implemented in lambda_containers.py file. Can you try and move these changes to the lambda_containers.py file inside a method like _additional_container_options that is called from base-class here? In the subclass, you can add these properties only for golang in debug mode so it doesn't impact other runtimes.

@austinlparker

This comment has been minimized.

Copy link
Contributor

austinlparker commented Jun 26, 2018

@sanathkr I pushed my WIP changes to see if we're on the same page still - needs cleanup and pruning of the 'old' debug flags, but the basic idea is there.

Instead of passing in debug args via the command line on --debug_port and --debug_args, there's a --debug-context-file argument that will be able to contain an entry per runtime, and get sorted out through there. I've got a few bugs and such that I know about but I wanted to make sure this is moving in the right direction.

@bheemreddy181

This comment has been minimized.

Copy link

bheemreddy181 commented Jun 28, 2018

Debugging version of SAM with GO isn't merged yet? with Sam CLI version 0.4.0 i was not able to debug Go Lamdas.

@sanathkr
Copy link
Contributor

sanathkr left a comment

@austinlparker Its amazing that you got the DebuggerContext implemented so fast! Love the passion :) I have one suggestion to maintain backwards compat and still have this functionality.

"""
Implementation of the ``cli`` method, just separated out for unit testing purposes
"""

LOG.debug("local invoke command is called")

event_data = _get_event(event)

debug_context = DebugContext.get_debug_ctx(debug_context_file)

This comment has been minimized.

@sanathkr

sanathkr Jun 28, 2018

Contributor

Let's not change the CLI interface because it would be a breaking change. But I like everything about the DebugContext encapsulating all of debug parameters.

For Golang, we do need to expose another property called --debugger-path. This is creating an explosion of parameters but I think it is okay compared to breaking backwards compatibility.

The other option is to expose another file for debug context, but this creates yet another config file to write. Instead of a debugging specific YAML file, it is better to implement a generic .samrc configuration file where customers can provide a more properties including debugging context. I think .samrc is a better solution in the longer term.

Do you agree with this? Can you change the CLI interface to add a debugger_path arg and restore other debug properties? In the implementation, we can set default values for debugger_path on a per-runtime basis.

:param DebugContext debug_options: DebugContext for the runtime of the container.
:return dict: Dictionary containing volume map passed to container creation.
"""
if not debug_options:

This comment has been minimized.

@sanathkr

sanathkr Jun 28, 2018

Contributor

this should return None if debug_options.debugger_path is empty

+ [
"-debug=true",
"-delvePort=" + str(debug_port),
"-delvePath=/tmp/lambci_debug_files/dlv",

This comment has been minimized.

@sanathkr

sanathkr Jun 28, 2018

Contributor

can you move /tmp/lambci_debug_files to a class-level or module-level constant?

austinlparker added some commits Jun 29, 2018

move default container debug path to class const
return addt'l volumes if debug path exists
@sanathkr
Copy link
Contributor

sanathkr left a comment

Hey @austinlparker, thanks for making all the changes. The code looks perfect and docs are great! 💯

I do have one small comment about a test though. It should be a quick change to prevent loss of test coverage. This will ensure future changes don't break debugging :)

README.rst Outdated
@@ -399,11 +399,21 @@ Debugging Golang functions
^^^^^^^^^^^^^^^^^^^^^^^^^^

Golang function debugging is slightly different when compared to Node.JS,
Java, and Python. We utilize `delve <https://github.com/derekparker/delve>`__
Java, and Python. We require `delve <https://github.com/derekparker/delve>`__

This comment has been minimized.

@sanathkr

sanathkr Jun 29, 2018

Contributor

Love the attention to detail in the docs! 👏

This comment has been minimized.

@austinlparker

austinlparker Jun 29, 2018

Contributor

Thanks, I think the updated phrasing makes more sense since the delve binary is now user-provided.


def _get_debug_context(debug_port, debug_args, debugger_path):
"""
Returns a debug context from debug options; Separated out for unit testing.

This comment has been minimized.

@sanathkr

sanathkr Jun 29, 2018

Contributor

nice!

:param string debugger_path: Path to debugger on host
:return DebugContext:
"""
if not debug_port or debug_args or debugger_path:

This comment has been minimized.

@sanathkr

sanathkr Jun 29, 2018

Contributor

it took me a while to validate and understand this boolean, but it does work :)

debug_args=None):

self.debug_port = debug_port
self.runtime = runtime

This comment has been minimized.

@sanathkr

sanathkr Jun 29, 2018

Contributor

so runtime is unused, right? can we remove it?

This comment has been minimized.

@austinlparker

austinlparker Jun 29, 2018

Contributor

It's currently unused, yeah, I can pop it out of there.


# Run the container and get results
self.manager_mock.run.assert_called_with(container)
self.runtime._configure_interrupt.assert_called_with(self.name, self.DEFAULT_TIMEOUT, container, True)
self.runtime._configure_interrupt.assert_called_with(self.name, self.DEFAULT_TIMEOUT, container, False)

This comment has been minimized.

@sanathkr

sanathkr Jun 29, 2018

Contributor

This change actually causes a loss of coverage. We do want to verify that when DebuggingContext is set, the runtime passes True here.

This is important because the interrupt handler performs something very different when this property is set to True. This is the closest we have to ensuring the debugging continues to work.

Can you set DebugContext object when creating the LambdaRuntime object so this condition will hold true?

stdout=stdout,
stderr=stderr)

# Run the container and get results
self.manager_mock.run.assert_called_with(container)

self.runtime._configure_interrupt.assert_called_with(self.name, self.DEFAULT_TIMEOUT, container, True)
self.runtime._configure_interrupt.assert_called_with(self.name, self.DEFAULT_TIMEOUT, container, False)

This comment has been minimized.

@sanathkr

sanathkr Jun 29, 2018

Contributor

same as above.

@sanathkr
Copy link
Contributor

sanathkr left a comment

Perfect! Thanks a ton for going through all the iterations really quickly and producing high quality code 👷

I will mark the PR for merging and get it out in the next release.

@jfuss jfuss merged commit 9ae4b80 into awslabs:develop Jul 2, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jfuss added a commit that referenced this pull request Jul 3, 2018

jfuss added a commit to jfuss/aws-sam-cli that referenced this pull request Jul 3, 2018

Revert "feat(debugging): Support debugging Golang functions. (awslabs…
…#495)"

This reverts commit 9ae4b80.
The commit breaks functional and integ tests. Reverting to make develop buildable

jfuss added a commit that referenced this pull request Jul 3, 2018

Revert "feat(debugging): Support debugging Golang functions. (#495)" (#…
…526)

This reverts commit 9ae4b80.
The commit breaks functional and integ tests. Reverting to make develop buildable

jfuss added a commit to jfuss/aws-sam-cli that referenced this pull request Jul 19, 2018

feat(debugging): Support debugging Golang functions.
Golang Debugging was initally implemented in awslabs#495, but reverted in awslabs#526
due to functional and integration tests failures. This commit reverts awslabs#526
and adds in the testing gaps. The specific differences are:

* _get_debug_context method was replaces with the __bool__/__nonzero (py2)
* Added additional unit tests for lambda_container to cover added methods
* Updated Functional tests
* Added debug_context support in start-lambda

@jfuss jfuss referenced this pull request Jul 19, 2018

Merged

feat(debugging): Support debugging Golang functions. #565

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