feat(lambda): L2 constructs for SnapStart by roger-zhangg · Pull Request #26761 · aws/aws-cdk

roger-zhangg · 2023-08-15T17:41:35Z

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 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 from @DeerajTheepshi and discussion 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

aws-cdk-automation

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.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

the original handler.zip file is corrupted so added a new one

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

roger-zhangg · 2023-08-15T22:21:31Z

added a handler-snapstart.zip in integration test for the original handler.zip seems corrupted. Can we confirm if this make sense, then I'll replace the handler.zip with this new zip

unzip test/aws-lambda/test/handler.zip -d ~/tmp
Archive:  test/aws-lambda/test/handler.zip
error [test/aws-lambda/test/handler.zip]:  missing 12435228 bytes in zipfile
  (attempting to process anyway)
error [test/aws-lambda/test/handler.zip]:  start of central directory not found;
  zipfile corrupt.
  (please check that you have transferred or created the zipfile in the
  appropriate BINARY mode and that you have compiled UnZip properly)

roger-zhangg · 2023-08-15T23:06:25Z

Got some strange 503 error in codebuild

aws-cdk: �[2K�[1Gaws-cdk: �[2m$ cdk-build�[22m
cdk-assets: �[2K�[1Gcdk-assets: �[1myarn run v1.22.19�[22m
cdk-assets: �[2K�[1G�[2m$ cdk-test�[22m
aws-cdk: curl: (22) The requested URL returned error: 503 
aws-cdk: Error: ./generate.sh exited with error code 22
aws-cdk: Build failed.!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

link to the original log

Doesn't have any problem with yarn build locally. Can anyone help to take a look?

packages/aws-cdk-lib/aws-lambda/lib/function.ts

packages/aws-cdk-lib/aws-lambda/lib/snapstart-config.ts

packages/aws-cdk-lib/aws-lambda/test/function.test.ts

roger-zhangg · 2023-08-15T23:55:25Z

packages/aws-cdk-lib/aws-lambda/lib/function.ts

      codeSigningConfigArn: props.codeSigningConfig?.codeSigningConfigArn,
      architectures: this._architecture ? [this._architecture.name] : undefined,
      runtimeManagementConfig: props.runtimeManagementMode?.runtimeManagementConfig,
+      snapStart: this.configureSnapStart(props),


Thanks, wil check this out

packages/aws-cdk-lib/aws-lambda/lib/function.ts

colifran

@roger-zhangg this is looking pretty good so far! See my comments and let me know if you have any questions.

Pull request has been modified.

roger-zhangg · 2023-08-16T21:21:38Z

@colifran I've updated per our discussion, please take a look when you have time, thanks!

colifran · 2023-08-16T22:07:18Z

packages/aws-cdk-lib/aws-lambda/README.md

+  code: lambda.Code.fromAsset(path.join(__dirname, 'handler.zip')),
+  runtime: lambda.Runtime.JAVA_11,
+  handler: 'example.Handler::handleRequest',
+  snapStart: lambda.SnapStartConfig.ON_PUBLISHED_VERSIONS,


@roger-zhangg in case you didn't catch this from the build logs, it looks like the build is failing because you changed the class to SnapStartConf, but didn't update it here.

Thanks for the catch, I'll update it

Also I wonder is there a local equivalent to this cloud build? So I won't miss next time

Not that I'm aware of. If I have a failing build I always use the build logs we generate to understand why it's failing. In this case, at the bottom of the build logs it pointed to the README so I knew to check there.

roger-zhangg · 2023-08-17T23:21:30Z

Thanks Colin, I think I got what you meant. I'll check out the code first and get to you if I have any problem

valerena · 2023-08-18T00:09:14Z

@roger-zhangg sorry to be doing this now, but there was some additional context I wasn't aware of regarding the first go at implementing this. Specifically, we were against adding SnapStart as a property on FunctionOptions. The reason for this is that from the perspective of a user, they would see this and want to use it not necessarily knowing all the other requirements such as runtime, architecture, storage size restrictions, etc. The direction we wanted to move this in was to implement SnapStart as a method on the Functionconstruct. This could then be used as: lambdaFunction.enableSnapStart(). This method would then be responsible for performing the necessary validation and enabling SnapStart on the function. Again sorry for recognizing this late. @TheRealAmazonKendra can provide some more context if necessary as they were involved on the previous PR. In the meantime, I'll also think about how this method would work. Let me know if you have any questions.

What's the difference in practice with having "a method" instead of having the validation as part of the constructor? We took inspiration from the existing behavior for the VPC Configuration vpcConfig field, which also has a lot of restrictions and validation.

valerena · 2023-08-18T00:26:58Z

My main concern about that is: how do customers know what is a property and what is a method? (of course, they will have to look at the documentation). Is there any other property that exists directly in a Lambda function via the API that in CDK is not a field in the constructor? Just so we don't add this as an "exception" from all the other fields that are directly supported.

aws-cdk-automation · 2023-08-20T00:08:45Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-08-21T00:08:16Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-08-22T00:08:20Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

roger-zhangg · 2023-08-22T16:51:47Z

Hi @colifran , do you have a chance to look into @valerena comments? Thanks!

colifran · 2023-08-22T17:09:39Z

@roger-zhangg @valerena sorry for the delay. I will get back to you on this today.

@roger-zhangg @valerena I've spent some more time thinking about this and I'm not entirely sure what advantage we gain by making this a method vs. just adding it as a property. After re-reading the initial PR it seems like one of the concerns with making this a property was that there was an assumption that to enable SnapStart, a version has to already be published or the deployment would fail. It looks like further down this was found to not be the case and instead the $latest version of the lambda function would not benefit from SnapStart, but when you publish a new version from it afterwards it will have SnapStart capabilities. As @valerena mentions, do we really gain anything from having this as a method with verification vs. adding this as a property and performing the validation when we configure SnapStart? It seems like we're just moving the validation which is going to be throwing an error one way or another if the configured lambda function doesn't support SnapStart. The current implementation mirrors configureVpc. It looks like we're doing similar verification with code guru and we have added a Runtime property to help determine whether a specific Runtime supports code guru profiling. I'd still like some additional eyes on this - @mrgrain @TheRealAmazonKendra your thoughts on this would be appreciated.

aws-cdk-automation · 2023-08-23T00:08:20Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

colifran · 2023-08-23T21:58:49Z

packages/aws-cdk-lib/aws-lambda/lib/function.ts

+      throw new Error('SnapStart currently only support published versions');
+    }
+
+    if (props.runtime != Runtime.JAVA_11 && props.runtime != Runtime.JAVA_17 ) {


@roger-zhangg looks like we're still waiting on some feedback, but pending that the only other change I would make here is to update our Runtime class to have supportsSnapStart as a LambdaRuntimeProp. Then we can add supportsSnapStart as true to any relevant Runtime when it gets added in the future:

aws-cdk/packages/aws-cdk-lib/aws-lambda/lib/runtime.ts

Lines 3 to 26 in 5fef403

export interface LambdaRuntimeProps {

/**

* Whether the ``ZipFile`` (aka inline code) property can be used with this runtime.

* @default false

*/

readonly supportsInlineCode?: boolean;

/**

* Whether the runtime enum is meant to change over time, IE NODEJS_LATEST.

* @default false

*/

readonly isVariable?: boolean;

/**

* The Docker image name to be used for bundling in this runtime.

* @default - the latest docker image "amazon/public.ecr.aws/sam/build-<runtime>" from https://gallery.ecr.aws

*/

readonly bundlingDockerImage?: string;

/**

* Whether this runtime is integrated with and supported for profiling using Amazon CodeGuru Profiler.

* @default false

*/

readonly supportsCodeGuruProfiling?: boolean;

We could then change this check to if (!props.runtime.supportsSnapStart) { //... }

mrgrain · 2023-08-23T22:32:55Z

I think a prop with validation is fine.
We should however add a acknowledgable warning (addWarningV2) that informs users about the need to create a version to get any benefit from it.
See: #26144

aws-cdk-automation · 2023-08-24T00:07:55Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

Pull request has been modified.

roger-zhangg · 2023-08-24T17:52:38Z

Hey @colifran @mrgrain Thank you all for your comments, I've updated the code per your suggestions. Please help to take another look when you are available. Thanks!

colifran · 2023-08-24T18:31:28Z

packages/aws-cdk-lib/aws-lambda/lib/function.ts

+    // so it can't be checked at function set up time
+    // SnapStart supports the Java 11 and Java 17 (java11 and java17) managed runtimes.
+    // See https://docs.aws.amazon.com/lambda/latest/dg/snapstart.html
+    if (props.snapStart != SnapStartConf.ON_PUBLISHED_VERSIONS) {


Last thing and then we can approve this. Can we remove this? The only way a user can use SnapStartConf is as SnapStartConf.ON_PUBLISHED_VERSIONS

colifran

@roger-zhangg Looks good!

Pull request has been modified.

aws-cdk-automation · 2023-08-24T20:30:31Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 477188f
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2023-08-24T20:31:59Z

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

mriccia · 2023-08-29T09:58:36Z

It is great to have SnapStart support with CDK, thank you for this feature.

I have a question about this feature implementation. Looking through the PR it seems that there is no check verifying that Lambda Versioning is enabled.

You can use SnapStart only on published function versions and aliases that point to versions. You can't use SnapStart on a function's unpublished version ($LATEST). Source

IMO it would make sense to either print out a warning, or even raise an error if SnapStart is configured without Versions being enabled. Would it be possible to consider this improvement?

valerena · 2023-08-29T15:41:29Z

@mriccia One thing to know is that even though SnapStart works on published versions, the configuration is at the function level, and it will mean that all future versions of the function will have SnapStart enabled. So you don't really need to have versioning enabled right now for this to come into play in the future (and it doesn't hurt to activate it before having any version).

Having said that, a printed warning related to this was added, not checking the current versioning, but for users to be aware in general: https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-lambda/lib/function.ts#L1316

Does that make sense to you?

mriccia · 2023-08-30T08:01:09Z

That's great @valerena, yes this warning was what I was looking for (apologies for not spotting it in the code!).

roger-zhangg and others added 7 commits August 11, 2023 22:57

add snapstart in L2 construnct

362479d

fix type

400a97b

fix test err

75960e3

add test, doc, comment

19cc504

add test

5ff6bd1

address few problem per Renato's feedback

7c7ad7c

Merge branch 'aws:main' into snapstart_L2

f2e2907

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Aug 15, 2023

aws-cdk-automation requested a review from a team August 15, 2023 17:41

aws-cdk-automation previously requested changes Aug 15, 2023

View reviewed changes

add integration test and snapshot.

0d57360

the original handler.zip file is corrupted so added a new one

Merge branch 'main' into snapstart_L2

09f87eb

fix lint

1714849

colifran self-requested a review August 15, 2023 23:17

valerena reviewed Aug 15, 2023

View reviewed changes

packages/aws-cdk-lib/aws-lambda/lib/function.ts Outdated Show resolved Hide resolved

colifran reviewed Aug 15, 2023

View reviewed changes

packages/aws-cdk-lib/aws-lambda/lib/snapstart-config.ts Outdated Show resolved Hide resolved

colifran reviewed Aug 15, 2023

View reviewed changes

packages/aws-cdk-lib/aws-lambda/test/function.test.ts Show resolved Hide resolved

roger-zhangg commented Aug 15, 2023

View reviewed changes

colifran reviewed Aug 15, 2023

View reviewed changes

packages/aws-cdk-lib/aws-lambda/lib/function.ts Outdated Show resolved Hide resolved

colifran previously requested changes Aug 15, 2023

View reviewed changes

refactor to abstract&render

427878e

colifran reviewed Aug 16, 2023

View reviewed changes

colifran reviewed Aug 23, 2023

View reviewed changes

Merge branch 'main' into snapstart_L2

e552e2d

add snapstart runtime check, add warning

e1ce197

colifran reviewed Aug 24, 2023

View reviewed changes

remove unnecessary snapstart value check

8bad263

colifran previously approved these changes Aug 24, 2023

View reviewed changes

Merge branch 'main' into snapstart_L2

477188f

colifran approved these changes Aug 24, 2023

View reviewed changes

mergify bot merged commit 356f302 into aws:main Aug 24, 2023

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Aug 24, 2023

github-actions bot mentioned this pull request Sep 1, 2023

Monthly PRs metrics report - Aug 2023 #26965

Closed

	export interface LambdaRuntimeProps {
	/**
	* Whether the ``ZipFile`` (aka inline code) property can be used with this runtime.
	* @default false
	*/
	readonly supportsInlineCode?: boolean;

	/**
	* Whether the runtime enum is meant to change over time, IE NODEJS_LATEST.
	* @default false
	*/
	readonly isVariable?: boolean;

	/**
	* The Docker image name to be used for bundling in this runtime.
	* @default - the latest docker image "amazon/public.ecr.aws/sam/build-<runtime>" from https://gallery.ecr.aws
	*/
	readonly bundlingDockerImage?: string;

	/**
	* Whether this runtime is integrated with and supported for profiling using Amazon CodeGuru Profiler.
	* @default false
	*/
	readonly supportsCodeGuruProfiling?: boolean;

Conversation

roger-zhangg commented Aug 15, 2023

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

roger-zhangg commented Aug 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roger-zhangg commented Aug 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

roger-zhangg Aug 15, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

colifran left a comment

Choose a reason for hiding this comment

Uh oh!

roger-zhangg commented Aug 16, 2023

Uh oh!

colifran Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

roger-zhangg Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

roger-zhangg Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

colifran Aug 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roger-zhangg commented Aug 17, 2023

Uh oh!

valerena commented Aug 18, 2023

Uh oh!

valerena commented Aug 18, 2023

Uh oh!

aws-cdk-automation commented Aug 20, 2023

Uh oh!

aws-cdk-automation commented Aug 21, 2023

Uh oh!

aws-cdk-automation commented Aug 22, 2023

Uh oh!

roger-zhangg commented Aug 22, 2023

Uh oh!

colifran commented Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aws-cdk-automation commented Aug 23, 2023

Uh oh!

colifran Aug 23, 2023

Choose a reason for hiding this comment

Uh oh!

mrgrain commented Aug 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aws-cdk-automation commented Aug 24, 2023

Uh oh!

roger-zhangg commented Aug 24, 2023

Uh oh!

colifran Aug 24, 2023

Choose a reason for hiding this comment

Uh oh!

colifran left a comment

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Aug 24, 2023

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Aug 24, 2023

Uh oh!

mriccia commented Aug 29, 2023

Uh oh!

valerena commented Aug 29, 2023

roger-zhangg commented Aug 15, 2023 •

edited

Loading

roger-zhangg commented Aug 15, 2023 •

edited

Loading

colifran Aug 16, 2023 •

edited

Loading

colifran commented Aug 22, 2023 •

edited

Loading

mrgrain commented Aug 23, 2023 •

edited

Loading