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

Invoke local docker #5863

Merged
merged 18 commits into from Mar 14, 2019

Conversation

Projects
5 participants
@dschep
Copy link
Member

dschep commented Feb 22, 2019

This is a work in progress for implementing docker based invoke local support.

Feedback greatly appreciated

What did you implement:

Closes #5848 by using docker with the docker-lambda images from lambci.
Closes #3456

The main benefits are:

  • More runtimes supported, even provided!
  • Layers support
  • runtime environments more closely resemble real lambda env
    • logging environment
    • paths (your handler is actually run at /var/task)
    • linux (as opposed to host OS)

The feature is currently implemented by default for previously unsupported runtimes, and can be enabled for python/javascript/java/ruby with the the --docker flag.

How did you implement it:

  • local layers are resolved by traversing the Ref and arn based layers are downloaded and extracted into .serverless/invokeLocal/layers/<layername>.
  • generate .serverless/invokeLocal/Dockerfile with a FROM that bases it on lambci/lambda:${runtime} and then ADDs each layer to /opt
  • build said Dockerfile
  • run the image with working dir mounted as /var/task and passing the lambda and event as the arguments

How can we verify it:

  • install with npm i -g serverless/serverless#invoke-local-docker
  • create an aws project
  • sls invoke local -f hello --docker
  • add a layer and try to use it again!

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: NO
Is it a breaking change?: NO

@dschep

This comment has been minimized.

Copy link
Member Author

dschep commented Feb 22, 2019

I've tested with:

@pmuens
Copy link
Member

pmuens left a comment

Just test it today and it works great so far 👍

Just added some comments for code snippets I had to swap out in order to make this work.

While playing around with this I had a some questions about compiled runtimes and dependency usage. The Serverless Framework for example ships with different templates for different build systems. Is there something special we need to do here if we e.g. pick Java as a runtime and use gradle vs. maven? How would that work with the official Lamci Docker image? Another question would be about cross-compilation. All binaries need to be build for Linux in order to work within Lambda. So the question here is if we keep invoke local separate and let the deploy command deal with cross compilation or if we should roll out containers for more than just invoke local since right now you need certain plugins to deal with the cross-compilation issue.

A prodct call would be if we want to add Docker supported invoke local on newer runtimes and e.g. keep the older ones where no Docker was needed before. We could implement a simple check which runtime is used and then only execute the Docker path once one of the compiled / newer runtimes is used. This way not all users need to install Docker to use invoke local and it therefore wouldn't be a breaking change.

Show resolved Hide resolved lib/plugins/aws/invokeLocal/index.js Outdated
Show resolved Hide resolved lib/plugins/aws/invokeLocal/index.js
Show resolved Hide resolved lib/plugins/aws/invokeLocal/index.js Outdated
Show resolved Hide resolved lib/plugins/aws/invokeLocal/index.js Outdated
Show resolved Hide resolved lib/plugins/aws/invokeLocal/index.js Outdated
Show resolved Hide resolved lib/plugins/aws/invokeLocal/index.js Outdated
@dschep

This comment has been minimized.

Copy link
Member Author

dschep commented Feb 25, 2019

Switched to path.join, the right logging func, and split download into its own step.

Also thinking layer content should be cached between invokations (SAM does this too). Should that live in a global cache/tmp dir? or in .serverless? Do we have any caching in the framework currently?

@dschep dschep force-pushed the invoke-local-docker branch from 4d61753 to accd931 Mar 5, 2019

@pmuens
Copy link
Member

pmuens left a comment

Just tested this out again today while looking into #3456 and it works great!

Love the --docker flag to opt-in for unsupported runtimes 👌

Did another code review and added some comments....

Show resolved Hide resolved lib/plugins/aws/invokeLocal/index.js Outdated
Show resolved Hide resolved lib/plugins/aws/invokeLocal/index.js Outdated
Show resolved Hide resolved lib/plugins/aws/invokeLocal/index.test.js
Show resolved Hide resolved lib/plugins/aws/invokeLocal/index.test.js
Show resolved Hide resolved lib/plugins/aws/invokeLocal/index.test.js Outdated
Show resolved Hide resolved lib/plugins/aws/invokeLocal/index.js Outdated
Show resolved Hide resolved lib/plugins/aws/invokeLocal/index.js Outdated
Show resolved Hide resolved lib/plugins/aws/invokeLocal/index.js Outdated
Show resolved Hide resolved lib/plugins/aws/invokeLocal/index.js Outdated
Show resolved Hide resolved lib/plugins/aws/invokeLocal/index.js Outdated

@dschep dschep marked this pull request as ready for review Mar 6, 2019

@dschep dschep requested a review from pmuens Mar 6, 2019

@sheerun

This comment has been minimized.

Copy link

sheerun commented Mar 12, 2019

I get following error:

Serverless plugin "/usr/local/lib/node_modules/serverless/lib/plugins/aws/invokeLocal/index.js" initialization errored: Cannot find module 'jszip'

@triptec

This comment has been minimized.

Copy link

triptec commented Mar 12, 2019

@dschep seems jszip is listed under devDependencies so that might need to be moved to dependencies. Thanks for the work by the way, looking forward to having this merged.

@pmuens
Copy link
Member

pmuens left a comment

Just tested it with the follwoing config and it works fine!

service:
  name: test

provider:
  name: aws
  stage: dev
  runtime: nodejs8.10

functions:
  hello:
    handler: handler.hello
    layers:
      - arn:aws:lambda:us-east-1:744348701589:layer:bash:5

Also used Python and Ruby in combination with --docker and both worked fine as well.

Looks like jszip needs to be moved form devDependencies to dependencies (as mentioned) above. Other than that it looks like it's GTM quite soon :shipit: 🎉

Show resolved Hide resolved lib/plugins/aws/invokeLocal/index.js Outdated
Show resolved Hide resolved lib/plugins/aws/invokeLocal/index.js Outdated
Show resolved Hide resolved lib/plugins/aws/invokeLocal/index.js Outdated
Show resolved Hide resolved lib/plugins/aws/invokeLocal/index.test.js

@dschep dschep requested a review from pmuens Mar 13, 2019

@pmuens

pmuens approved these changes Mar 14, 2019

Copy link
Member

pmuens left a comment

Thanks for addressing the PR comments 👍

Just looked into this again and tested it thoroughly. Works great. I just added a check whether the Docker daemon is running. This way users won't be surprised if invoke local doesn't work and they only see an error message with the exit code.

Will merge once the build is green.

@pmuens pmuens added pr/in-review and removed pr/in-progress labels Mar 14, 2019

@pmuens pmuens added this to the 1.39.0 milestone Mar 14, 2019

@pmuens pmuens added this to In progress in Serverless via automation Mar 14, 2019

@pmuens pmuens merged commit 00d129c into master Mar 14, 2019

2 checks passed

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

Serverless automation moved this from In progress to Done Mar 14, 2019

@pmuens pmuens deleted the invoke-local-docker branch Mar 14, 2019

@softprops softprops referenced this pull request Mar 15, 2019

Merged

invoke local support #29

scottbartell added a commit to scottbartell/serverless that referenced this pull request Mar 16, 2019

@sime sime referenced this pull request Mar 17, 2019

Closed

Docker support #622

'Lambda', 'getLayerVersion', { LayerName: layerArn, VersionNumber: layerVersion })
.then(layerInfo => download(
layerInfo.Content.Location,
layerContentsPath,

This comment has been minimized.

Copy link
@endeepak

endeepak Apr 2, 2019

Contributor

@dschep I think this should be downloading to layerContentsCachePath instead of layerContentsPath. Currently this code creates empty directory in layerContentsCachePath in first run and subsequent runs will have empty directory copied to layerContentsPath. Due to this handler fails to import layer in subsequent runs

This comment has been minimized.

Copy link
@endeepak

endeepak Apr 2, 2019

Contributor

I've raised an issue for this -> #5990

This comment has been minimized.

Copy link
@dschep

dschep Apr 8, 2019

Author Member

Good catch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.