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

fix(route53): timeouts due to delete-existing-record-set-handler's wait time #27060

Conversation

pistazie
Copy link

@pistazie pistazie commented Sep 8, 2023

This PR updates the wait time of the delete-existing-record-set-handler from 60 to 1800 seconds.

Commit 399b6bb upgraded the handler to use the AWS SDK V3 (was V2) and made the following change:

-  await route53.waitFor('resourceRecordSetsChanged', { Id: changeResourceRecordSets.ChangeInfo.Id }).promise();
+  await waitUntilResourceRecordSetsChanged({ client: route53, maxWaitTime: 60 }, { Id: changeResourceRecordSets?.ChangeInfo?.Id })

The V2 route53.waitFor waits for 1800 seconds (see reference)

Waits for the resourceRecordSetsChanged state by periodically calling the underlying [Route53.getChange()](https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/Route53.html#getChange-property) operation every 30 seconds (at most 60 times).

The new explicit configuration was set to 60 seconds which is too short and caused timeouts and failure in our cloudformation stacks


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

…onds to regression caused by AWS SDK V3 migration Regression introduced in 399b6bb
@github-actions github-actions bot added p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Sep 8, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team September 8, 2023 08:13
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a 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.

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

@pistazie pistazie changed the title Updates delete-existing-record-set-handler fix: timeouts due to delete-existing-record-set-handler's wait time Sep 8, 2023
@aws-cdk-automation aws-cdk-automation added pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. and removed pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. labels Sep 8, 2023
@mrgrain mrgrain added sdk-v3-upgrade Tag issues that are associated to SDK V3 upgrade. Not limited to CR usage of SDK only. node18-upgrade Any work (bug, feature) related to Node 18 upgrade labels Sep 8, 2023
@kaizencc kaizencc added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Sep 8, 2023
@kaizencc kaizencc changed the title fix: timeouts due to delete-existing-record-set-handler's wait time fix(route53): timeouts due to delete-existing-record-set-handler's wait time Sep 8, 2023
@kaizencc
Copy link
Contributor

kaizencc commented Sep 8, 2023

hi @pistazie I went to update the snapshot for you but it looks like I don't have permissions to update to the pitch-io fork.

Instead, I've created #27068. If you want, you can update the snapshot on this PR before the other one is merged and we'll go with yours. Otherwise, I'll add you as a collaborator on the other PR and get it in asap.

@aws-cdk-automation aws-cdk-automation dismissed their stale review September 8, 2023 15:08

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

@pistazie
Copy link
Author

pistazie commented Sep 8, 2023

hi @pistazie I went to update the snapshot for you but it looks like I don't have permissions to update to the pitch-io fork.

Instead, I've created #27068. If you want, you can update the snapshot on this PR before the other one is merged and we'll go with yours. Otherwise, I'll add you as a collaborator on the other PR and get it in asap.

thanks for picking this up @kaizencc . Would love to be a contributor. also fixed snapshots (hopefully well)

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Sep 8, 2023
@kaizencc
Copy link
Contributor

kaizencc commented Sep 8, 2023

@pistazie

#27068 (comment)

Are you down to move the timeout to 890? the custom resource lambda times out after 15 minutes, which is the max.

To update the integ test real fast you can run yarn integ test/aws-route53/test/integ.delete-existing-record-set.js --update-on-failed --dry-run

@kaizencc
Copy link
Contributor

kaizencc commented Sep 8, 2023

fyi I went and called out this exact scenario in #27072. Basically, its really tough for us to accept contributions without allowing commits to your branch, since not only does that mean I can't do it but neither can our automation that syncs with main. In short, to accept contributions we need contributors to develop on a fork in their own personal account (the issue here), and not on their fork's main branch.

Going to close this over the other one for this exact reason, but thank you for the contribution and highlighting a nasty bug in our custom resource.

@kaizencc kaizencc closed this Sep 8, 2023
mergify bot pushed a commit that referenced this pull request Sep 8, 2023
…it time (#27068)

This PR updates the wait time of the `delete-existing-record-set-handler` from 60 to 1800 seconds.

Commit 399b6bb upgraded the handler to use the AWS SDK V3 (was V2) and made the following change:

```
-  await route53.waitFor('resourceRecordSetsChanged', { Id: changeResourceRecordSets.ChangeInfo.Id }).promise();
+  await waitUntilResourceRecordSetsChanged({ client: route53, maxWaitTime: 60 }, { Id: changeResourceRecordSets?.ChangeInfo?.Id })
```

The V2 `route53.waitFor` waits for 1800 seconds (see [reference](https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/Route53.html#waitFor-property))
> `Waits for the resourceRecordSetsChanged state by periodically calling the underlying [Route53.getChange()](https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/Route53.html#getChange-property) operation every 30 seconds (at most 60 times).` 

The new explicit configuration was set to 60 seconds which is too short and caused timeouts and failure in our cloudformation stacks

Supersedes and closes #27060 

Co-authored-by: Tom Roshko [hi@pistazie.rocks](mailto:hi@pistazie.rocks)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mikewrighton pushed a commit that referenced this pull request Sep 14, 2023
…it time (#27068)

This PR updates the wait time of the `delete-existing-record-set-handler` from 60 to 1800 seconds.

Commit 399b6bb upgraded the handler to use the AWS SDK V3 (was V2) and made the following change:

```
-  await route53.waitFor('resourceRecordSetsChanged', { Id: changeResourceRecordSets.ChangeInfo.Id }).promise();
+  await waitUntilResourceRecordSetsChanged({ client: route53, maxWaitTime: 60 }, { Id: changeResourceRecordSets?.ChangeInfo?.Id })
```

The V2 `route53.waitFor` waits for 1800 seconds (see [reference](https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/Route53.html#waitFor-property))
> `Waits for the resourceRecordSetsChanged state by periodically calling the underlying [Route53.getChange()](https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/Route53.html#getChange-property) operation every 30 seconds (at most 60 times).` 

The new explicit configuration was set to 60 seconds which is too short and caused timeouts and failure in our cloudformation stacks

Supersedes and closes #27060 

Co-authored-by: Tom Roshko [hi@pistazie.rocks](mailto:hi@pistazie.rocks)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@werkdny werkdny deleted the update-delete-existing-record-set-handler-wait-time branch September 15, 2023 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK node18-upgrade Any work (bug, feature) related to Node 18 upgrade p2 pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. pr-linter/exempt-integ-test The PR linter will not require integ test changes sdk-v3-upgrade Tag issues that are associated to SDK V3 upgrade. Not limited to CR usage of SDK only.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants