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

[go-to-k]

I approved, but the PR doesn't have the label.

Could you run PR Linter again?

[go-to-k]

Could you try making some changes to the description of this PR without running CodeBuild CI?

[ren-yamanashi]

@go-to-k

I ran PR Linter without runnning CodeBuild again, but the PR doesn't have the label...
Looking at other PRs, it was labeled successfully, but is there a problem with what I submitted?

[go-to-k]

It appears that the status is CHANGES_REQUESTED and the communityApproved is false even though it has been approved: https://github.com/aws/aws-cdk/actions/runs/10669155243/job/29570426536

raw data: 
{
  "go-to-k": {
    "id": 2225629631,
    "node_id": "PRR_kwDOBk6Df86EqGm_",
    "user": {
      "login": "go-to-k",
      "id": 24818752,
      "node_id": "MDQ6VXNlcjI0ODE4NzUy",
      "avatar_url": "https://avatars.githubusercontent.com/u/24818752?u=e284689a061d1e9378ffd7b4e76bdc5320bfc45a&v=4",
      "gravatar_id": "",
      "url": "https://api.github.com/users/go-to-k",
      "html_url": "https://github.com/go-to-k",
      "followers_url": "https://api.github.com/users/go-to-k/followers",
      "following_url": "[https://api.github.com/users/go-to-k/following{/other_user}](https://api.github.com/users/go-to-k/following%7B/other_user%7D)",
      "gists_url": "[https://api.github.com/users/go-to-k/gists{/gist_id}](https://api.github.com/users/go-to-k/gists%7B/gist_id%7D)",
      "starred_url": "[https://api.github.com/users/go-to-k/starred{/owner}{/repo}](https://api.github.com/users/go-to-k/starred%7B/owner%7D%7B/repo%7D)",
      "subscriptions_url": "https://api.github.com/users/go-to-k/subscriptions",
      "organizations_url": "https://api.github.com/users/go-to-k/orgs",
      "repos_url": "https://api.github.com/users/go-to-k/repos",
      "events_url": "[https://api.github.com/users/go-to-k/events{/privacy}](https://api.github.com/users/go-to-k/events%7B/privacy%7D)",
      "received_events_url": "https://api.github.com/users/go-to-k/received_events",
      "type": "User",
      "site_admin": false
    },
    "body": "",
    "state": "CHANGES_REQUESTED",
    "html_url": "https://github.com/aws/aws-cdk/pull/30920#pullrequestreview-2225629631",
    "pull_request_url": "https://api.github.com/repos/aws/aws-cdk/pulls/30920",
    "author_association": "CONTRIBUTOR",
    "_links": {
      "html": {
        "href": "https://github.com/aws/aws-cdk/pull/30920#pullrequestreview-2225629631"
      },
      "pull_request": {
        "href": "https://api.github.com/repos/aws/aws-cdk/pulls/30920"
      }
    },
    "submitted_at": "2024-08-07T16:14:56Z",
    "commit_id": "ef494bd1fe8f15fcb4d5d60e4b804cdfc8123366"
  }
}

evaluation:  {
  "draft": false,
  "mergeable_state": "blocked",
  "maintainerRequestedChanges": false,
  "maintainerApproved": false,
  "communityRequestedChanges": true,
  "communityApproved": false,
  "userRequestsExemption": false
}

Apparently there was too much history to get all the latest data.

https://github.com/aws/aws-cdk/blob/main/tools/@aws-cdk/prlint/lint.ts#L376

  private async assessNeedsReview(
    pr: Pick<GitHubPr, 'mergeable_state' | 'draft' | 'labels' | 'number'>,
  ): Promise<void> {
    const reviews = await this.client.pulls.listReviews(this.prParams);
    console.log(JSON.stringify(reviews.data));

I will consult with the CDK team.

P.S. I will take the problem in the PR: #31290

[ren-yamanashi]

Thank you very much!

[go-to-k]

@ren-yamanashi

Hi, the fix PR has been merged!

#31290

I approved it again and the review label was successfully attached.

[ren-yamanashi]

Thank you for your quick response!

[shikha372]

To understand it better for review, seeing the original issue i think there was a suggestion that we could use task definition of fargate to specify cpu and memory limits, is that not possible for this use case..?

[ren-yamanashi]

@shikha372

When specifying taskDefinition as a props of ApplicationLoadBalancedFargateService, the newly added containerCpu and containerMemoryLimitMiB props in this MR do not work.

See: https://github.com/aws/aws-cdk/blob/89290c648a2234479802e1218b6dd2e47f7b1b33/packages/aws-cdk-lib/aws-ecs-patterns/lib/fargate/application-load-balanced-fargate-service.ts#L87-%23L99

so, I think it impossible.

[shikha372]

where are these defaults defined ?

[ren-yamanashi]

@shikha372

I used this default value as defined in FargateTaskDefinitionProps.

See:

aws-cdk/packages/aws-cdk-lib/aws-ecs/lib/fargate/fargate-task-definition.ts

Line 38 in 3e9e0a8

* @default 256

Should I write the code as follows?

  private validateContainerCpu(containerCpu?: number, cpu: number = 256) { // default value for FargateTaskDefinitionProps.cpu is 256

[shikha372]

Thanks @ren-yamanashi , I see the default values are in fargate task definition and not being set under props.cpu of ApplicationLoadBalancer class, I would prefer if we keep the check without default value as we might miss the update in case it changes in future as well.

[ren-yamanashi]

@shikha372

Thank you for your comment.

In the current validateContainerCpu method, it compares the values of containerCpu and cpu and returns an error if containerCpu exceeds cpu.

As you pointed out, referencing the default value of FargateServiceBaseProps.cpu in ApplicationLoadBalancedFargateService’s validateContainerCpu could indeed lead to an issue where we might miss the update in case it changes in future as well.

However, avoiding the use of the default value of FargateServiceBaseProps.cpu within ApplicationLoadBalancedFargateService to address this concern could lead to the following problem:

validateContainerCpu method might not validate properly

If we don’t reference the default value of FargateServiceBaseProps.cpu, the cpu param in validateContainerCpu would need to be optional.
(because props.cpu is optional)

  private validateContainerCpu(containerCpu?: number, cpu?: number) {
    /**
     * ... Implementation of the method
     */
  }

In this scenario, it becomes challenging to validate cases where:

• containerCpu is set, but cpu is not.

In this cases, an error should be returned if containerCpu exceeds the default value of FargateServiceBaseProps.cpu (256), but without referencing the default value within the method, determining if an error should be returned becomes difficult.

---

Possible Approaches for Resolution

Taking these considerations into account, I suggest the following approach. What could you think?
Also, if there are any other good ways, could you please let me know?

Note: this approaches would result in referencing the default value of FargateServiceBaseProps.cpu in ApplicationLoadBalancedFargateService, but I thought it could be more appropriate than the current implementation, so I am suggesting them.

Override props.cpu in ApplicationLoadBalancedFargateServiceProps

override props.cpu in ApplicationLoadBalancedFargateServiceProps and pass it to the FargateTaskDefinition argument within ApplicationLoadBalancedFargateService, as shown below:

export interface ApplicationLoadBalancedFargateServiceProps extends ApplicationLoadBalancedServiceBaseProps, FargateServiceBaseProps {
  /**
   * ... some comments
   *
   * @default 256
   */
  readonly cpu?: number;

 /**
  * ... some props
  */
}

export class ApplicationLoadBalancedFargateService extends ApplicationLoadBalancedServiceBase {
  // ... some implementation
  constructor(scope: Construct, id: string, props: ApplicationLoadBalancedFargateServiceProps = {}) {
    // ... some implementation

      this.taskDefinition = new FargateTaskDefinition(this, 'TaskDef', {
        cpu: props.cpu ?? 256,
        // ... some props
      });
  }
}   

[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]

AWS CodeBuild CI Report

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

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