fix(custom-resources): handle Inactive lambda functions #20922

comcalvi · 2022-06-29T20:58:43Z

all lambda functions can become inactive eventually. This will result in invocations failing. This PR adds logic to retry the invocation up to a maximum of ten minutes and some change. This number was chosen because a lambda only runs for 15 minutes, and 10 seemed like a good number.

Closes #20123.

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use 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

gitpod-io · 2022-06-29T20:58:50Z

packages/@aws-cdk/custom-resources/lib/provider-framework/runtime/framework.ts

rix0rrr · 2022-06-30T12:58:03Z

packages/@aws-cdk/custom-resources/lib/provider-framework/runtime/framework.ts

+
+        log('user function is still being initialized by Lambda, retrying with delay of: ', newDelay);
+
+        return setTimeout(invokeUserFunction, newDelay, functionArnEnv, payload, newRetryOptions);


This is not as readable as a for loop with a sleep.

function sleep(ms: number): Promise<void> { return new Promise<void>(ok => window.setTimeout(ok, ms)); }

You can call await sleep(10_000) on that.

rix0rrr

Still changes requested, getting it out of my queue

… its state

TheRealAmazonKendra · 2022-08-04T20:05:20Z

packages/@aws-cdk/custom-resources/lib/provider-framework/runtime/framework.ts

@@ -13,6 +13,9 @@ export = {
  [consts.FRAMEWORK_ON_TIMEOUT_HANDLER_NAME]: onTimeout,
 };

+const BASE_SLEEP = 10_000;
+const MAX_TOTAL_SLEEP = 620_000;


I apparently already forgot about our conversation on the duplicate PR because I was about to be like, why this number? 😂

Rereading that, however, made me realize that the ordering on these checks might be somewhat off. If the lambda. is called while it is in inactive or pending state, the invocation will simply fail so a new invocation will need to be called instead of giving time for the current invocation to pass. Basically, I think that invokeUserFunction may need to have the following workflow:

Within a while loop `await invokeFunction` -> `await getFunctionResponse` -> if `Configuration. LastUpdateStatus.failure && Configuration.State === inactive or pending` -> retry else if -> `Configuration. LastUpdateStatus.failure` i.e. it's status is active or failed -> get and throw error error (break) else -> return payload (break)

The above could be a do-while loop so that you don't have to use breaks but obviously the actual implementation is up to you.

Also, regarding this number, do we know in general how long it usually takes a function to return to active? That data might be good in determining this number.

Unfortunately, we do not at this time. I should have lambdas that will become inactive in the next week or so (someone from lambda told me offline that this is ~30 days), so I can check once I have an inactive function.

My original assessment was only partially right. I was under the impression that the Lamba function failed, not errored, but upon further digging, I see that it throws an exception. See outbound.ts for my revised suggestion here.

TheRealAmazonKendra · 2022-08-04T20:19:52Z

packages/@aws-cdk/custom-resources/lib/provider-framework/runtime/framework.ts

+      });
+
+      if (!(getFunctionResponse.Configuration?.State === 'Inactive' || getFunctionResponse.Configuration?.State === 'Pending')) {
+        break;


Doesn't this just break the while loop and not the if statement here? If so, this will still continue on to log that the error was thrown and then throw the error even though the lambda has now succeeded (presumably).

ahh good catch, you're right

packages/@aws-cdk/custom-resources/test/provider-framework/mocks.ts

TheRealAmazonKendra · 2022-08-04T22:17:30Z

packages/@aws-cdk/custom-resources/lib/provider-framework/runtime/framework.ts

+  // parse function name from arn
+  // arn:${Partition}:lambda:${Region}:${Account}:function:${FunctionName}
+  const arn = functionArn.split(':');
+  const functionName = arn[arn.length - 1];


Is there a circumstance where this lambda might have a version added to the end of it or are our custom resources using unqualified ARNs?

The user function is under the user's control, so they can create versions as they please. However, getFunction() will happily accept a name with a :version-string at the end as well, so we're safe in either case.

The issue here is that if they do provide us with one that has a version at the end, arn[arn.length - 1] will pick up the version number instead of the function name, since it would be arn:${Partition}:lambda:${Region}:${Account}:function:${FunctionName}:${Version}

TheRealAmazonKendra · 2022-08-04T22:20:45Z

packages/@aws-cdk/custom-resources/lib/provider-framework/runtime/framework.ts

+    while (totalSleep <= MAX_TOTAL_SLEEP) {
+      // if the user's lambda has become Inactive, we must retry the invocation until Lambda finishes provisioning resources for it.
+      const getFunctionResponse = await getFunction({
+        FunctionName: functionName,


Looks like we can just provide the ARN here instead of trying to parse per GetFunction Documentation.

We can, but we need the name later anyway for the logs (line 154 in this file with these changes), and imo passing the name is cleaner. If you prefer though we can change this to the ARN

TheRealAmazonKendra · 2022-08-08T23:03:15Z

packages/@aws-cdk/custom-resources/lib/provider-framework/runtime/framework.ts

+  // parse function name from arn
+  // arn:${Partition}:lambda:${Region}:${Account}:function:${FunctionName}
+  const arn = functionArn.split(':');
+  const functionName = arn[arn.length - 1];


The issue here is that if they do provide us with one that has a version at the end, arn[arn.length - 1] will pick up the version number instead of the function name, since it would be arn:${Partition}:lambda:${Region}:${Account}:function:${FunctionName}:${Version}

TheRealAmazonKendra · 2022-08-10T02:37:13Z

packages/@aws-cdk/custom-resources/lib/provider-framework/runtime/framework.ts

@@ -13,6 +13,9 @@ export = {
  [consts.FRAMEWORK_ON_TIMEOUT_HANDLER_NAME]: onTimeout,
 };

+const BASE_SLEEP = 10_000;
+const MAX_TOTAL_SLEEP = 620_000;


My original assessment was only partially right. I was under the impression that the Lamba function failed, not errored, but upon further digging, I see that it throws an exception. See outbound.ts for my revised suggestion here.

TheRealAmazonKendra · 2022-08-10T02:49:14Z

packages/@aws-cdk/custom-resources/lib/provider-framework/runtime/outbound.ts

+async function defaultGetFunction(req: AWS.Lambda.GetFunctionRequest): Promise<AWS.Lambda.GetFunctionResponse> {
+  if (!lambda) {
+    lambda = new AWS.Lambda(awsSdkConfig);
+  }
+
+  return lambda.getFunction(req).promise();
+}
+


So, according to the documentation, we can run into a situation where new lamba functions also haven't finished provisioning before we try to invoke them so I wonder if the right path forward here is to replace `defaultInvokeFunction with the following (roughly, this also contains notes):

async function defaultInvokeFunction(req: AWS.Lambda.InvocationRequest): Promise<AWS.Lambda.InvocationResponse> { if (!lambda) { lambda = new AWS.Lambda(awsSdkConfig); } let status = await defaultGetFunction(req); if (status.Configuration?.State !== 'Failed') { try { // If it is inactive, pending, or failed it will throw instead of returning return lambda.invoke(req).promise(); } catch (e) { // We only care about inactive or pending for retries while (status.Configuration?.State === 'Inactive' || status.Configuration?.State === 'Pending') { // Add wait of some sort in here if we want // Since we don't know how long the reprovisioning of resources will take, we shouldn't add in a max time. status = await defaultGetFunction(req); } } } // Now that it's either active or failed, return the terminal response, error or not return lambda.invoke(req).promise(); }

This would mean that we don't want to export defaultGetFunction as getFunction since the scope of its use is only in this file. This edit would require no change (I think) to the logic in invokeUserFunction() in framework.ts. Well, except that there's still the issue with the ARN that I commented on. Maybe we just never take in qualified ARNs, though, so maybe that isn't an issue. On the other hand, you could export getFunction and use that to make sure we're getting the correct function name. It's part of Configuration.

What do you think?

Naumel · 2022-09-12T13:37:59Z

@Mergifyio update

mergify · 2022-09-12T13:38:46Z

update

✅ Branch has been successfully updated

aws-cdk-automation · 2022-09-12T13:55:29Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: e8f3eac
Result: FAILED
Build Logs (available for 30 days)

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

TheRealAmazonKendra · 2022-11-02T16:53:11Z

Closing this as the work has been continued in another PR.

gshpychka · 2022-11-02T18:47:54Z

@TheRealAmazonKendra which one?

retry logic

a9c4d46

github-actions bot added the p2 label Jun 29, 2022

comcalvi added pr-linter/exempt-test The PR linter will not require test changes pr-linter/exempt-integ-test The PR linter will not require integ test changes and removed p2 labels Jun 29, 2022

comcalvi marked this pull request as draft June 29, 2022 20:59

aws-cdk-automation requested a review from a team June 29, 2022 20:59

mergify bot added the contribution/core This is a PR that came from AWS. label Jun 29, 2022

rix0rrr requested changes Jun 30, 2022

View reviewed changes

fixed small bug

6fbe8eb

comcalvi mentioned this pull request Jul 14, 2022

fix(custom-resources): inactive user function throws error that breaks provider framework #21095

Closed

4 tasks

rix0rrr requested changes Jul 18, 2022

View reviewed changes

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels Jul 19, 2022

comcalvi and others added 10 commits July 19, 2022 15:52

instead of checking error message, ping the lambda function and check…

d1fd547

… its state

added jitter to backoff

dee5710

test

701306b

mocks created

5b2a308

finalized testing

11505fd

refactor to use iteration instead of recursion

bc2eef6

newline

830686c

move code

ed3b5a7

merge conflict

4f7ebd1

Merge branch 'main' into customresource

a34e600

comcalvi removed pr-linter/exempt-test The PR linter will not require test changes pr-linter/exempt-integ-test The PR linter will not require integ test changes labels Aug 3, 2022

comcalvi marked this pull request as ready for review August 3, 2022 23:42

comcalvi requested a review from TheRealAmazonKendra August 4, 2022 16:56

TheRealAmazonKendra requested changes Aug 4, 2022

View reviewed changes

comcalvi added 5 commits August 4, 2022 16:05

fixed retry logic

6b0cf52

fixed testing

74f3dc5

elif

928fed7

fixed testing bug

0257e58

doc

9baa34a

TheRealAmazonKendra requested changes Aug 10, 2022

View reviewed changes

Merge branch 'main' into customresource

e8f3eac

TheRealAmazonKendra closed this Nov 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(custom-resources): handle Inactive lambda functions #20922

fix(custom-resources): handle Inactive lambda functions #20922

comcalvi commented Jun 29, 2022 •

edited

Loading

gitpod-io bot commented Jun 29, 2022

rix0rrr Jun 30, 2022

rix0rrr left a comment

TheRealAmazonKendra Aug 4, 2022

TheRealAmazonKendra Aug 4, 2022

TheRealAmazonKendra Aug 4, 2022

TheRealAmazonKendra Aug 4, 2022

comcalvi Aug 4, 2022

TheRealAmazonKendra Aug 10, 2022

TheRealAmazonKendra Aug 4, 2022

comcalvi Aug 4, 2022

TheRealAmazonKendra Aug 4, 2022

comcalvi Aug 4, 2022

TheRealAmazonKendra Aug 8, 2022

TheRealAmazonKendra Aug 4, 2022

comcalvi Aug 4, 2022 •

edited

Loading

TheRealAmazonKendra Aug 8, 2022

TheRealAmazonKendra Aug 10, 2022

TheRealAmazonKendra Aug 10, 2022

Naumel commented Sep 12, 2022

mergify bot commented Sep 12, 2022

aws-cdk-automation commented Sep 12, 2022

TheRealAmazonKendra commented Nov 2, 2022

gshpychka commented Nov 2, 2022


		log('user function is still being initialized by Lambda, retrying with delay of: ', newDelay);

		return setTimeout(invokeUserFunction, newDelay, functionArnEnv, payload, newRetryOptions);

fix(custom-resources): handle Inactive lambda functions #20922

fix(custom-resources): handle Inactive lambda functions #20922

Conversation

comcalvi commented Jun 29, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Jun 29, 2022

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comcalvi Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naumel commented Sep 12, 2022

mergify bot commented Sep 12, 2022

✅ Branch has been successfully updated

aws-cdk-automation commented Sep 12, 2022

AWS CodeBuild CI Report

TheRealAmazonKendra commented Nov 2, 2022

gshpychka commented Nov 2, 2022

comcalvi commented Jun 29, 2022 •

edited

Loading

comcalvi Aug 4, 2022 •

edited

Loading