-
Notifications
You must be signed in to change notification settings - Fork 4k
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(lambda): support for Lambda SnapStart #23196
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an integ test for this change.
@@ -3152,6 +3152,29 @@ test('set SnapStart to desired value', () => { | |||
}); | |||
}); | |||
|
|||
test('enable SnapStart on L2 lambda construct', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test('enable SnapStart on L2 lambda construct', () => { | |
test('function using SnapStart', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an integ test for this change.
Any suggestion how this can be done ? To generate the snapshot I need a java zip file for the lambda function. Do cloudFormation/lambda services currently have a validation to check the runtime for snapStart ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added integ tests for this change. Things should be better now ! Could you please review the PR whenever possible ?
0c60b9c
to
d22f74d
Compare
da7803e
to
526f138
Compare
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
Pull request has been modified.
## Lambda with SnapStart | ||
|
||
```ts | ||
const fn = new lambda.Function(this, 'MyFunction', { | ||
code: lambda.Code.fromAsset('handler.zip'), | ||
runtime: lambda.Runtime.JAVA_11, | ||
handler: 'example.Handler::handleRequest', | ||
snapStart: true, | ||
}); | ||
``` | ||
|
||
See [the AWS documentation](https://docs.aws.amazon.com/lambda/latest/dg/snapstart.html) to learn more about AWS Lambda SnapStart | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK, Lambda SnapStart only works with published versions. If we just create a lambda function like this we only get $latest version, which I am afraid is not supported by SnapStart. Maybe we should create a lambda.Version
for the function afterwards and address this in the readme?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, SnapStart only supports JAVA_11 runtime atm. Worth a notice in the README or/and doc string in the snapStart
prop. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, thanks for your contribution. I can't wait to see it being officially supported as L2 from the community contribution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, snapStart is put into effect only on published versions. Using function.currentVersion as it's the recommended practice rather than managing lambda versions through lambda.Version
Added java 11 runtime information to both README and doc string above snap start
snapStart: true, | ||
}); | ||
|
||
const version = fn.currentVersion; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Good to know fn.currentVersion will actually create a new version if no existing version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually another great reason for this to not be a prop. For this to be a prop, you should be able to create a brand new Lambda function with this prop enabled, and the deployment should succeed.
This won't work though, because to enable start, a version has to already be published, and that needs an existing function. Normally, CFN can create the lambda function and the version at once since it can figure out the dependency, but this is a circular dependency; the Version depends on the Function, but the Function also depends on the Version because snap start is a property on the function itself.
(unless of course CFN figures this out for us, which I will check manually later, but it's not clear right now).
So, in summary: we need a method enableSnapStart()
that does this error checking and clearly documents the restrictions, including the restriction that a lambda function must already have been deployed to enable this (assuming this restriction actually exists, again I'll verify this later).
@comcalvi, Could you please review the changes ? |
@Naumel , could you please take a look at this PR ? Requesting as you had reviewed my previous PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution! The quality of this contribution is fantastic, but I'm concerned about the limitations of SnapStart. Per the documentation this is only available in limited cases so I don't think adding it as a prop to FunctionOptions
is the right level of abstraction here.
I'm still working my way through the documentation on this so I'm not sure exactly what the right user experience is, but I don't think adding the prop here is the right approach.
My understanding so far is that this is functionality that can be added to existing functions. Given that, this seems like it should potentially be a function that gets called on the lambda function instead.
I'm still looking into this but wanted to provide my initial feedback in a timely manner.
Please make sure that your PR body describes the problem the PR is solving, and the design approach and alternatives considered. Explain why the PR solves the problem. A link to an issue is helpful, but does not replace an explanation of your thought process (See Contributing Guide, Pull Requests) |
@@ -36,4 +36,12 @@ alias.addFunctionUrl({ | |||
// to validate the changed function hash. | |||
cdk.Aspects.of(stack).add(new lambda.FunctionVersionUpgrade(LAMBDA_RECOGNIZE_LAYER_VERSION)); | |||
|
|||
// Integ test for lambda with snapStart |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Integ test for lambda with snapStart |
code: lambda.Code.fromAsset('handler.zip'), | ||
runtime: lambda.Runtime.JAVA_11, | ||
handler: 'example.Handler::handleRequest', | ||
snapStart: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I concur with @TheRealAmazonKendra. There's too many restrictions on the lambda to put this as a constructor prop (from https://docs.aws.amazon.com/lambda/latest/dg/snapstart.html):
SnapStart supports the Java 11 runtime.
SnapStart does not support provisioned concurrency, the arm64 architecture, the Lambda Extensions API, Amazon Elastic File System (Amazon EFS), AWS X-Ray, or ephemeral storage greater than 512 MB.
it's good that you added error checking for the Java11 runtime case, but all of these other restrictions must also be checked at synth time. This bloats the constructor with unnecessary checks that will eventually be removed when Lambda rolls this out to more function types.
Because we need those type checks today, and we will one day have to deprecate them, I agree with @TheRealAmazonKendra's suggestion to make this a new method (maybe enableSnapStart()
?) on the Function
class that will do all of this error checking in one place.
snapStart: true, | ||
}); | ||
|
||
const version = fn.currentVersion; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually another great reason for this to not be a prop. For this to be a prop, you should be able to create a brand new Lambda function with this prop enabled, and the deployment should succeed.
This won't work though, because to enable start, a version has to already be published, and that needs an existing function. Normally, CFN can create the lambda function and the version at once since it can figure out the dependency, but this is a circular dependency; the Version depends on the Function, but the Function also depends on the Version because snap start is a property on the function itself.
(unless of course CFN figures this out for us, which I will check manually later, but it's not clear right now).
So, in summary: we need a method enableSnapStart()
that does this error checking and clearly documents the restrictions, including the restriction that a lambda function must already have been deployed to enable this (assuming this restriction actually exists, again I'll verify this later).
Hi @comcalvi From what I have tested using CDK L1 and observed from the console, when you create a AWS::Lambda::Function with the SnapStart prop enabled with JAVA 11 runtime, you literally can deploy this Lambda function with SnapStart capability enabled. However, the $latest version of the lambda function at this moment will not benefit from the SnapStart capability, it only happens when you publish a new version from it afterwards that will have the SnapStart capabilities. When you publish a new version from a SnapStart enabled function, it takes additional time(a few minutes) processing before the version is ready to serve the traffic and when this lambda version is invoked for the first time it takes very little time to restore the snap rather then traditional cold start and that is how it gets speed boosted compared to normal lambda cold start. That being said, You are allowed to publish a lambda function with SnapStart enabled but only lambda versions published afterwards from it will benefit. From the author's API designed: // this will create a lambda function with SnapStart turned on
// but traffic hitting this function $latest version will not benefit
const fn = new lambda.Function(this, 'Func', {
...
snapStart: true,
};
// this behind the scene should create a new lambda.Version from the function above and will
// have the SnapStart advantages when traffic hit this lambda version
const version = fn.currentVersion I kind of agree I'm also afraid the snapStart: props.snapStart ? this._enableSnapStart() : undefined And we use |
Wild idea: Would the UX be better if we had a separate // this will create a lambda function with SnapStart turned on
// but traffic hitting this function $latest version will not benefit
const snappy = new lambda.SnapStartFunction(this, 'Func', {
...
});
// this behind the scene should create a new lambda.Version from the function above and will
// have the SnapStart advantages when traffic hit this lambda version
const version = snappy.currentVersion; Potentially you have a slightly odd prop |
I was actually contemplating this same idea. My hangup was two-fold. The first is the use case you mentioned of turning off SnapStart. The second is extensibility. Given that the allowed values for this setting in CFN are not just true/false, and instead |
Ahh okay, I'm happy to defer to this then. That means that we can create a snap start lambda and have it work in one deployment, so long as we publish a version for them. I'm still in favor of the method over the prop because this is finicky enough.
While I agree that users should understand that enabling snap start requires a published version, allowing them to provision a "snap start enabled" lambda that doesn't actually use snap start doesn't help them understand the publishing version requirements. Instead, it looks like a bug (and that's because it is a bug). It's a better dev experience to publish the version for them whenever snap start is enabled.
I don't agree with this, because it's a silent failure. If a user passes It's much more confusing to tell your deployment tool (CDK) that you want snap start enabled and it said "OK, here's your snap start lambda" but then when they go to the console it turns out that snap start wasn't actually enabled. |
While the L1 and CFN may allow silent failures in this case, I don't think we should. Deploying something that doesn't actually do the thing the user thinks it will without a failure message is not a good user experience. We should not have silent failures in general. I disagree that it should be the user's responsibility to know that only published versions will benefit from this and that they would have to take extra steps to enable something that's a universal prop in this construct. That's a rather poor user experience. |
If we want to do this, I won't block it, but I'm more in favor of the method. I would fully support the creation of a new construct if the restrictions on snap start functions prevented them from integrating with other services in the same way in which regular lambdas do (eg if a snap start couldn't be used with api gateway or event bridge or something like that) but given that the restrictions only affect the lambda itself, adding just one method is enough to handle the error checking. |
There are additional limitations to take into account for this as well, this is not currently available in all commercial regions, per https://aws.amazon.com/blogs/aws/new-accelerate-your-lambda-functions-with-lambda-snapstart/
|
And I'm not entirely convinced either. The API design for it is a bit baffling to be honest. I guess the new construct would still expose a prop I fall a bit more in the camp of it being a users responsibility to know they have to create a version. I think we could navigate this with clever naming, e.g. call it Would be good to understand the future plans for this, maybe we can reach out internally. |
I reached out to the lambda team yesterday. The contact for this information will be out for a few more days so we should probably put this PR on hold for the moment. |
@comcalvi , I have been stuck at work for the past week, couldn't follow up sooner. From the discussions above, can I conclude that what we are expecting is an enableSnapStart() method on the Function class that performs the validations and also publishes the lambda for the user ? |
yes, but please wait for @TheRealAmazonKendra to communicate the Lambda team's update before continuing. We need to understand the direction they plan to take this in so we can make the best decision. |
Sounds good. Let’s wait for the updates |
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
Closes #23153 - Support SnapStart on L2 constructs by setting `snapStart: lambda.SnapStartConfig.ON_PUBLISHED_VERSIONS`. This value will be converted to `SnapStart: {ApplyOn: 'PublishedVersions',}` in template. - Supports checking limitation stated in [SnapStart](https://docs.aws.amazon.com/lambda/latest/dg/snapstart.html) except for Provisioned Concurrency which is at version level and can't be checked with function level props. - This PR is heavily inspired by [PR](#23196) from @DeerajTheepshi and [discussion](#23153 (comment)) in the issue, Thank you ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Fixes #23153
All Submissions:
Adding new Construct Runtime Dependencies:
New Features
yarn integ
to deploy the infrastructure and generate the snapshot (i.e.yarn integ
without--dry-run
)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license