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

feat(cli): support hotswapping Lambda functions that use Docker images #18319

Merged
merged 11 commits into from Jan 14, 2022

Conversation

tmokmss
Copy link
Contributor

@tmokmss tmokmss commented Jan 8, 2022

closes #18302

We must just update ImageUri with the new ECR image url.

PR for InlineCode hotswap: #18408

----

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

@gitpod-io
Copy link

gitpod-io bot commented Jan 8, 2022

@tmokmss tmokmss marked this pull request as draft January 8, 2022 10:56
@github-actions github-actions bot added the package/tools Related to AWS CDK Tools or CLI label Jan 8, 2022
@mergify
Copy link
Contributor

mergify bot commented Jan 8, 2022

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

@tmokmss tmokmss changed the title (cli): Support hotswap for Lambda code with InlineCode and DockerImageCode feat(cli): Support hotswap for Lambda code with InlineCode and DockerImageCode Jan 8, 2022
@tmokmss tmokmss marked this pull request as ready for review January 9, 2022 04:24
@rix0rrr rix0rrr assigned skinny85 and unassigned rix0rrr Jan 10, 2022
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Looks great @tmokmss! I tested the functionality, and it works beautifully. If we could only separate the inline code changes from the Docker image changes, that would be great.

@skinny85
Copy link
Contributor

One small detail: I see that sometimes the deployment takes a while with the Docker images - I keep refreshing, but the old code is still there for like 5 to 10 seconds. It could be because my test app also uses API Gateway.

Did you see that behavior too? Maybe we need to wait for something in the Docker image case?

@tmokmss
Copy link
Contributor Author

tmokmss commented Jan 12, 2022

Hi @skinny85 thank you for the review! I agree with your point, possibly submitting a new revision and a PR for inline code today.

As for a few seconds delay on Lambda code update, I usually see them even in non-Docker Lambda functions. I guess it's a Lambda internal matter and there isn't particularly much to do for us.

@skinny85
Copy link
Contributor

Hi @skinny85 thank you for the review! I agree with your point, possibly submitting a new revision and a PR for inline code today.

Of course! Thank you for submitting the PR, it's great 🙂.

As for a few seconds delay on Lambda code update, I usually see them even in non-Docker Lambda functions. I guess it's a Lambda internal matter and there isn't particularly much to do for us.

Really? That's super interesting! Any chance you can record a quick video/GIF showing me that happening?

I actually have a sneaking suspicion there might be something we can do there, as I had to do something similar in this code that deals with Lambda Versions.

@tmokmss
Copy link
Contributor Author

tmokmss commented Jan 12, 2022

@skinny85
Hi I found a blog referring to this behavior. https://aws.amazon.com/blogs/compute/coming-soon-expansion-of-aws-lambda-states-to-all-functions/

Considering this statement below, I guess non-docker and non-VPC Lambda functions are currently updated synchronously without any delay.

Previously any function that was zip-file based and not attached to a VPC would only show an Active state.

BTW I was using hotswap with VPC Lambda, which I guess is why I'm experiencing the delay even in non-Docker functions!

But be careful, the blog also says there is a plan that all the functions will going to have equally the delay from February 1 2022.

So I guess we should use lambda.waitFor after updateFunctionCode to make sure code is updated and new revision is active for all the types(s3 and docker) of Lambda code. The only thing I'm afraid is that if it degrades cdk watch experience because each hotswap would require a few additional seconds. (I actually tried with waitFor enabled but it feels rather slow to complete...)

edit) Calling updateFunctionCode while a previous update is in progress will fail, so we should wait until an update completes anyway. At the same time, I'm afraid it might be a bit inefficient to wait for seconds per update when there're many functions to be hotswapped. But maybe we can optimize them if needed in another PR later.

@mergify mergify bot dismissed skinny85’s stale review January 12, 2022 14:17

Pull request has been modified.

@skinny85
Copy link
Contributor

Thanks for all of the information @tmokmss, that's super, super valuable!

I agree with your conclusion: let's only worry about Docker image hotswapping in this PR, and we'll tackle waiting for the update to complete in a separate PR. I'll create an issue to track that work.

@tmokmss tmokmss changed the title feat(cli): Support hotswap for Lambda code with InlineCode and DockerImageCode feat(cli): Support hotswap for Lambda code with DockerImageCode Jan 13, 2022
@tmokmss
Copy link
Contributor Author

tmokmss commented Jan 13, 2022

Hi @skinny85, I fixed tests and also opened a PR for InlineCode hotswap. I hope you can check them when you get a chance 🙏

skinny85
skinny85 previously approved these changes Jan 13, 2022
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Looks fantastic @tmokmss, short and super clean! Thanks for the contribution.

@skinny85
Copy link
Contributor

Ah, one last detail - can you update the ReadMe of the package with this new capability? In this chapter: https://github.com/aws/aws-cdk/blob/master/packages/aws-cdk/README.md#hotswap-deployments-for-faster-development

@tmokmss
Copy link
Contributor Author

tmokmss commented Jan 14, 2022

@skinny85 I'm not very sure what words to add since it already covers Lambda code hotswap capability, maybe like this?

- Code asset and tag changes of AWS Lambda functions.
+ Code asset (including Docker Lambda) and tag changes of AWS Lambda functions.

or maybe adding pr-linter/exempt-readme label?

@skinny85
Copy link
Contributor

@skinny85 I'm not very sure what words to add since it already covers Lambda code hotswap capability, maybe like this?

- Code asset and tag changes of AWS Lambda functions.
+ Code asset (including Docker Lambda) and tag changes of AWS Lambda functions.

or maybe adding pr-linter/exempt-readme label?

How about Code asset (including Docker image) and tag changes of AWS Lambda functions.?

@mergify mergify bot dismissed skinny85’s stale review January 14, 2022 01:31

Pull request has been modified.

@skinny85 skinny85 changed the title feat(cli): Support hotswap for Lambda code with DockerImageCode feat(cli): support hotswapping Lambda functions that use Docker images Jan 14, 2022
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @tmokmss!

@mergify
Copy link
Contributor

mergify bot commented Jan 14, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 8703071
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 6b553b7 into aws:master Jan 14, 2022
@mergify
Copy link
Contributor

mergify bot commented Jan 14, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify bot pushed a commit that referenced this pull request Jan 19, 2022
)

Similarly to #18319, this PR supports hotswap of Lambda functions that use `InlineCode`.

`InlineCode` uses [CloudFormation `ZipFile` feature](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-lambda-function-code.html#:~:text=requires%3A%20No%20interruption-,ZipFile,-\(Node.js%20and). To emulate its behavior, we create a zip file of the provided inline code with its filename `index.js` or `index.py` according to the runtime (CFn only supports python or nodejs for ZipFile), and pass the file's binary buffer to Lambda API.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@jeromevdl
Copy link

jeromevdl commented Jan 25, 2022

@tmokmss Do you have any sample on how to use this? I'm using code: lambda.DockerImageCode.fromImageAsset() but hotswap is not working. You mention ImareUri in the PR, does it mean I should use lambda.DockerImageCode.fromEcr() instead ? In this case, I need to build and push the docker image to ECR myself.

@tmokmss
Copy link
Contributor Author

tmokmss commented Jan 26, 2022

Hi @jeromevdl I confirmed it works with lambda.DockerImageCode.fromImageAsset, on v1.140.0 (it's not released on v2 yet).

e.g.

    new lambda.DockerImageFunction(this, 'Handler', {
      code: lambda.DockerImageCode.fromImageAsset('lambda', {
      }),
    });

What cdk diff output do you get before you try to hotswap?

@jeromevdl
Copy link

Thanks for the answer, I'm on CDK v2... Any idea when it will be released?

@tmokmss
Copy link
Contributor Author

tmokmss commented Jan 26, 2022

Considering the last v2 release was two weeks ago, it should be soon:) (I'm not very sure though as I'm just a contributor

@jeromevdl
Copy link

Considering the last v2 release was two weeks ago, it should be soon:) (I'm not very sure though as I'm just a contributor

Are you sure you're just a simple contributor? ;-) It was just released, thank you!

@tmokmss tmokmss deleted the hotswap_docker_lambda branch January 26, 2022 23:34
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
aws#18319)

closes aws#18302

We must just update `ImageUri` with the new ECR image url.

PR for `InlineCode` hotswap: aws#18408
 
`----`

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
…#18408)

Similarly to aws#18319, this PR supports hotswap of Lambda functions that use `InlineCode`.

`InlineCode` uses [CloudFormation `ZipFile` feature](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-lambda-function-code.html#:~:text=requires%3A%20No%20interruption-,ZipFile,-\(Node.js%20and). To emulate its behavior, we create a zip file of the provided inline code with its filename `index.js` or `index.py` according to the runtime (CFn only supports python or nodejs for ZipFile), and pass the file's binary buffer to Lambda API.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package/tools Related to AWS CDK Tools or CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(cli): Support hotswap for Docker Image Lambda function
5 participants