feat(ecs-patterns): add containerCpu and containerMemoryLimitMiB property to ApplicationLoadBalancedFargateService

[ren-yamanashi]

Issue #20638

Closes #20638

Reason for this change

ApplicationLoadBalancedFargateService did not allow you to specify the CPU and memory of the container.

Description of changes

• Add containerCpu property to ApplicationLoadBalancedFargateServiceProps
• Add containerMemoryLimitMiB property to ApplicationLoadBalancedFargateServiceProps

Description of how you validated changes

Added both unit and integration tests.

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

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

[go-to-k]

Could you change the PR title from "feat(application-load-balanced-fargate-service): ..." to "feat(ecs-patterns): add containerCpu and containerMemoryLimitMiB property to ApplicationLoadBalancedFargateService"? Because we should specify the module name, not the construct name.

[ren-yamanashi]

@go-to-k

Could you change the PR title from "feat(application-load-balanced-fargate-service): ..." to "feat(ecs-patterns): add containerCpu and containerMemoryLimitMiB property to ApplicationLoadBalancedFargateService"? Because we should specify the module name, not the construct name.

Thank you for the suggestion. I have updated the PR title. Please check the changes at your convenience.

[go-to-k]

Thanks for this PR! I made some comments.

[ren-yamanashi]

@go-to-k

Thanks for the review! I have fixed it, so please check it out!

(A link to the fix commit is provided in reply to the thread.)

[go-to-k]

If the containerCpu and the containerMemoryLimitMiB are larger than (or equal to) the cpu and the memoryLimitMiB, will CloudFormation deploy errors occur?
If so, it is good to add validations for them. (and also good to add corresponding unit tests)

for example:

      if (
        props.containerCpu &&
        !Token.isUnresolved(props.containerCpu) &&
        props.cpu &&
        !Token.isUnresolved(props.cpu) &&
        props.containerCpu > props.cpu // or `props.containerCpu >= props.cpu`
      ) {
        throw new Error('containerCpu must be less than or equal to cpu');
      }
      // The same applies to memory.
      // ...

see: https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-ecs-patterns/lib/fargate/application-load-balanced-fargate-service.ts#L108-L119

As with the validateHealthyPercentage method, it may also be possible to check for values that are not negative. (If CFn fails for negative values.)

[shikha372]

Thank you @ren-yamanashi for defining the alternate approach in detail, I would not modify the current behaviour of the construct by setting the default for props.cpu, I believe in case, value of containerCpu and cpu doesn't align with each other, this would result in a failed deployment (please let me know if that is not the case), given that if intention by adding this validation is to fail fast before the deployment itself, can we add a warning instead of an error in the function so that in case these values change in near future it will not block customer deployments.

[aws-cdk-automation]

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.

Pull request has been modified.

[gracelu0]

Hi @ren-yamanashi , discussed with @shikha372 and the suggestion is to validate using this.taskDefinition.cpu since FargateTaskDefinition sets a default for cpu. i.e.

this.validateContainerCpu(props.containerCpu, this.taskDefinition.cpu);

It looks like we will need to surface cpu in the FargateTaskDefinition class as a public readonly member in order to do this. Let us know if you're still planning to work on this, otherwise we can take over to get it merged (we will make sure to credit you as a co-author :) ). Thanks!

Pull request has been modified.

[gracelu0]

Thank you for contributing!

[mergify]

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

[mergify]

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

[gracelu0]

@Mergifyio update

[mergify]

update

❌ Mergify doesn't have permission to update

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/analytics-metadata-updater.yml without workflows permission

Pull request has been modified.

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[mergify]

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

[github-actions]

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.