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

Lambci #15

Merged
merged 31 commits into from Jul 23, 2020
Merged

Lambci #15

merged 31 commits into from Jul 23, 2020

Conversation

jssmith
Copy link
Collaborator

@jssmith jssmith commented Mar 5, 2020

This change adds custom runtime support and layers support to the awsLambda provider.

@jssmith jssmith requested a review from NathanTP March 5, 2020 03:58
@jssmith jssmith marked this pull request as ready for review March 19, 2020 03:57
Copy link
Collaborator Author

@jssmith jssmith left a comment

Choose a reason for hiding this comment

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

This change represents a great deal of progress by adding support for custom runtimes and layers on Lambda, as well as for the Docker-based Lambda-like environment that can run either locally or on a remote server. I can suggest a number of enhancements that should probably be deferred to a separate task in the interest of getting this merged: 1/ provide SRK mechanism to start the lambci provider, 2/ review use of SSH and consider using Go rather than system libraries, 3/ some reorganization of the documentation.

configs/example-srk.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@NathanTP NathanTP left a comment

Choose a reason for hiding this comment

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

lots of inline comments.

Some broad comments:

  1. More comments would be nice, at least at the tops of functions (e.g. 'NextLayerVersion' in lambci-lambda/util.go)
  2. More docs, lots of this stuff was new to me and even after reading the code I wasn't 100% sure what it did.
  3. I'm still pretty unclear on runtimes, layers, and how tied they are to providers.

In principle, this all looks good.

cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
configs/example-srk.yaml Outdated Show resolved Hide resolved
configs/example-srk.yaml Outdated Show resolved Hide resolved
docs/source/Examples/advanced.rst Show resolved Hide resolved
pkg/lambci-lambda/service.go Outdated Show resolved Hide resolved
pkg/lambci-lambda/service.go Outdated Show resolved Hide resolved
pkg/lambci-lambda/service.go Show resolved Hide resolved
pkg/lambci-lambda/service.go Show resolved Hide resolved
pkg/lambci-lambda/service.go Show resolved Hide resolved
Copy link
Collaborator

@leoholz leoholz left a comment

Choose a reason for hiding this comment

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

@NathanTP, thank you for the detailed review. I integrated a lot of the suggested changes and merged the master branch with the new SRK server.
We probalby should resolve the confusion about runtimes and layers and how the SRK should handle them next.

pkg/lambci-lambda/service.go Show resolved Hide resolved
docs/source/Examples/advanced.rst Show resolved Hide resolved
docs/source/Examples/lambci.rst Outdated Show resolved Hide resolved
docs/source/Examples/lambci.rst Outdated Show resolved Hide resolved
$3

As an example the following command will start a Python lambda function
container with data from the ``~/lambci`` directory.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently this is independent of SRK and must be setup manually. We should consider a deeper integration later on.

docs/source/Examples/lambci.rst Show resolved Hide resolved
docs/source/Examples/lambci.rst Outdated Show resolved Hide resolved
Using a custom runtime
*******************************************************************************

To use a custom runtime, specify ``provided`` as runtime name for the
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's specific for AWS and LambCI (which mirrors AWS). It is the runtime name that must be specified if the actual runtime is provided via a layer. The "provided" runtime probably just passes through to the layer-provided runtime.


To use a custom runtime, specify ``provided`` as runtime name for the
``lambci.sh`` script. The lambda container then expects a complete lambda
runtime in the ``runtime`` directory. For this, create a layer that contains
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, there is some confusion about this term. Let's first discuss how the SRK should handle layers and custom runtimes and then move on to documentation.

@jssmith
Copy link
Collaborator Author

jssmith commented Apr 3, 2020

I made some extensive updates to the documentation. The idea is that you can more or less copy-and-paste and be up and running. @leoholz, please see if I got things right. @NathanTP please have a look and see whether this resolves your concerns.

Copy link
Contributor

@NathanTP NathanTP left a comment

Choose a reason for hiding this comment

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

I've added #18 and #19 to document some future work to build on this PR. We don't need to boil the ocean here and this PR is too big already, I just don't want to forget.

There are some comments/change requests inline. A few more global notes:

  • Document lambci and the new aws-lambda options in docs/source/Configuration.rst
  • examples/requests needs a README. What does it do?
  • We could probably move the whole shell package into the srk package (it already includes a bunch of utilities in pkg/srk/util.go

Overall I'm pretty happy with this now, it all makes sense.

configs/example-srk.yaml Outdated Show resolved Hide resolved
configs/example-srk.yaml Outdated Show resolved Hide resolved
docs/source/Examples/lambci.rst Outdated Show resolved Hide resolved
docs/source/Examples/lambci.rst Show resolved Hide resolved
pkg/aws-lambda/awslambda.go Outdated Show resolved Hide resolved
pkg/lambci-lambda/service.go Show resolved Hide resolved
pkg/lambci-lambda/service.go Outdated Show resolved Hide resolved
pkg/lambci-lambda/util.go Outdated Show resolved Hide resolved
pkg/shell/run.go Show resolved Hide resolved
srkServer/server/server.go Show resolved Hide resolved
@leoholz
Copy link
Collaborator

leoholz commented Jun 15, 2020

Please see the update commit and my comments.

Copy link
Contributor

@NathanTP NathanTP left a comment

Choose a reason for hiding this comment

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

Some additional comments (mostly docs stuff). I'm still not 100% sure about 'files' and 'layers', I'll discuss with Johann offline and get back here with our conclusions. Otherwise this is looking pretty good.

docs/source/Examples/lambci.rst Outdated Show resolved Hide resolved
docs/source/Examples/lambci.rst Show resolved Hide resolved
docs/source/Examples/lambci.rst Show resolved Hide resolved
docs/source/Examples/lambci.rst Show resolved Hide resolved
docs/source/Examples/lambci.rst Show resolved Hide resolved
docs/source/Examples/lambci.rst Show resolved Hide resolved
@jssmith
Copy link
Collaborator Author

jssmith commented Jul 22, 2020

@NathanTP please review - I've updated the documentation in accordance with your requests and merged the latest changes.

Copy link
Contributor

@NathanTP NathanTP left a comment

Choose a reason for hiding this comment

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

It looks like all my comments have been resolved. I'm in favor of just merging this now and dealing with any lingering problems as needed.

@jssmith jssmith merged commit bfe3567 into master Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants