fix(ecs-patterns): memory limit is not set at the container level #21201

gnagy · 2022-07-18T11:49:48Z

I ran into an issue when deploying a Java application on Fargate, where the container kept getting killed because of out-of-memory condition. Setting memoryLimitMiB also on container fixed the problem, based on:

https://aws.amazon.com/blogs/containers/how-amazon-ecs-manages-cpu-and-memory-resources/

[Update 12/11/2020] Some applications are container-aware and can configure themselves to take full advantage of the resources available inside the container. The latest versions of Java are a good example of this pattern. For this reason, some of these applications work best when CPU and memory resources are explicitly configured at the container level, in addition to the configuration at the task level. In other words, while containers without specific resource configurations can nominally access all task resources, aws/amazon-ecs-agent#1735.

I initially asked about this in #13127

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-07-18T11:49:55Z

TheRealAmazonKendra

Thank you for your contribution. A couple quick initial comments:

In general this looks like a very straight forward change but we will still need unit tests on it.
This change should probably be made on all the Fargate service types, not just here.
Make sure the build succeeds. Currently it's failing on a formatting issue but I suspect when that's fixed it will fail on the integration tests due to this change.

based on ecs patterns

Pull request has been modified.

TheRealAmazonKendra

Thanks for the update! The build is still failing on formatting issues. Please run the linter locally to find the locations causing the failure. Additionally, this change will change the output of tests in both this package and almost certainly in other packages that use these constructs in their integration tests. It may be beneficial to run the build locally so you can find those. It's also fine to just keep pushing changes and update based off the build failure if the local build is too laborious to run. It takes quite a while.

Pull request has been modified.

gnagy · 2022-07-21T09:05:26Z

Hi, I think I got the code and tests working, and now the build is still failing with Error: Some changes were destructive!. This sounds scary and not sure what it means. Can you help me with what are the next steps?

TheRealAmazonKendra

This the change I expected it to need when I mentioned above that it will probably cause the integration tests to fail. Because you're adding these fields, they now show up in the output of the integration tests. Just go ahead and add those values in the expected output and this change looks good to go, as far as I'm concerned.

Don't be too surprised if these same failures show up elsewhere that happens to use these constructs in their integration tests. They'll just need the same update.

Pull request has been modified.

TheRealAmazonKendra

🎉Looks good! Thank you for all your work on this!

mergify · 2022-07-22T20:55:35Z

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).

Pull request has been modified.

TheRealAmazonKendra · 2022-07-25T20:53:03Z

@Mergifyio update

mergify · 2022-07-25T20:53:09Z

update

✅ Branch has been successfully updated

mergify · 2022-07-25T20:53:11Z

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).

TheRealAmazonKendra · 2022-07-25T20:55:08Z

@Mergifyio refresh

mergify · 2022-07-25T20:55:36Z

refresh

✅ Pull request refreshed

mergify · 2022-07-25T21:29:06Z

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).

aws-cdk-automation · 2022-07-25T22:14:37Z

AWS CodeBuild CI Report

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

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

mergify · 2022-07-25T22:15:14Z

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).

…s#21201) I ran into an issue when deploying a Java application on Fargate, where the container kept getting killed because of out-of-memory condition. Setting memoryLimitMiB also on container fixed the problem, based on: https://aws.amazon.com/blogs/containers/how-amazon-ecs-manages-cpu-and-memory-resources/ > [Update 12/11/2020] Some applications are container-aware and can configure themselves to take full advantage of the resources available inside the container. The latest versions of Java are a good example of this pattern. For this reason, some of these applications work best when CPU and memory resources are explicitly configured at the container level, in addition to the configuration at the task level. In other words, while containers without specific resource configurations can nominally access all task resources, aws/amazon-ecs-agent#1735. I initially asked about this in aws#13127 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] 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*

…evel (#21201)" This reverts commit f2098b7.

…evel" (#21530) This reverts #21201 due to #21484: Recent update breaks FargateService L3 constructs which have added additional containers to the task definition Closes #21484 Re-opens #13127 ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] 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*

…s#21201) I ran into an issue when deploying a Java application on Fargate, where the container kept getting killed because of out-of-memory condition. Setting memoryLimitMiB also on container fixed the problem, based on: https://aws.amazon.com/blogs/containers/how-amazon-ecs-manages-cpu-and-memory-resources/ > [Update 12/11/2020] Some applications are container-aware and can configure themselves to take full advantage of the resources available inside the container. The latest versions of Java are a good example of this pattern. For this reason, some of these applications work best when CPU and memory resources are explicitly configured at the container level, in addition to the configuration at the task level. In other words, while containers without specific resource configurations can nominally access all task resources, aws/amazon-ecs-agent#1735. I initially asked about this in aws#13127 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] 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*

…evel" (aws#21530) This reverts aws#21201 due to aws#21484: Recent update breaks FargateService L3 constructs which have added additional containers to the task definition Closes aws#21484 Re-opens aws#13127 ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] 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*

fix(aws-ecs-patterns): Set memoryLimitMiB also on container

b481225

aws-cdk-automation requested a review from a team July 18, 2022 11:50

github-actions bot added the p2 label Jul 18, 2022

TheRealAmazonKendra changed the title ~~fix(aws-ecs-patterns): Set memoryLimitMiB also on container~~ fix(ecs-patterns): memory limit is not set at the container level Jul 18, 2022

TheRealAmazonKendra previously requested changes Jul 18, 2022

View reviewed changes

gnagy added 2 commits July 19, 2022 12:00

Add cpu and memory limit fix to all fargate patterns, add test

f09c937

based on ecs patterns

Merge branch 'main' into fix/aws-ecs-patterns-fargate-memory-limit

0ab6f91

TheRealAmazonKendra previously requested changes Jul 19, 2022

View reviewed changes

Fix lint errors

f43eff6

gnagy added 2 commits July 20, 2022 17:11

Fix tests :fingerscrossed:

0c68a47

Fix Cpu and Memory types in expected template output

da2d244

TheRealAmazonKendra previously requested changes Jul 21, 2022

View reviewed changes

Fix integration tests

c3cdc92

TheRealAmazonKendra previously approved these changes Jul 22, 2022

View reviewed changes

mergify bot and others added 2 commits July 22, 2022 21:08

Merge branch 'main' into fix/aws-ecs-patterns-fargate-memory-limit

1c14336

Fix one more test

ca3f612

TheRealAmazonKendra approved these changes Jul 25, 2022

View reviewed changes

Merge branch 'main' into fix/aws-ecs-patterns-fargate-memory-limit

eb4212f

Merge branch 'main' into fix/aws-ecs-patterns-fargate-memory-limit

f31e646

mergify bot merged commit f2098b7 into aws:main Jul 25, 2022

This was referenced Aug 5, 2022

(ApplicationLoadBalancedFargateService): Container size won't update #13127

Open

(ecs-patterns): Recent update breaks FargateService L3 constructs which have added additional containers to the task definition #21484

Closed

TheRealAmazonKendra added a commit that referenced this pull request Aug 10, 2022

Revert "fix(ecs-patterns): memory limit is not set at the container l…

aaecc93

…evel (#21201)" This reverts commit f2098b7.

TheRealAmazonKendra mentioned this pull request Aug 10, 2022

Revert "fix(ecs-patterns): memory limit is not set at the container level" #21530

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ecs-patterns): memory limit is not set at the container level #21201

fix(ecs-patterns): memory limit is not set at the container level #21201

gnagy commented Jul 18, 2022

gitpod-io bot commented Jul 18, 2022

TheRealAmazonKendra left a comment •

edited

Loading

TheRealAmazonKendra left a comment

gnagy commented Jul 21, 2022

TheRealAmazonKendra left a comment

TheRealAmazonKendra left a comment

mergify bot commented Jul 22, 2022

TheRealAmazonKendra commented Jul 25, 2022

mergify bot commented Jul 25, 2022

mergify bot commented Jul 25, 2022

TheRealAmazonKendra commented Jul 25, 2022

mergify bot commented Jul 25, 2022

mergify bot commented Jul 25, 2022

aws-cdk-automation commented Jul 25, 2022

mergify bot commented Jul 25, 2022

fix(ecs-patterns): memory limit is not set at the container level #21201

fix(ecs-patterns): memory limit is not set at the container level #21201

Conversation

gnagy commented Jul 18, 2022

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Jul 18, 2022

TheRealAmazonKendra left a comment • edited Loading

Choose a reason for hiding this comment

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

gnagy commented Jul 21, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

mergify bot commented Jul 22, 2022

TheRealAmazonKendra commented Jul 25, 2022

mergify bot commented Jul 25, 2022

✅ Branch has been successfully updated

mergify bot commented Jul 25, 2022

TheRealAmazonKendra commented Jul 25, 2022

mergify bot commented Jul 25, 2022

✅ Pull request refreshed

mergify bot commented Jul 25, 2022

aws-cdk-automation commented Jul 25, 2022

AWS CodeBuild CI Report

mergify bot commented Jul 25, 2022

TheRealAmazonKendra left a comment •

edited

Loading